1379338 - scriptify preferences XBL (i.e. convert preferences XBL to JS)

Status: RESOLVED FIXED

(Firefox :: Settings UI, task, P3)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

mossop: this is a mostly-conservative conversion from XBL bindings to JS objects for the XBL bindings used by the in-content-new preferences implementation.

I also did some code refactoring (removal of unused code, DRY, etc.) and optimization for perf (although I haven't yet measured the perf of this implementation relative to the original).  But I mostly conserved the original implementation, including some of the JS in the XBL bindings and most of the JS in the in-content-new directory.

I started this work to help estimate how long it would take to do this kind of conversation, not with the intent to land it (at least not by itself).  Now that it's done, however, I'm posting it here to see if you think it would be useful to land it (f.e. to improve the perf of that page, if indeed this does improve perf).

Comment 1

[Dave Townsend [:mossop]]

Comment on attachment 8884475 [details] [diff] [review]
scriptify preferences XBL

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

If we were going to land this I might use ES6 classes for the new work and separate it out into its own file so it could be reused easily. The name Preferences is problematic as it matches a Preferences.jsm so might want to bikeshed on that a little too.

Other than that this all looks like a fine conversion.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to Dave Townsend [:mossop] from comment #1)
> If we were going to land this I might use ES6 classes for the new work

Indeed, that would make sense for the Preference instances (and for the Preferences singleton, if we were to make it a constructor in a JSM, as described below).


> and
> separate it out into its own file so it could be reused easily.

I considered moving the implementation to its own file, and moving it to a separate script would be trivial.  It would also be relatively straightforward to move that script from browser/ back to toolkit/, for reuse by other toolkit-based apps.

But making that script a JSM would be harder, because the implementation accesses the DOM extensively (primarily to update elements when their prefs' values change).

In theory, it would be possible to convert the Preferences singleton to a constructor that takes a *window* object, which the Preferences instance—and the individual Preference instances—would then use to access the DOM of the page that constructed the instance.

But that would complicate the implementation, and it wouldn't particularly benefit from being a JSM.  It would still only be used by the about:preferences page, since there isn't other code that displays controls for manipulating preferences in the same way.  And we would have to be careful not to leak DOM objects when about:preferences is unloaded, given that JSMs outlive the contexts that import them.


> The name
> Preferences is problematic as it matches a Preferences.jsm so might want to
> bikeshed on that a little too.

Indeed.  I considered PrefModel.js[m], as the implementation somewhat resembles the "model" part of an MVC architecture (with some "controller" thrown in for good measure).  But I ultimately decided to stick it in preferences.js for the purposes of this exercise, since the original implementation had been in preferences.xml.

I'm happy to bikeshed on the name ad nauseum. ;-)

Comment 3

[Myk Melez [:myk] [@mykmelez]]

@mossop: I wrote this "test" <https://github.com/mykmelez/gecko/commit/24c0bb1be> to measure about:preferences startup perf (both initial time to initialize the page and mean time across 100 iterations) and ran it against both the current implementation and the one in this bug.

Here are five runs on the current implementation (on my fast Mac laptop):

>Initial time to initialize prefs page: 770.605.
>Mean time to initialize prefs page: 270.86295000000007.
>
>Initial time to initialize prefs page: 779.9200000000001.
>Mean time to initialize prefs page: 275.797.
>
>Initial time to initialize prefs page: 773.955.
>Mean time to initialize prefs page: 270.94775000000004.
>
>Initial time to initialize prefs page: 798.04.
>Mean time to initialize prefs page: 296.45694999999995.
>
>Initial time to initialize prefs page: 757.62.
>Mean time to initialize prefs page: 324.21815000000004.

And here are five runs on the new one:

>Initial time to initialize prefs page: 662.74.
>Mean time to initialize prefs page: 226.7580000000001.
>
>Initial time to initialize prefs page: 719.205.
>Mean time to initialize prefs page: 229.9592500000001.
>
>Initial time to initialize prefs page: 649.85.
>Mean time to initialize prefs page: 226.82820000000004.
>
>Initial time to initialize prefs page: 643.48.
>Mean time to initialize prefs page: 217.63655.
>
>Initial time to initialize prefs page: 672.485.
>Mean time to initialize prefs page: 221.69095.

I don't particularly trust the "initial time" numbers, which are easy to influence.  But the "mean time" numbers seem reasonable, and shows significant improvement for the implementation in this bug.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3271548d8230205e73dd3bebe2e91fd8496ad268

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3c22ce282b726f5f81d003c31bca2b22fd3b226

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8886494 [details] [diff] [review]
scriptify preferences XBL

Jared: this patch to convert the <preferences>, <preference>, and <prefpane> XBL bindings to JS objects/classes originated in an experiment to determine the cost/feasibility of converting XBL to JS in Firefox's codebase.  Although I didn't originally intend to land these changes, after analyzing the results and discussing them with Mossop and others, I concluded that it's worth considering.

Besides reducing our dependency on XBL, and improving code readability (IMHO) relative to the XBL implementation, the patch also improves about:preferences pageload perf, possibly resolving the issue in bug 1375870.

(To be fair: it isn't clear that the conversion to JS is itself responsible for improving perf.  While doing the conversion, I noticed and fixed a couple of obvious performance issues, and the perf win may be due to those changes.  It certainly was easier to identify and resolve them in a JS codebase than a XBL one, however.)

Along the way, I identified a few blocks of code that don't seem to be used anymore and removed them, namely the helpButtonCommand function and the gSyncPane.prefArray property.  But I mostly avoided any refactoring of the about:preferences codebase and made only the minimal changes necessary to enable it to work with a pure-JS implementation of the XBL bindings.

Note that this patch doesn't include conversions of the various sub-dialogs (Fonts, Colors, etc.).  I'm working on a followup that converts those as well, but I thought it worth separating those concerns, which will require some additional conversion (as they also use <prefwindow> elements).

All of the browser/components/preferences/in-content-new/ tests pass with these changes, although the number of tests has changed (because there are test scripts that assert for every DOM element in the page, and there are fewer DOM elements in the pages now that I've removed the <preferences> and <preference> elements).

Per mossop, I converted Preference and PrefPane to ES6 classes and moved the implementation to a separate file, toolkit/content/preferences.js.  I didn't rename it, although that name is overloaded (by Preferences.jsm, among other things), since I can't think of a better name, and it's consistent with the XBL bindings file preferences.xml.  I'm happy to consider alternatives, however.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Also adding needinfo request per note in Jared's Bugzilla name.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8886494 [details] [diff] [review]
scriptify preferences XBL

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

This is very cool! Thank you for picking this up, it will be a huge help in shifting from XUL to HTML.

Bug 1365133 should land soon and that will bitrot this work (nothing too crazy but files are moving around). I would prefer that you land your work on top of bug 1365133 since that bug has been trying to land now for a couple weeks and I don't want to delay it some more.

Marking feedback+ for now, and then once bug 1365133 lands we can rebase this and I'll do a more thorough review.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Evan and Ricky, please take a look at the patch here. I've asked that this not land until after bug 1365133, but you should be aware of the work here since it will change some of the underlying preferences implementation.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Bug 1365133 should land soon and that will bitrot this work (nothing too
> crazy but files are moving around). I would prefer that you land your work
> on top of bug 1365133 since that bug has been trying to land now for a
> couple weeks and I don't want to delay it some more.
> 
> Marking feedback+ for now, and then once bug 1365133 lands we can rebase
> this and I'll do a more thorough review.

Ok, that sounds good, I'll rebase this patch and then re-request review once that one lands.

Comment 11

[Ricky Chien [:rickychien] (inactive)]

We'll pay attention on this. thanks!

Comment 12

[Evan Tseng [:evanxd]]

Sure, thank you.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks :myk's comment over bug 1382170 now I know about this too. One thing to note that since re-org v2 (bug 1365133) the page have since loads even slower because of the reason below.

On the profile I pasted on that bug, I found that we've found a lot of time in <preferences> constructor (note the "s") simply iterate through it's <preference> children. This patch might help by get rid of that. 

If you are looking at getting this landed on Fx56, I probably shouldn't be working on that bug just yet.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Marking feedback+ for now, and then once bug 1365133 lands we can rebase
> this and I'll do a more thorough review.

Ok, I've rebased against bug 1365133.  I also included a "test" (browser_startup_perf.js) that measures the time it takes to load and initialize about:preferences (up to the point at which init_all emits the "Initialized" event) and computes an average across 100 loads.

On my machine, I see averages in the 346-373ms range before applying this patch; afterward, they're in the 197-230ms range.

(I don't expect to land this test; it's just there to demonstrate the perf improvements I'm seeing.)

I'm still considering the ergonomics of the API for registering preferences, but that doesn't affect reviewability, as changes to it can happen in followups.

Comment 15

[Florian Quèze [:florian]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)

> I don't particularly trust the "initial time" numbers, which are easy to
> influence.

I think the 'initial time' numbers are slow in large part due to bug 1375978.

It would be nice to compare the performance of this JS implementation against the XBL one once bug 1375870 is fixed, because the numbers for the existing implementation don't mean much until that bug is fixed.

I share Mossop's already expressed concern about reusing the 'Preferences' name, especially when you have some Preferences.get calls in your new code that really look like you are using Preferences.jsm (and will be confusing when searching across the whole codebase).

Preferences.jsm is in most cases mostly overhead, and we have bug 1357517 on file to remove it... but it would take a non-trivial amount of effort to get rid of it. If re-using 'Preferences' is really the best name here, maybe we should consider renaming the existing one, eg. to 'PreferencesDeprecated'.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(In reply to Florian Quèze [:florian] [:flo] from comment #15)

> It would be nice to compare the performance of this JS implementation
> against the XBL one once bug 1375870 is fixed, because the numbers for the
> existing implementation don't mean much until that bug is fixed.

Indeed!  Note that the primary goal here is to reduce our dependency on XBL and improve code readability, not to improve performance.  I just happened to implement some perf improvements in the process.

So I don't expect this implementation to be faster than the XBL one, once bug 1375870 and/or other perf issues in the XBL implementation are addressed.  And fixing those issues may be the faster route to improving perf, depending on how much more work is required to land them.


> I share Mossop's already expressed concern about reusing the 'Preferences'
> name, especially when you have some Preferences.get calls in your new code
> that really look like you are using Preferences.jsm (and will be confusing
> when searching across the whole codebase).
> 
> Preferences.jsm is in most cases mostly overhead, and we have bug 1357517 on
> file to remove it... but it would take a non-trivial amount of effort to get
> rid of it. If re-using 'Preferences' is really the best name here, maybe we
> should consider renaming the existing one, eg. to 'PreferencesDeprecated'.

I'm happy to rename things.  However, note that this script, which replaces the bindings in preferences.xml, provides a different kind of API than Preferences.jsm, which is a wrapper around XPCOM APIs.  So this work doesn't deprecate that module.

Also, there are two distinct naming questions:

1. the name of the script (currently preferences.js, and located in the toolkit/content/ directory);
2. the names of the globals that it defines (currently Preferences, Preference, and PrefPane; will eventually also include PrefWindow).

The first question is relatively unimportant, since this script will only be loaded in a few XUL documents (the about:preferences page and its subdialogs), and only via <script> tags.  It won't be Cu.import-ed from various parts of the codebase like Preferences.jsm.

So I would keep the script name preferences.js, since it replaces the similarly-named preferences.xml, it defines multiple "preferences"-related globals, and references to it (<script src="chrome://global/content/preferences.js"/>) are hard to confuse with Preferences.jsm references (Cu.import("resource://gre/modules/Preferences.jsm")).

The second question is somewhat more significant, since preferences.js defines a Preferences global (the equivalent of the <preferences> binding in preferences.xml), and Preferences.jsm also defines a Preferences global.

That isn't a problem for browser/components/preferences/ code, which doesn't import Preferences.jsm (although it does access it via Services.prefs).  But it could confuse engineers who use Preferences.jsm elsewhere, and it would become a problem if direct Preferences.jsm usage is ever introduced into browser/components/preferences/.

So it makes some sense to rename the Preferences global that preferences.js defines, and possibly also the Preference global.

How about calling them PrefSet and Pref?

Those names are unique within browser/components/preferences/, are sufficiently different from Preferences.jsm's global, and use the "Pref" abbreviation, which is consistent with the other global that the script defines (PrefPane) and another one it will eventually define (PrefWindow).

It's also consistent with the various <*set> tags that XUL provides to collect various kinds of XUL elements: <commandset>, <keyset>, <popupset>, etc.  (On the other hand, it's inconsistent with the CS concept of a set, since it indexes preferences by ID; it's more like a map.)

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Can we use the "Binding" as a suffix? PreferencesBinding, PreferenceBinding, etc? I think that's most clear to me and doesn't run in to naming conflicts.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8888096 [details] [diff] [review]
scriptify preferences XBL

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

r=me with some questions below...

From comment 6, "While doing the conversion, I noticed and fixed a couple of obvious performance issues, and the perf win may be due to those changes." Can you describe which performance improvements you made? It is hard to compare what was simply moved/reimplemented and what was changed.

::: browser/components/preferences/in-content-new/tests/browser.ini
@@ +59,5 @@
>  [browser_security-2.js]
>  [browser_siteData.js]
>  [browser_siteData2.js]
>  [browser_site_login_exceptions.js]
> +[browser_startup_perf.js]

We shouldn't check in this test, as it will never fail and will just make test runs take longer. We should instead have a Talos test that tracks the performance of opening preferences. I've filed bug 1384272 to track this.

::: browser/components/preferences/in-content-new/tests/privacypane_tests_perwindow.js
@@ +310,5 @@
>  
>  const gPrefCache = new Map();
>  
>  function cache_preferences(win) {
> +  let prefs = win.Preferences.getAll();

This is now getting all preferences, not just ones that are part of #privacyPreferences. Is that intended?

::: browser/themes/osx/preferences/preferences.css
@@ +22,5 @@
>    margin-bottom: 4px !important;
>  }
>  
> +.prefpane {
> +  padding: 12px 12px 0 12px;

padding-top: 12px;
padding-bottom: 12px;
padding-inline-start: 0;
padding-inline-end: 12px;

@@ +25,5 @@
> +.prefpane {
> +  padding: 12px 12px 0 12px;
> +}
> +
> +prefpane .groupbox-body, .prefpane .groupbox-body {

Please put each selector on its own line.

@@ +30,5 @@
>    -moz-appearance: none;
>    padding: 8px 4px 4px 4px;
>  }
>  
> +prefpane .groupbox-title, .prefpane .groupbox-title {

Ditto.

::: toolkit/content/preferences.js
@@ +1,1 @@
> +/* - This Source Code Form is subject to the terms of the Mozilla Public

Please rename this file to preferencesBindings.js

@@ +250,5 @@
> +    // This "change" event handler tracks changes made to preferences by
> +    // sources other than the user in this window.
> +    var elements = document.getElementsByAttribute("preference", this.id);
> +    for (var i = 0; i < elements.length; ++i)
> +      this.setElementValue(elements[i]);

We still will need to find a way to not do this anymore. It's n^2 for each item that gets added. Since we have addAll(), can we only run this at the end of the addAll?

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Please hold off on landing this for 56, though I'm not sure if you were planning that since it is so close to the merge anyways.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Do we still need this given bug 1375870 has landed?

Comment 21

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Please hold off on landing this for 56, though I'm not sure if you were
> planning that since it is so close to the merge anyways.

I wasn't planning to do so!  I'm working through your review comments and will post a new patch soon, but I won't target this for Firefox 56.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #20)
> Do we still need this given bug 1375870 has landed?

We presumably don't need it for perf, given the fix for bug 1375870, although we still want it for incremental replacement of XBL in Firefox.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Comment on attachment 8888096 [details] [diff] [review]
> scriptify preferences XBL
> 
> Review of attachment 8888096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with some questions below...
> 
> From comment 6, "While doing the conversion, I noticed and fixed a couple of
> obvious performance issues, and the perf win may be due to those changes."
> Can you describe which performance improvements you made? It is hard to
> compare what was simply moved/reimplemented and what was changed.

Yes, the primary optimization is another form of the fix for bug 1382170: the implementation no longer iterates preferences for every added preference.

A secondary, related optimization is that the Preference constructor no longer updates elements for all preferences when a pref is dynamically added after page load.  This too was fixed in the XBL implementation over in bug 1382170.

Note a behavior change in this patch: the XBL implementation delays calling updateElements() on any prefs until all prefs are added, while this implementation calls it after adding each pref.

@manishearth suggests in https://bugzilla.mozilla.org/show_bug.cgi?id=1375870#c11 that the delay is necessary, although it's unclear what breaks without it, since this implementation passes tests.

If we do need to re-introduce the delay, however, it's trivial to do, and doing it wouldn't affect the perf gain.  (In fact, an earlier version of the patch included the delay.  I removed it in a refactoring step after noticing that tests pass without it.)


> ::: browser/components/preferences/in-content-new/tests/browser.ini
> @@ +59,5 @@
> >  [browser_security-2.js]
> >  [browser_siteData.js]
> >  [browser_siteData2.js]
> >  [browser_site_login_exceptions.js]
> > +[browser_startup_perf.js]
> 
> We shouldn't check in this test, as it will never fail and will just make
> test runs take longer. We should instead have a Talos test that tracks the
> performance of opening preferences. I've filed bug 1384272 to track this.

Indeed.  I've removed it from this patch.


> :::
> browser/components/preferences/in-content-new/tests/
> privacypane_tests_perwindow.js
> @@ +310,5 @@
> >  
> >  const gPrefCache = new Map();
> >  
> >  function cache_preferences(win) {
> > +  let prefs = win.Preferences.getAll();
> 
> This is now getting all preferences, not just ones that are part of
> #privacyPreferences. Is that intended?

When I researched consumers of the old implementation, this is the only code I found that explicitly retrieves preferences by group (i.e. by <preferences> element), so I concluded that the implementation doesn't actually need to group prefs, provided this test works equivalently when it saves/restores all prefs instead of that specific group, which appears to be the case.

(It's also more robust against future changes that trigger changes in non-privacy prefs when privacy prefs change.)


> ::: browser/themes/osx/preferences/preferences.css
> @@ +22,5 @@
> >    margin-bottom: 4px !important;
> >  }
> >  
> > +.prefpane {
> > +  padding: 12px 12px 0 12px;
> 
> padding-top: 12px;
> padding-bottom: 12px;
> padding-inline-start: 0;
> padding-inline-end: 12px;

Ok, I've made this change.


> @@ +25,5 @@
> > +.prefpane {
> > +  padding: 12px 12px 0 12px;
> > +}
> > +
> > +prefpane .groupbox-body, .prefpane .groupbox-body {
> 
> Please put each selector on its own line.
> 
> @@ +30,5 @@
> >    -moz-appearance: none;
> >    padding: 8px 4px 4px 4px;
> >  }
> >  
> > +prefpane .groupbox-title, .prefpane .groupbox-title {
> 
> Ditto.

Ok, I've made these changes too.


> ::: toolkit/content/preferences.js
> @@ +1,1 @@
> > +/* - This Source Code Form is subject to the terms of the Mozilla Public
> 
> Please rename this file to preferencesBindings.js

I've made this change, but are you sure you want to conserve the term "binding" from the XBL implementation?  There isn't a binding (in the XBL sense of that word, anyway) in the new implementation.


> @@ +250,5 @@
> > +    // This "change" event handler tracks changes made to preferences by
> > +    // sources other than the user in this window.
> > +    var elements = document.getElementsByAttribute("preference", this.id);
> > +    for (var i = 0; i < elements.length; ++i)
> > +      this.setElementValue(elements[i]);
> 
> We still will need to find a way to not do this anymore. It's n^2 for each
> item that gets added. Since we have addAll(), can we only run this at the
> end of the addAll?

This only runs once per added preference, and each call only sets the values of the elements for that preference, so it should be O(N), just like the XBL implementation (with the fix for bug 1382170).

We could delay running this until the end of addAll, similar to the way the XBL implementation delays running it until this.preferences._constructedChildrenCount == this.preferences._preferenceChildren.length.  And we might want to do that to match the behavior of the XBL implementation, as discussed above (although it isn't clear that it's necessary).  But it wouldn't reduce the amount of work we do.

However, a potential optimization here (which is perhaps what you were suggesting?) would be to avoid calling updateElements entirely on page load and instead iterate over and update elements with "preference" attributes.  I'll try that and see if it makes a difference.

Waiting to request re-review pending that investigation.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

(In reply to Myk Melez [:myk] [@mykmelez] from comment #22)
> Note a behavior change in this patch: the XBL implementation delays calling
> updateElements() on any prefs until all prefs are added, while this
> implementation calls it after adding each pref.
> 
> @manishearth suggests in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1375870#c11 that the delay is
> necessary, although it's unclear what breaks without it, since this
> implementation passes tests.
> 
> If we do need to re-introduce the delay, however, it's trivial to do, and
> doing it wouldn't affect the perf gain.

I spoke too soon, as I've identified the code that depends on that delay (it's in subdialogs).  So I've reintroduced the delay in this patch.

I'm still waiting to request re-review pending investigation of that additional updateElements perf fix.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

This patch is rebased against the tip of central.

I've started to convert the subdialog implementations as well, and as that work progresses, I've grown more concerned about the consequences of landing this patch without those conversions, which would leave the preferences interfaces in an intermediate state, in which some of them are converted while others still use the old XBL bindings.

So instead of landing this patch I'm going to finish converting the subdialogs and then re-request review on the complete conversion.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

This patch is functionally complete and removes preferences.xml completely, but there are still some todos to either do or spin off into followup bugs, and I'm going to do that first before requesting review.

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

And here's the complete patch to replace preferences.xml with the JavaScript implementation in preferencesBindings.js.

mossop: before I request review, would it make sense to do a lightweight design review of the architecture in preferencesBindings.js?  That might help to ensure that we're employing architectural best practices, not only in that file but in any others that get modeled after it as we convert additional bindings.

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 8933570 [details] [diff] [review]
scriptify preferences XBL

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

::: toolkit/content/preferencesBindings.js
@@ +3,5 @@
> +   - You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/EventEmitter.jsm");

Please declare:

const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

@@ +133,5 @@
> +    return temp && temp.nodeType == Node.ELEMENT_NODE ?
> +           temp : aStartElement;
> +  },
> +
> +  get DeferredTask() {

This could be created with XPCOMUtils.defineLazyModuleGetter, but, why is this a property on Preferences rather than just a top-level?

@@ +257,5 @@
> +window.addEventListener("DOMContentLoaded",
> +  Preferences.onDOMContentLoaded.bind(Preferences), { once: true, capture: true });
> +window.addEventListener("input", Preferences.onInput.bind(Preferences));
> +window.addEventListener("select", Preferences.onSelect.bind(Preferences));
> +window.addEventListener("unload", Preferences.onUnload.bind(Preferences), { once: true });

I'd like to see these registrations moved into an init method on Preferences.

Comment 28

[Dave Townsend [:mossop]]

Architecturally I think this looks sound

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

(In reply to Dave Townsend [:mossop] from comment #27)

> const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Making only this change will trigger a "SyntaxError: redeclaration of const" because preferencesBindings.js is loaded in contexts that have already defined those constants.

But we can work around that by employing the module pattern to enclose the declarations in their own scope.  And that has some other benefits (like hiding private state).  So I've done that in this version of the patch.


> @@ +133,5 @@
> > +    return temp && temp.nodeType == Node.ELEMENT_NODE ?
> > +           temp : aStartElement;
> > +  },
> > +
> > +  get DeferredTask() {
> 
> This could be created with XPCOMUtils.defineLazyModuleGetter, but, why is
> this a property on Preferences rather than just a top-level?

Good catch!  This part of the implementation is a straight port from preferences.xml, where DeferredTask is a property of the <prefpane> binding: <https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/toolkit/content/widgets/preferences.xml#1265-1277>.

It should indeed be updated, just as I've updated other parts of preferences.xml that I initially ported as-is.  I've done so in this version of the patch, replacing it with a top-level call to XPCOMUtils.defineLazyModuleGetter.


> @@ +257,5 @@
> > +window.addEventListener("DOMContentLoaded",
> > +  Preferences.onDOMContentLoaded.bind(Preferences), { once: true, capture: true });
> > +window.addEventListener("input", Preferences.onInput.bind(Preferences));
> > +window.addEventListener("select", Preferences.onSelect.bind(Preferences));
> > +window.addEventListener("unload", Preferences.onUnload.bind(Preferences), { once: true });
> 
> I'd like to see these registrations moved into an init method on Preferences.

Preferences is a singleton that should only be initialized on script load, and initialization should be internal to the script (not invokable by external consumers).  Putting it here instead of within an "init" method enforces both of those constraints.


Jared: this is ready for your re-review.  It's a combination of the patch you previously reviewed, with some updates, along with other changes to replace the <prefwindow>, <prefpane>, and <panebutton> bindings, enabling us to remove preferences.xml entirely.

Along the way, I removed some cruft (f.e. all the prefpane-switching code from back when prefwindows had multiple prefpanes) and refactored some code to use modern best practices (although some archaisms may have slipped through, like the custom DeferredTask lazy-loader that mossop noticed).

Comment 30

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

(This version of the patch adds one line to fix a linting issue.)

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Zibi: this try run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0f4c92) is failing the l10n jobs on "make repackage-zip" because "Error: Missing file: ../../dist/xpi-stage/locale-[LANGCODE]/browser/chrome/[LANGCODE]/locale/browser/preferences/donottrack.dtd".

I removed that file, which was vestigial (used only by donottrack.xul, which I also removed, because it was vestigial, never used) along with its jar.mn entry.  Is there something else I need to do to mark it removed so the l10n repackager doesn't fail on it?

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

Fix for trivial issue found in tryserver run.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7162bf52e58a3da35cafa81ccf85da60396fbbf5

Comment 33

[Zibi Braniecki [:zbraniecki][:gandalf]]

Pike - is there any l10n-specific clobber one has to do? It seems to me that :myk's patch is doing the right thing and I'd expect a clobber to work.

Comment 34

[Axel Hecht [:Pike]]

That's an expected bustage of the L10n job, which is just a repack of the latest nightly with the current tree. If you change the list of files to build, that BURNS like chipotle.

I triggered the other kind of l10n jobs that actually repack a build on that revision, which should pass.

Comment 35

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #29)

> However, a potential optimization here (which is perhaps what you were
> suggesting?) would be to avoid calling updateElements entirely on page load
> and instead iterate over and update elements with "preference" attributes. 
> I'll try that and see if it makes a difference.

Note: I did end up making this change, although it doesn't seem to have impacted perf.

Comment 36

[Axel Hecht [:Pike]]

PS: the l10n try builds I triggered are green, so that part's OK

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8935506 [details] [diff] [review]
scriptify preferences XBL

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

r- due to the errors listed below. Sorry for the delay in reviewing. Next version can you use MozReview and push this (unchanged) version there first so I can do easier interdiffs?

With your patch applied on m-c tip (5572465c08a9):

#1 I get the following error in the Browser Console when loading "about:preferences#privacy":
TypeError: pref is null  preferencesBindings.js:104:9
  onDOMContentLoaded chrome://global/content/preferencesBindings.js:104:9
  onDOMContentLoaded self-hosted:948:17

A couple issues here,
1a, we should replace `document.getElementsByAttribute()` with `document.querySelectorAll()` and use an attribute selector, since `getElementsByAttribute` is XUL-only and non-standard.
1b, this code is broken because the <preference/> elements that this tries to get a reference to don't exist anymore

#2 When clicking on "Clear your recent history", nothing happens and I see this in the Browser Console:
TypeError: ts is null  privacy.js:856:9
  clearPrivateDataNow chrome://browser/content/preferences/in-content/privacy.js:856:9
  init/< chrome://browser/content/preferences/in-content/privacy.js:200:9
  <anonymous> self-hosted:948:17

This is coming from `var ts = document.getElementById("privacy.sanitize.timeSpan");`, where the element with that ID doesn't exist anymore (it was a <preference/> element). 

#3, When clicking "Show tab previews in the Windows taskbar" and "Show a touch keyboard when necessary" (both Windows-only prefs), I see this in the Browser Console:
TypeError: preference is null preferencesBindings.js:168:15
  userChangedValue chrome://global/content/preferencesBindings.js:168:15
  onCommand chrome://global/content/preferencesBindings.js:189:7
  onCommand self-hosted:948:17

#4, (possibly a dupe of #1), When creating a new Container I see the following in the Browser Console:
TypeError: pref is null preferencesBindings.js:104:9
  onDOMContentLoaded chrome://global/content/preferencesBindings.js:104:9
  onDOMContentLoaded self-hosted:948:17

#5, When opening the Languages subdialog I see the following error in the Browser Console:
ReferenceError: gLanguagesDialog is not defined languages.xul:1:1
  onresize chrome://browser/content/preferences/languages.xul:1:1

#6, When choosing "Use custom settings for History" I get the following error:
TypeError: document.getElementById(...) is null privacy.js:580:9
  updatePrivacyMicroControls chrome://browser/content/preferences/in-content/privacy.js:580:9
  init/< chrome://browser/content/preferences/in-content/privacy.js:195:7
  <anonymous> self-hosted:948:17

::: browser/base/content/sanitizeDialog.js
@@ +150,5 @@
> +   * Return the boolean prefs that enable/disable clearing of various kinds
> +   * of history.  The only pref this excludes is privacy.sanitize.timeSpan.
> +   */
> +  _getItemPrefs() {
> +    return Preferences.getAll().filter(p => p.id !== "privacy.sanitize.timeSpan");

Doesn't this also include other prefs too (depending on the order that the preferences are accessed)?

@@ +166,4 @@
>      var i = 0;
> +    var prefs = this._getItemPrefs();
> +    while (!found && i < prefs.length) {
> +      var preference = prefs[i];

This loop can be replaced by:
var found = this._getItemPrefs().some(pref => !!pref.value && !pref.disabled);

::: toolkit/content/preferencesBindings.js
@@ +92,5 @@
> +
> +    _DOMContentLoaded: false,
> +
> +    onDOMContentLoaded() {
> +      this._DOMContentLoaded = true;

Instead of adding a DOMContentLoaded event listener that updates all of the preferences, could we create a promise that resolves when the DOMContentLoaded event is fired and then the prefs that are waiting for DOMContentLoaded can use the `then` on the promise to update their element?

Something like:
```js
const Preferences = {
  ...
  _domContentLoadedPromise: new Promise(resolve => {
    window.addEventListener("domcontentloaded", resolve, { capture: true, once: true });
  },
  ...
}
```
Then inside of Preferences.add() we can just call `this._domContentLoadedPromise.then(() => { pref.updateElements(); });`, and we may be able to remove the "Update all elements" block below.

@@ +122,5 @@
> +        this._finalizeDeferredElements();
> +      }
> +
> +      var preferences = Preferences.getAll();
> +      for (var i = 0; i < preferences.length; ++i) {

nit, for..of since `i` is not used otherwise.

@@ +164,5 @@
> +      let element = this.getPreferenceElement(aElement);
> +      if (element.hasAttribute("preference")) {
> +        if (element.getAttribute("delayprefsave") != "true") {
> +          var preference = Preferences.get(element.getAttribute("preference"));
> +          var prefVal = preference.getElementValue(element);

nit, s/var/let/

@@ +210,5 @@
> +
> +    _fireEvent(aEventName, aTarget) {
> +      // Panel loaded, synthesize a load event.
> +      try {
> +        var event = document.createEvent("Events");

s/var/let/

@@ +216,5 @@
> +        var cancel = !aTarget.dispatchEvent(event);
> +        if (aTarget.hasAttribute("on" + aEventName)) {
> +          var fn = new Function("event", aTarget.getAttribute("on" + aEventName));
> +          var rv = fn.call(aTarget, event);
> +          if (rv == false)

if (!rv)

@@ +244,5 @@
> +    },
> +  };
> +
> +  Services.prefs.addObserver("", Preferences);
> +  window.addEventListener("change", Preferences.onChange.bind(Preferences));

I prefer that we don't use `bind` here because it creates a new function that makes it error prone to leak when calling removeEventListener (if new code gets added to do this at some point). Instead if we use `window.addEventListener("change", Preferences)`, we can then use `handleEvent` and route the event-type specific handling elsewhere without creating new functions and still having `this` refer to `Preferences`.

Can you make that change here and below?

@@ +298,5 @@
> +    }
> +
> +    _reportUnknownType() {
> +      var msg = "Preference with id='" + this.id + "' and name='" +
> +                this.name + "' has unknown type '" + this.type + "'.";

Please use a template string here.

@@ +585,5 @@
> +    }
> +  }
> +
> +  return Preferences;
> +}.bind({})());

Can you explain why we need to use bind() here? I see that it's a self-executing function to create a closure but otherwise I don't think I've seen this convention in our codebase before outside of the JS tests (https://searchfox.org/mozilla-central/search?q=.bind(%7B%7D)&path=).

Comment 38

[Myk Melez [:myk] [@mykmelez]]

Attachment: Bug 1379338 - scriptify preferences XBL;

Converts the XBL bindings for Preferences to either JS singletons/classes
or variants of more general bindings, as appropriate, so that we can remove
the XBL bindings from the codebase as part of our XBL removal project.

Review commit: https://reviewboard.mozilla.org/r/208790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208790/

Comment 39

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8938074 [details]
Bug 1379338 - scriptify preferences XBL;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208790/diff/1-2/

Comment 40

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> Comment on attachment 8935506 [details] [diff] [review]
> scriptify preferences XBL
> 
> Review of attachment 8935506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- due to the errors listed below. Sorry for the delay in reviewing. Next
> version can you use MozReview and push this (unchanged) version there first
> so I can do easier interdiffs?

Yes, I've pushed the old version with only the minimum changes necessary for it to apply cleanly to the tip of central (which only required resolving a single merge conflict).

I'll also reference commits to the GitHub branch where I'm doing this work (https://github.com/mykmelez/gecko/tree/webify-pref-subdialogs), so you can look at the changes individually, in case that helps your re-review.


> With your patch applied on m-c tip (5572465c08a9):
> 
> #1 I get the following error in the Browser Console when loading
> "about:preferences#privacy":
> TypeError: pref is null  preferencesBindings.js:104:9
>   onDOMContentLoaded chrome://global/content/preferencesBindings.js:104:9
>   onDOMContentLoaded self-hosted:948:17
> 
> A couple issues here,
> 1a, we should replace `document.getElementsByAttribute()` with
> `document.querySelectorAll()` and use an attribute selector, since
> `getElementsByAttribute` is XUL-only and non-standard.

Good point, as that'll matter in the future, and it's worth making it better today.

Note: I wrote a micro-benchmark to test this change, and document.querySelectorAll("[preference]") is much slower than document.getElementsByAttribute("preference", "*"), at about 24ms vs. 1ms for 1000 calls.  But both of them still take much less than a millisecond per call, however; and since we don't do this that often*, the difference seems irrelevant.

(* I instrumented the code to see how often we do this in tests, and most of them result in fewer than 10 calls, although several get into double digits, and a few into three digits, with a max of 206 calls.)

I made this change in https://github.com/mykmelez/gecko/commit/893c7b9034c2, where I also changed the other three calls to document.getElementsByAttribute, refactoring them all into a getElementsByAttribute local function that uses querySelectorAll internally (to reduce churn in the event that we decide to change the way we retrieve elements in the future).


> 1b, this code is broken because the <preference/> elements that this tries
> to get a reference to don't exist anymore

This code actually tries to get elements with preference attributes, not <preference/> elements, and those still exist.  I'm not sure what the issue is here, as I haven't been able to reproduce this on any platform (Windows, Mac, Linux).  So I've added some code in https://github.com/mykmelez/gecko/commit/3f8c1a670848 that logs an error to the console if there isn't a Preference for an ID in a preference attribute.

If you can reproduce, would you let me know which ID is reported to the console and on which platform you're testing?


> #2 When clicking on "Clear your recent history", nothing happens and I see
> this in the Browser Console:
> TypeError: ts is null  privacy.js:856:9
>   clearPrivateDataNow
> chrome://browser/content/preferences/in-content/privacy.js:856:9
>   init/< chrome://browser/content/preferences/in-content/privacy.js:200:9
>   <anonymous> self-hosted:948:17
>
> This is coming from `var ts =
> document.getElementById("privacy.sanitize.timeSpan");`, where the element
> with that ID doesn't exist anymore (it was a <preference/> element). 

Ah, right, I missed that one while converting all these calls, and I guess the tests don't cover it either.

Fixed in https://github.com/mykmelez/gecko/commit/31feebabd1cc.


> #3, When clicking "Show tab previews in the Windows taskbar" and "Show a
> touch keyboard when necessary" (both Windows-only prefs), I see this in the
> Browser Console:
> TypeError: preference is null preferencesBindings.js:168:15
>   userChangedValue chrome://global/content/preferencesBindings.js:168:15
>   onCommand chrome://global/content/preferencesBindings.js:189:7
>   onCommand self-hosted:948:17

This is due to an error in the conversion from preprocessor directives to AppConstants conditionals.  I converted `#ifdef XP_WIN` to `if (AppConstants.XP_WIN)` when it should be `if (AppConstants.platform === "win")`.

Fixed in https://github.com/mykmelez/gecko/commit/9546e68730bf.


> #4, (possibly a dupe of #1), When creating a new Container I see the
> following in the Browser Console:
> TypeError: pref is null preferencesBindings.js:104:9
>   onDOMContentLoaded chrome://global/content/preferencesBindings.js:104:9
>   onDOMContentLoaded self-hosted:948:17

I can't reproduce this one either, but the code I added to log missing Preference objects for #1 should log for this one too.  If you can reproduce, can you let me know which ID is reported?


> #5, When opening the Languages subdialog I see the following error in the
> Browser Console:
> ReferenceError: gLanguagesDialog is not defined languages.xul:1:1
>   onresize chrome://browser/content/preferences/languages.xul:1:1

I haven't been able to reproduce this one on any platform either.  But I had moved the location of the <script> tag that loads languages.js, which defines gLanguagesDialog.  I don't see how that could have caused this problem, but I also don't think it needed to be moved, so I've moved it back to its original location in https://github.com/mykmelez/gecko/commit/857cbcba35b3.  Can you still reproduce the problem?


> #6, When choosing "Use custom settings for History" I get the following
> error:
> TypeError: document.getElementById(...) is null privacy.js:580:9
>   updatePrivacyMicroControls
> chrome://browser/content/preferences/in-content/privacy.js:580:9
>   init/< chrome://browser/content/preferences/in-content/privacy.js:195:7
>   <anonymous> self-hosted:948:17

This is caused by the change in bug 1423564, which landed after I created the patch.

Fixed in https://github.com/mykmelez/gecko/commit/50574926bd92.


> ::: browser/base/content/sanitizeDialog.js
> @@ +150,5 @@
> > +   * Return the boolean prefs that enable/disable clearing of various kinds
> > +   * of history.  The only pref this excludes is privacy.sanitize.timeSpan.
> > +   */
> > +  _getItemPrefs() {
> > +    return Preferences.getAll().filter(p => p.id !== "privacy.sanitize.timeSpan");
> 
> Doesn't this also include other prefs too (depending on the order that the
> preferences are accessed)?

It only includes the Preference objects that the sanitize dialog creates, which are all "item" prefs, except for privacy.sanitize.timeSpan.  Here's the complete list of IDs of Preference objects that the sanitize dialog creates:

  privacy.cpd.history
  privacy.cpd.formdata
  privacy.cpd.downloads
  privacy.cpd.cookies
  privacy.cpd.cache
  privacy.cpd.sessions
  privacy.cpd.offlineApps
  privacy.cpd.siteSettings
  privacy.sanitize.timeSpan


> @@ +166,4 @@
> >      var i = 0;
> > +    var prefs = this._getItemPrefs();
> > +    while (!found && i < prefs.length) {
> > +      var preference = prefs[i];
> 
> This loop can be replaced by:
> var found = this._getItemPrefs().some(pref => !!pref.value &&
> !pref.disabled);

Indeed, much better.  Replaced in https://github.com/mykmelez/gecko/commit/a252a4650f65.


> ::: toolkit/content/preferencesBindings.js
> @@ +92,5 @@
> > +
> > +    _DOMContentLoaded: false,
> > +
> > +    onDOMContentLoaded() {
> > +      this._DOMContentLoaded = true;
> 
> Instead of adding a DOMContentLoaded event listener that updates all of the
> preferences, could we create a promise that resolves when the
> DOMContentLoaded event is fired and then the prefs that are waiting for
> DOMContentLoaded can use the `then` on the promise to update their element?
> 
> Something like:
> ```js
> const Preferences = {
>   ...
>   _domContentLoadedPromise: new Promise(resolve => {
>     window.addEventListener("domcontentloaded", resolve, { capture: true,
> once: true });
>   },
>   ...
> }
> ```
> Then inside of Preferences.add() we can just call
> `this._domContentLoadedPromise.then(() => { pref.updateElements(); });`, and
> we may be able to remove the "Update all elements" block below.

Indeed, this works and is more elegant/simpler.

I would be concerned about the cost of running getElementsByAttribute for each Preference (especially now that we implement it using the slower querySelectorAll).  I designed the onDOMContentLoaded method to avoid doing that.  However, I've profiled page load, and getElementsByAttribute barely registers with this change.  So I'm not concerned.

I did, however, leave onDOMContentLoaded in the codebase, modifying it so that it no longer updates elements but does still log an error if it can't find a Preference for an element.  That way we can still see if you reproduce that error you experienced earlier.

Fixed in https://github.com/mykmelez/gecko/commit/8344eec971f3.


> @@ +122,5 @@
> > +        this._finalizeDeferredElements();
> > +      }
> > +
> > +      var preferences = Preferences.getAll();
> > +      for (var i = 0; i < preferences.length; ++i) {
> 
> nit, for..of since `i` is not used otherwise.

Indeed, fixed in https://github.com/mykmelez/gecko/commit/ea9afc148606.


> @@ +164,5 @@
> > +      let element = this.getPreferenceElement(aElement);
> > +      if (element.hasAttribute("preference")) {
> > +        if (element.getAttribute("delayprefsave") != "true") {
> > +          var preference = Preferences.get(element.getAttribute("preference"));
> > +          var prefVal = preference.getElementValue(element);
> 
> nit, s/var/let/
> 
> @@ +210,5 @@
> > +
> > +    _fireEvent(aEventName, aTarget) {
> > +      // Panel loaded, synthesize a load event.
> > +      try {
> > +        var event = document.createEvent("Events");
> 
> s/var/let/

These were both inherited from the old codebase.  In https://github.com/mykmelez/gecko/commit/8f6204c80ddd, I changed them and all other instances of "var" to either "const" (if they don't change post-declaration) or "let" (if they do).  I also switched a couple more for loops to for..of in the process.


> @@ +216,5 @@
> > +        var cancel = !aTarget.dispatchEvent(event);
> > +        if (aTarget.hasAttribute("on" + aEventName)) {
> > +          var fn = new Function("event", aTarget.getAttribute("on" + aEventName));
> > +          var rv = fn.call(aTarget, event);
> > +          if (rv == false)
> 
> if (!rv)
> 
> @@ +244,5 @@
> > +    },
> > +  };
> > +
> > +  Services.prefs.addObserver("", Preferences);
> > +  window.addEventListener("change", Preferences.onChange.bind(Preferences));
> 
> I prefer that we don't use `bind` here because it creates a new function
> that makes it error prone to leak when calling removeEventListener (if new
> code gets added to do this at some point). Instead if we use
> `window.addEventListener("change", Preferences)`, we can then use
> `handleEvent` and route the event-type specific handling elsewhere without
> creating new functions and still having `this` refer to `Preferences`.
> 
> Can you make that change here and below?

Done in https://github.com/mykmelez/gecko/commit/cc06aaf752c4, where I also removed the binding from Preferences.onDOMContentLoaded.


> @@ +298,5 @@
> > +    }
> > +
> > +    _reportUnknownType() {
> > +      var msg = "Preference with id='" + this.id + "' and name='" +
> > +                this.name + "' has unknown type '" + this.type + "'.";
> 
> Please use a template string here.

Done in https://github.com/mykmelez/gecko/commit/ccca80e3dfbb.


> @@ +585,5 @@
> > +    }
> > +  }
> > +
> > +  return Preferences;
> > +}.bind({})());
> 
> Can you explain why we need to use bind() here? I see that it's a
> self-executing function to create a closure but otherwise I don't think I've
> seen this convention in our codebase before outside of the JS tests
> (https://searchfox.org/mozilla-central/search?q=.bind(%7B%7D)&path=).

We want to lazily attach DeferredTask to the closure's scope by passing the *this* object to XPCOMUtils.defineLazyModuleGetter:

  XPCOMUtils.defineLazyModuleGetter(this, "DeferredTask",
                                    "resource://gre/modules/DeferredTask.jsm");

But *this* would be undefined by default within the closure, because the script is in strict mode.  So doing this would throw an error.  Binding the closure gives defineLazyModuleGetter a *this* object to attach DeferredTask to.


For completeness (i.e. to reference all commits I made on top of the previous version of the patch): after making all these changes, I linted them and fixed the linter issues in https://github.com/mykmelez/gecko/commit/ff23baadee6c.


Requesting needinfo per jaws's Bugzilla message.

Comment 41

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Note: I wrote a micro-benchmark to test this change, and document.querySelectorAll("[preference]") is much slower than
> document.getElementsByAttribute("preference", "*"), at about 24ms vs. 1ms for 1000 calls.  But both of them still take much less
> than a millisecond per call, however; and since we don't do this that often*, the difference seems irrelevant.
> (* I instrumented the code to see how often we do this in tests, and most of them result in fewer than 10 calls, although several
> get into double digits, and a few into three digits, with a max of 206 calls.)

Could you file a bug for this? Fluent is heavily using document.querySelectorAll("[data-l10n-id]") and I think it's generally worth it putting this case on a fast path (which seems to be available based on the `getElementsByAttribute` performance.)

Comment 42

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8938074 [details]
Bug 1379338 - scriptify preferences XBL;

https://reviewboard.mozilla.org/r/208790/#review215070

Thank you for the detailed mini-interdiffs. This is much easier to review.

I'll note the following (all seen on Windows 10 64-bit with a clean profile using your commit based off of the same parent changeset):

#1 When I open the Preferences, I see the following error in my Browser Console:
TypeError: aField is null  FormLikeFactory.jsm:109:1
This error is reproducible any time I put focus in a text field within the preferences (Search box or Home Page box)

I don't know why that would start failing now, I didn't see that in my previous review and I don't see anything here that gives reason for it. It is worth looking in to before landing though.

There were a few issues that I saw before that you could not reproduce. I am no longer able to reproduce these issues with your latest patch. Maybe they were fixed by other commits or indirectly though the other changes you have made for this revision. Either way, this looks good to go. Thanks!

Comment 43

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #42)

> #1 When I open the Preferences, I see the following error in my Browser
> Console:
> TypeError: aField is null  FormLikeFactory.jsm:109:1
> This error is reproducible any time I put focus in a text field within the
> preferences (Search box or Home Page box)
> 
> I don't know why that would start failing now, I didn't see that in my
> previous review and I don't see anything here that gives reason for it. It
> is worth looking in to before landing though.

I saw this on Mac too.  It was bug 1426561, which has since been fixed.


> There were a few issues that I saw before that you could not reproduce. I am
> no longer able to reproduce these issues with your latest patch. Maybe they
> were fixed by other commits or indirectly though the other changes you have
> made for this revision. Either way, this looks good to go. Thanks!

Woot!  Here's an updated version of the patch that resolves trivial conflicts with a couple other patches that have landed on central.  This is the version I'll push.

Comment 44

[Myk Melez [:myk] [@mykmelez]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #41)

> Could you file a bug for this? Fluent is heavily using
> document.querySelectorAll("[data-l10n-id]") and I think it's generally worth
> it putting this case on a fast path (which seems to be available based on
> the `getElementsByAttribute` performance.)

I filed this as bug 1427926.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76ff10b9bff
scriptify preferences XBL; r=jaws

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/232c97d7c2ea
scriptify preferences XBL: remove unreferenced panebuttons. r=bustagge-fix on a CLOSED TREE

Comment 48

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing Marionette headless' test_anonymous_content.py TestAnonymousNodes.test_find_anonymous_children:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5cfa9fe889a926ad1bf7937a7d35d33ca6c4d1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a76ff10b9bff38ae4398845ad4bb14f425430b9f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log (so far on OS X 10.10 debug and Windows 7 pgo): https://treeherder.mozilla.org/logviewer.html#?job_id=154135111&repo=mozilla-inbound

17:06:54     INFO -  1515085614594	Marionette	TRACE	7 -> [0,20,"findElements",{"using":"anon","value":null}]
17:06:54     INFO -  1515085614596	Marionette	TRACE	7 <- [1,20,null,[{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"57a45589-adcb-4dbe-9157-27b3f829d5c5","ELEMENT":"57a45589-adcb-4dbe ... ement-9fc5-4b51-a3c8-01716eedeb04":"71a4c397-03d5-4dcc-a13b-208d3d1f3cd3","ELEMENT":"71a4c397-03d5-4dcc-a13b-208d3d1f3cd3"}]]
17:06:54     INFO -  1515085614597	Marionette	TRACE	7 -> [0,21,"getContext",{}]
17:06:54     INFO -  1515085614598	Marionette	TRACE	7 <- [1,21,null,{"value":"chrome"}]
17:06:54     INFO -  1515085614599	Marionette	TRACE	7 -> [0,22,"setContext",{"value":"chrome"}]
17:06:54     INFO -  1515085614599	Marionette	TRACE	7 <- [1,22,null,{}]
17:06:54     INFO -  1515085614600	Marionette	TRACE	7 -> [0,23,"takeScreenshot",{"highlights":null,"full":true,"hash":false,"id":null,"scroll":true}]
17:06:54     INFO -  1515085614602	Marionette	TRACE	7 <- [1,23,null,{"value":"iVBORw0KGgoAAAANSUhEUgAAAL0AAAByCAYAAAAGadBRAAAF50lEQVR4nO2cTU8abRiFn58DCT/Bqu2CdguNyST9B12xIUYk6QYTy6KW ... cXFxGPxz2/wZKfEo/Hsbi46ETXAbytHxEHpSfioPREHJSeiIPSE3FQeiIOSk/EQemJOCg9EQelJ+Kg9EQclJ6Ig9ITcfwHq1/ONuHwef4AAAAASUVORK5CYII="}]
17:06:54     INFO -  1515085614603	Marionette	TRACE	7 -> [0,24,"setContext",{"value":"chrome"}]
17:06:54     INFO -  1515085614603	Marionette	TRACE	7 <- [1,24,null,{}]
17:06:54     INFO -  1515085614605	Marionette	TRACE	7 -> [0,25,"getContext",{}]
17:06:54     INFO -  1515085614605	Marionette	TRACE	7 <- [1,25,null,{"value":"chrome"}]
17:06:54     INFO -  1515085614606	Marionette	TRACE	7 -> [0,26,"setContext",{"value":"content"}]
17:06:54     INFO -  1515085614606	Marionette	TRACE	7 <- [1,26,null,{}]
17:06:54     INFO -  1515085614607	Marionette	TRACE	7 -> [0,27,"getPageSource",{}]
17:06:54     INFO -  1515085614608	Marionette	TRACE	7 <- [1,27,{"error":"no such window","message":"Unable to locate window","stacktrace":"WebDriverError@chrome://marionette/content/ ... et@chrome://marionette/content/server.js:505:8\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:500:9\n"},null]
17:06:54     INFO -  1515085614609	Marionette	TRACE	7 -> [0,28,"setContext",{"value":"chrome"}]
17:06:54     INFO -  Failed to gather test failure debug: Unable to locate window
17:06:54     INFO -  stacktrace:
17:06:54     INFO -  	WebDriverError@chrome://marionette/content/error.js:172:5
17:06:54     INFO -  	NoSuchWindowError@chrome://marionette/content/error.js:414:5
17:06:54     INFO -  	assert.that/<@chrome://marionette/content/assert.js:410:13
17:06:54     INFO -  	assert.window@chrome://marionette/content/assert.js:143:10
17:06:54     INFO -  	GeckoDriver.prototype.getPageSource@chrome://marionette/content/driver.js:1135:15
17:06:54     INFO -  	Async*despatch@chrome://marionette/content/server.js:557:20
17:06:54     INFO -  	async*execute@chrome://marionette/content/server.js:531:11
17:06:54     INFO -  	async*onPacket/<@chrome://marionette/content/server.js:506:15
17:06:54     INFO -  	async*onPacket@chrome://marionette/content/server.js:505:8
17:06:54     INFO -  	_onJSONObjectReady/<@chrome://marionette/content/transport.js:500:9
17:06:54     INFO -  1515085614609	Marionette	TRACE	7 <- [1,28,null,{}]
17:06:54    ERROR -  TEST-UNEXPECTED-FAIL | testing\marionette\harness\marionette_harness\tests\unit\test_anonymous_content.py TestAnonymousNodes.test_find_anonymous_children | AssertionError: 2 != 3
17:06:54     INFO -  Traceback (most recent call last):
17:06:54     INFO -    File "Z:\task_1515085223\build\venv\lib\site-packages\marionette_harness\marionette_test\testcases.py", line 156, in run
17:06:54     INFO -      testMethod()
17:06:54     INFO -    File "Z:\task_1515085223\build\tests\marionette\tests\testing\marionette\harness\marionette_harness\tests\unit\test_anonymous_content.py", line 85, in test_find_anonymous_children
17:06:54     INFO -      self.assertEquals(2, len(self.marionette.find_elements(By.ANON, None)))
17:06:54     INFO -  TEST-INFO took 168ms

The image references from the jar.mn also still have to be deleted.

Comment 49

[Myk Melez [:myk] [@mykmelez]]

Attachment: scriptify preferences XBL

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #48)
> Backed out for failing Marionette headless' test_anonymous_content.py
> TestAnonymousNodes.test_find_anonymous_children:

This test counts the number of anonymous elements in dialog.xml, to which I've added an element (a xul:keyset).  So I updated the test to assert the new number of elements in https://github.com/mykmelez/gecko/commit/9dff6bdd8248b9a6c4a18640e84735ab6328fc98.


> The image references from the jar.mn also still have to be deleted.

I've done that in https://github.com/mykmelez/gecko/commit/426c4f39c13522a9d8efccf6ef4992cb503d42af.


Here's a tryserver run for those tests to confirm:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2b045a110fddd5e7536dc953f54a1b8e062505a

Comment 50

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #49)
> Here's a tryserver run for those tests to confirm:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c2b045a110fddd5e7536dc953f54a1b8e062505a

Confirmed.  Relanding.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0adedb70b788
scriptify preferences XBL; r=jaws

Comment 52

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/0adedb70b788