Details
- Reviewers
bolsson smaug Gijs - Group Reviewers
dom-core firefox-desktop-core-reviewers Restricted Project desktop-theme-reviewers frontend-codestyle-reviewers - Commits
- rFIREFOXAUTOLAND1f5d422cf8f6: Bug 1991135, part 1 - Add support for <meta name=rating> behind a pref - r=dom…
- Bugzilla Bug ID
- 1991135
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
Event Timeline
| docshell/base/BrowsingContext.h | ||
|---|---|---|
| 288 | RTA? | |
| docshell/base/nsDocShell.cpp | ||
| 3571 | There is some inconsistency here. Some stuff talks about restricted content and some restricted to adults. | |
| docshell/base/nsDocShell.h | ||
| 785 | What does RTA mean? This could use some comment. | |
| dom/base/Document.cpp | ||
| 11882 | So this doesn't help if someone adds the element using JS. But maybe that is anyhow too late and it is ok to just disable js at that point | |
| docshell/base/nsDocShell.cpp | ||
|---|---|---|
| 3571 | I'll settle on restricted content in Gecko. The confusion comes in because the meta tag is called the "Restricted to Adults" label by its creator, hence RTA. Because of that being the history, and because its current only use I see is in adult content, I'd like to keep the references to "adult content" in the body text of the about:restricted page, with the expectation that we would swap out the strings based on the content attribute of the meta tag if the ontology grows, or if the semantics of the existing content changes drastically. So: "restricted content" and "adult content", but no explicit mentions of "restricted to adults" in the strings. | |
| docshell/base/nsDocShell.h | ||
| 785 | Restricted to Adults... refactoring thanks to another comment and adding a comment. | |
Code analysis found 3 defects in diff 1131192:
- 3 defects found by eslint (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1131192.
r- because I don't think this builds
| docshell/base/nsDocShell.cpp | ||
|---|---|---|
| 4128 | This should likely be a strong ref, given that TerminateParserAndDisableScripts() does non-trivial things. | |
| 4131 | More a generic error page question, are data: urls or srcdoc or javascript: urls shown in a reasonable way in the error page? | |
| dom/base/Document.cpp | ||
| 11873 | This is referring to DisplayRTAError, which doesn't seem to exist anymore. | |
| docshell/base/nsDocShell.cpp | ||
|---|---|---|
| 4131 | If the URI passed from the Document doesn't have a host, then we just fall back to a generic error message. This has me also updating the URI passed from the document to be NodePrincipal()->GetURI(), instead of mDocumentURI. | |
| dom/base/Document.cpp | ||
| 11873 | This is why I should stop working on just one more thing before calling it a night :) | |
I moved the Fluent to a non-translated folder because this is far enough away from release and I haven't finalized content.
Code analysis found 3 defects in diff 1131396:
- 3 defects found by eslint (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1131396.
| docshell/base/nsDocShell.cpp | ||
|---|---|---|
| 4128 | Do we need the URI param, which is taken from the document, when we anyhow access document here. | |
| dom/base/Document.cpp | ||
| 11874 | This isn't the right URI. NodePrincipal can be for example a null principal, in which case GetURI() will return some moz-nullprincipal:{..} thingie. | |
| docshell/base/nsDocShell.cpp | ||
|---|---|---|
| 4128 | You need to null check doc. This is called asynchronously so the whole DocShell might have been destroyed (not deleted yet though) so GetDocument may return null. | |
| 4131 | I see. Yeah, I guess there are cases when nodeprincipal's uri might be reasonable, but there certainly are cases when documenturi is better (all the sandboxed stuff) | |
| docshell/base/nsDocShell.h | ||
| 788 | The return value isn't every used for anything, so perhaps drop that? | |
(I very much did not review the frontend side, like the actual error page or anything else under toolkit/)
Some super high level comments... will leave this for now but can come back to it on Monday if nobody else does another review pass...
| toolkit/content/aboutRestricted.css | ||
|---|---|---|
| 7–9 ↗ | (On Diff #1132529) | Is there a design for this page? |
| toolkit/content/aboutRestricted.html | ||
| 21 ↗ | (On Diff #1132529) | Why do we not want this to be localized? |
| toolkit/content/aboutRestricted.js | ||
| 5 ↗ | (On Diff #1132529) | Let's remove this, it doesn't do anything anymore (I'm in the process of removing all the other annotations like this as they suggest they work, and also recently updated documentation). Instead, please add to this list. That will also allow you to get rid of all the no-undef exceptions below. |
| 25–26 ↗ | (On Diff #1132529) | This can be non-empty for data and file URIs which is probably going to be wrong... |
| toolkit/content/jar.mn | ||
| 35–37 | Could we put this in its own directory, please (both in source and in the eventual mapped location in omni.ja ? | |
| toolkit/content/aboutRestricted.css | ||
|---|---|---|
| 7–9 ↗ | (On Diff #1132529) | Not at this stage, but it follows other pages closely. |
| toolkit/content/aboutRestricted.html | ||
| 21 ↗ | (On Diff #1132529) | I haven't finalized the content and don't want to waste any localizer time. |
| toolkit/content/aboutRestricted.js | ||
| 5 ↗ | (On Diff #1132529) | I was wondering why this wasn't working! |
| 25–26 ↗ | (On Diff #1132529) | I'll prune this down to http(s). schemes. |
Code analysis found 2 defects in diff 1133380:
- 2 defects found by eslint (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1133380.
Thanks and apologies for the slow reviews here. This is close but I noticed a few different things, see inline comments.
| toolkit/content/aboutRestricted/aboutRestricted.css | ||
|---|---|---|
| 9 | I don't think any other pages are using this; I'd use the same styling as certerror to get the yellow warning triangle like those, at least for now. For context, we are in the process of switching to a different style of error page anyway, cf. https://searchfox.org/firefox-main/search?q=security.certerrors.felt-privacy-v1 , https://bugzilla.mozilla.org/show_bug.cgi?id=1990918 . So this is all a little awkward at the moment; especially without support from UX/content design at the moment. Based on your comments I'm assuming we'll iterate before shipping, but just wanted to flag that. | |
| toolkit/content/aboutRestricted/aboutRestricted.html | ||
| 20 | This is missing a value, so there's no icon on the tab when this page is shown. | |
| 41–43 | If this is a module it should be .mjs. | |
| toolkit/content/aboutRestricted/aboutRestricted.js | ||
| 12 ↗ | (On Diff #1133380) | The patch is still missing the changes to the eslint rollouts file, with that this should go away - https://searchfox.org/firefox-main/rev/2c8e9bccfd6ce951ceda8d46ab619f461b7fd0bc/eslint-file-globals.config.mjs#363-389 |
| 21 ↗ | (On Diff #1133380) | Is it double-encoded? I'd expect the search params object to already decode. |
| 42–47 ↗ | (On Diff #1133380) | Controversially, this is running off an event, which is what most of these pages do - but the script is loaded after the markup that this depends on, so there's no actual need to wait for the event, AIUI. Calling init() immediately (from the global scope) would be less code, and also hopefully guarantee that async l10n runs before we try to paint the error page (certainly updating the l10n info sooner gives us a better shot anyway). |