Closed Bug 897121 Opened 7 years ago Closed 6 years ago

Defect - Browser error: FlyoutPanelsUI.PrefsFlyout is undefined

Categories

(Firefox for Metro Graveyard :: Flyouts, defect, P2)

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jimm, Assigned: TimAbraldes)

References

Details

(Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3)

Attachments

(1 file, 2 obsolete files)

[JavaScript Error: "FlyoutPanelsUI.PrefsFlyout is undefined" {file: "chrome://mozapps/content/extensions/setting.xml" line: 71}]

I see this a lot while running tests.
Summary: Browser error: FlyoutPanelsUI.PrefsFlyout is undefined → Defect - Browser error: FlyoutPanelsUI.PrefsFlyout is undefined
Whiteboard: feature=defect c=tbd u=tbd p=0
Priority: -- → P3
The constructor for the XBL binding, "setting," calls its "preferenceChanged" handler [1].

In browser.xul, there is a setting whose "preferenceChanged" handler tries to call a function defined in our prefs flyout [2].

FlyoutPanelsUI.PrefsFlyout is not initialized until FlyoutPanelsUI.init is called, so when the "setting" constructor from [1] runs and tries to call the "preferenceChanged" handler from [2], we see that FlyoutPanelsUI.PrefsFlyout is undefined.

I'm not yet sure what the best approach to fixing this would be. Perhaps we should hold off on setting the "preferenceChanged" handler until the initialization of the prefs flyout. At that point we could also check the value of the pref and make sure we set its initial value correctly.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/setting.xml?rev=f56958fbcc5b#23
[2] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul?rev=bf035bca7f65#707
Based on the analysis above, I'm betting the user impact of this bug is that the "do not track" item in the prefs flyout doesn't accurately reflect the "do not track" preference when the metro browser starts up.

That would mean that this is causing bug 898641 and possibly also bug 898643.
Priority: P3 → P2
Depends on: 898643, 898641
Blocks: 831958, 831965
Whiteboard: feature=defect c=tbd u=tbd p=0 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0
Assignee: nobody → tabraldes
Blocks: metrov1it12
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3
Blocks: 899528
Attached patch 897121.patch (obsolete) — Splinter Review
Turns out we were trying to do too much. If we just let the <setting>s do their thing, they work perfectly.

