Fork and pref-off the new error pages

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: trisha, Assigned: trisha)

Tracking

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

No description provided.
Posted patch bug1463748.patch (obsolete) — Splinter Review
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 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+
Posted patch bug1463748.patch (obsolete) — Splinter Review
Attachment #8982256 - Attachment is obsolete: true
Attachment #8983090 - Flags: review?(jhofmann)
Posted patch bug1463748.patch (obsolete) — Splinter Review
Attachment #8983090 - Attachment is obsolete: true
Attachment #8983090 - Flags: review?(jhofmann)
Attachment #8984095 - Flags: review?(jhofmann)
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)
Posted patch bug1463748.patch (obsolete) — Splinter Review
Attachment #8984095 - Attachment is obsolete: true
Attachment #8984194 - Flags: review?(jhofmann)
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)
Attachment #8984194 - Attachment is obsolete: true
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+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac73efd7dea3
Fork and pref-off the new error pages r=johannh
https://hg.mozilla.org/mozilla-central/rev/ac73efd7dea3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.