Closed
Bug 1463748
Opened 7 years ago
Closed 6 years ago
Fork and pref-off the new error pages
Categories
(Firefox :: Security, enhancement, P3)
Firefox
Security
Tracking
()
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: trisha, Assigned: trisha)
References
Details
Attachments
(1 file, 4 obsolete files)
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
I have uploaded a work-in-progress patch for feedback. Can you please see if the pref name is okay? Also, do you think I'm going in the right direction with this? Thanks.
Attachment #8982256 -
Flags: feedback?(jhofmann)
Comment 2•7 years ago
|
||
Comment on attachment 8982256 [details] [diff] [review]
bug1463748.patch
Review of attachment 8982256 [details] [diff] [review]:
-----------------------------------------------------------------
This is going in a great direction! Please assign the URL as I mentioned below and then you can copy aboutNetError.xhtml and change something on the page to check whether it's working...
::: browser/components/about/AboutRedirector.cpp
@@ +149,5 @@
>
> + static bool sNCEPEnabledCacheInited = false;
> + if (!sNCEPEnabledCacheInited) {
> + Preferences::AddBoolVarCache(&AboutRedirector::sNewCertErrorPageEnabled,
> + "browser.securitynewcerterrorpage.enabled");
maybe browser.security.newcerterrorpage.enabled?
@@ +170,5 @@
> NS_ENSURE_SUCCESS(rv, rv);
> }
> +
> + if (path.EqualsLiteral("certerror")||
> + (sNewCertErrorPageEnabled && path.EqualsLiteral("certerror"))) {
this is the right condition, but you have one path.EqualsLiteral("certerror") too much in there, it can just be
if (sNewCertErrorPageEnabled && path.EqualsLiteral("certerror"))
@@ +175,5 @@
> + nsCOMPtr<nsINetUtil> netUtil =
> + do_GetService("chrome://browser/content/aboutNetError-new.xhtml", &rv);
> + NS_ENSURE_SUCCESS(rv, rv);
> + rv = netUtil->escapeURL(url);
> + NS_ENSURE_SUCCESS(rv, rv);
do_GetService is a getter for an XPCOM service (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/XPCOM_Interfaces#XPCOM_Services), so you can't assign the new URL like that. An easier way to do it would be:
url.AssignASCII("chrome://browser/content/aboutNetError-new.xhtml");
Attachment #8982256 -
Flags: feedback?(jhofmann) → feedback+
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8982256 -
Attachment is obsolete: true
Attachment #8983090 -
Flags: review?(jhofmann)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8983090 -
Attachment is obsolete: true
Attachment #8983090 -
Flags: review?(jhofmann)
Attachment #8984095 -
Flags: review?(jhofmann)
Comment 5•7 years ago
|
||
Comment on attachment 8984095 [details] [diff] [review]
bug1463748.patch
Review of attachment 8984095 [details] [diff] [review]:
-----------------------------------------------------------------
This looks great, just a few nits. Can you please run
hg copy browser/base/content/aboutNetError.xhtml browser/base/content/aboutNetError-new.xhtml --after
to make sure hg knows that we've just copied this file (so that blame gets preserved). Afterwards you'll probably have to do hg commit --amend again.
Thank you!
::: browser/app/profile/firefox.js
@@ +984,4 @@
> // Name of alternate about: page for certificate errors (when undefined, defaults to about:neterror)
> pref("security.alternate_certificate_error_page", "certerror");
>
> +//Indicates if new certificate error page (enabled) or not
nit: please add a space behind the //
@@ +984,5 @@
> // Name of alternate about: page for certificate errors (when undefined, defaults to about:neterror)
> pref("security.alternate_certificate_error_page", "certerror");
>
> +//Indicates if new certificate error page (enabled) or not
> +pref("browser.security.newcerterrorpage.enabled", true);
Should we default this to false for now?
::: browser/components/about/AboutRedirector.cpp
@@ +173,5 @@
> +
> + if (sNewCertErrorPageEnabled && path.EqualsLiteral("certerror")) {
> + url.AssignASCII("chrome://browser/content/aboutNetError-new.xhtml");
> + }
> +
please get rid of this whitespace
Attachment #8984095 -
Flags: review?(jhofmann)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8984095 -
Attachment is obsolete: true
Attachment #8984194 -
Flags: review?(jhofmann)
Comment 7•7 years ago
|
||
Comment on attachment 8984194 [details] [diff] [review]
bug1463748.patch
Review of attachment 8984194 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for correcting the nits, but are you sure that you did hg copy on the aboutNetError-new.xhtml file? Did that succeed? You can also just hg remove it and run
hg copy browser/base/content/aboutNetError.xhtml browser/base/content/aboutNetError-new.xhtml
and commit --amend again. Your patch should then indicate that you've made a copy of the file.
Can you try that? Thanks!
Attachment #8984194 -
Flags: review?(jhofmann)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8984194 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8985098 [details]
Bug 1463748 - Fork and pref-off the new error pages
https://reviewboard.mozilla.org/r/250806/#review257236
This looks great, thank you!
Attachment #8985098 -
Flags: review?(jhofmann) → review+
Comment 12•6 years ago
|
||
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac73efd7dea3
Fork and pref-off the new error pages r=johannh
Comment 13•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in
before you can comment on or make changes to this bug.
Description
•