This also fixes bug 898641 and bug 898643.
Attachment #783851 - Flags: review?(rsilveira)
Blocks: 898641
No longer depends on: 898641
Blocks: 898643
No longer depends on: 898643
Hey Tim, will bug 898641 and bug 898643 become resolved after Bug 897121 lands or will any work need to be done to them?
Flags: needinfo?(tabraldes)
(In reply to Marco Mucci [:MarcoM] from comment #4)
> Hey Tim, will bug 898641 and bug 898643 become resolved after Bug 897121
> lands or will any work need to be done to them?

From my testing, it looks like no additional work will be needed.
Flags: needinfo?(tabraldes)
That's great.  Thanks for the update Tim.
Comment on attachment 783851 [details] [diff] [review]
897121.patch

Review of attachment 783851 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
@@ -46,5 @@
> -  onDNTPreferenceChanged: function onDNTPreferenceChanged() {
> -    let selected = this._elements.dntNoPref.selected;
> -
> -    // When "tell sites nothing about my preferences" is selected, disable do not track.
> -    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", !selected);

I think this is still needed.  Note that there are two different privacy.donottrackheader.* preferences, and in some cases we need to update both.  The <setting> only updates one of them.
Comment on attachment 783851 [details] [diff] [review]
897121.patch

Review of attachment 783851 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this! The only issue is what Matt pointed out.

::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
@@ -46,5 @@
> -  onDNTPreferenceChanged: function onDNTPreferenceChanged() {
> -    let selected = this._elements.dntNoPref.selected;
> -
> -    // When "tell sites nothing about my preferences" is selected, disable do not track.
> -    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", !selected);

Yes, this is still needed. The "enabled" pref enables/disables the header while "value" pref is for your tracking options. You can do the binding on init. You can use http://donottrack.us/ to verify.
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> I think this is still needed.  Note that there are two different
> privacy.donottrackheader.* preferences, and in some cases we need to update
> both.  The <setting> only updates one of them.

Turns out I had 'privacy.donottrackheader.enabled' set to true, so modifying 'privacy.donottrackheader.value' was working perfectly (as tested on http://donottrack.us and also in the flyout UI).

From looking at the way 'privacy.donottrackheader.enabled' and 'privacy.donottrackheader.value' are used [1], I don't see a reason for having both of them. ISTM that we have 3 distinct desired behavioral outcomes (send a 0 in the dnt header, send a 1 in the dnt header, don't send the dnt header) and that we have 2 prefs which are both capable of representing 3 states (-1, 0, 1 for privacy.donottrackheader.value and true,false,non-existent for privacy.donottrackheader.enabled).

Any insight into why we have these two separate prefs?

[1] https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp?rev=14bd6018f287#418


In any case, the attached patch keeps the two prefs in sync with each other and with the UI in our prefs flyout.
Attachment #783851 - Attachment is obsolete: true
Attachment #783851 - Flags: review?(rsilveira)
Attachment #783950 - Flags: review?(rsilveira)
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #9)
> Created attachment 783950 [details] [diff] [review]
> Patch v2
> 
> (In reply to Matt Brubeck (:mbrubeck) from comment #7)
> > I think this is still needed.  Note that there are two different
> > privacy.donottrackheader.* preferences, and in some cases we need to update
> > both.  The <setting> only updates one of them.
> 
> Turns out I had 'privacy.donottrackheader.enabled' set to true, so modifying
> 'privacy.donottrackheader.value' was working perfectly (as tested on
> http://donottrack.us and also in the flyout UI).
> 
> From looking at the way 'privacy.donottrackheader.enabled' and
> 'privacy.donottrackheader.value' are used [1], I don't see a reason for
> having both of them. ISTM that we have 3 distinct desired behavioral
> outcomes (send a 0 in the dnt header, send a 1 in the dnt header, don't send
> the dnt header) and that we have 2 prefs which are both capable of
> representing 3 states (-1, 0, 1 for privacy.donottrackheader.value and
> true,false,non-existent for privacy.donottrackheader.enabled).
> 
> Any insight into why we have these two separate prefs?
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpHandler.cpp?rev=14bd6018f287#418
> 
> 
> In any case, the attached patch keeps the two prefs in sync with each other
> and with the UI in our prefs flyout.

Maybe Sid knows? Cc'ing him.
Comment on attachment 783950 [details] [diff] [review]
Patch v2

Review of attachment 783950 [details] [diff] [review]:
-----------------------------------------------------------------

Nice solution using prefs observer!

::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
@@ +38,5 @@
>        SanitizeUI.init();
>        this._hasShown = true;
> +
> +      Services.prefs.addObserver("privacy.donottrackheader.value",
> +                                 this._prefObserver.bind(this),

nit: The bind call is fine, but you're not using 'this' at _prefObserver so it's unnecessary.
Attachment #783950 - Flags: review?(rsilveira) → review+
Attached patch Patch v3Splinter Review
(In reply to Rodrigo Silveira [:rsilveira] from comment #11)
> Comment on attachment 783950 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 783950 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice solution using prefs observer!

Thanks!

> ::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
> @@ +38,5 @@
> >        SanitizeUI.init();
> >        this._hasShown = true;
> > +
> > +      Services.prefs.addObserver("privacy.donottrackheader.value",
> > +                                 this._prefObserver.bind(this),
> 
> nit: The bind call is fine, but you're not using 'this' at _prefObserver so
> it's unnecessary.

Oops, good catch. Fixed in the attached patch.


Note1: Carrying forward r+.
Note2: Some of the logic in this patch is implemented in various places in fennec, Firefox desktop, and b2g. I'm not sure how, but it'd be really nice if we could centralize the logic (maybe in the pref service itself?)
Attachment #783950 - Attachment is obsolete: true
Attachment #783998 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/959c593c6bd3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.