1392352 - Migrate the tabbrowser XBL binding to a JS class

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task, P2)

Description

[Brian Grinstead [:bgrins]]

The tabbrowser binding (in browser/base/content/tabbrowser.xml) has a lot of code, but it doesn't use a lot of DOM access on the <tabbrowser#content> node in browser.xul.

I'd like to document an investigation of migrating this code away from XBL and towards a JS class.

Comment 1

[Brian Grinstead [:bgrins]]

As a starting point, here's a script to generate a Custom Element class from a binding here: https://github.com/bgrins/xbl-analysis/blob/gh-pages/scripts/build-custom-elements.js, which leads to output like: https://github.com/bgrins/xbl-analysis/blob/61798dd1578ec0f468538a087b0d5713c635e9db/elements/generated/Tabbrowser.js.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

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

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

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

Comment 4

[Brian Grinstead [:bgrins]]

Posted some initial patches. A few notes:

* There are steps to build the file at the top of TabBrowser.js
* This is in a js file for now to make it easier to setup (so it can access window globals), but I assume perf would be better if this was in a JSM instead.
* The <browser> shrinks down to 25px x 25px after navigating from the start page. It's not clear to me why this is happening based on inspecting the XUL / CSS

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

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

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

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

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> * This is in a js file for now to make it easier to setup (so it can access
> window globals), but I assume perf would be better if this was in a JSM
> instead.

Mossop mentioned that it may end up that a <script> in browser.xul might be faster than a jsm in this case, because there's a lot of calls back and forth between the browser global and this object.

> * The <browser> shrinks down to 25px x 25px after navigating from the start
> page. It's not clear to me why this is happening based on inspecting the XUL
> / CSS

Fixed this issue in the latest version, this was due to a missing namespaceURI on the statuspanel sibling to the stack that contains the browser

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

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

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

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

Comment 10

[Brian Grinstead [:bgrins]]

Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcdaa1c79f742e73fdd9a49fb888bf1d64e55fee&group_state=expanded

Comment 11

[Brian Grinstead [:bgrins]]

Tests like browser/base/content/test/newtab/browser_newtab_1188015.js are failing due to an access of gBrowser.contentWindow, which in tests run through an nsIAddonInterposition for the <tabbrowser> element.  Specifically, this code detects the element: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/multiprocessShims.js#120, and this code replaces the implementation: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsParent.jsm#925.

Even when changing the condition in getObjectTag it still fails - the interposeCall function doesn't appear to be triggered for the new gBrowser object.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Tests like browser/base/content/test/newtab/browser_newtab_1188015.js are
> failing due to an access of gBrowser.contentWindow, which in tests run
> through an nsIAddonInterposition for the <tabbrowser> element. 
> Specifically, this code detects the element:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> addoncompat/multiprocessShims.js#120, and this code replaces the
> implementation:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> addoncompat/RemoteAddonsParent.jsm#925.
> 
> Even when changing the condition in getObjectTag it still fails - the
> interposeCall function doesn't appear to be triggered for the new gBrowser
> object.

Reading https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/idl/nsIAddonInterposition.idl#17-21, the interpose method is meant to be called any time an object from outside of the addon's compartment is accessed. Bill, do you have an idea why `gBrowser` as a <tabbrowser> would get interposed but `gBrowser` as a JS object wouldn't? And/or if there is a way to force an object to go through the interposition?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I suspect the problem is here:
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/js/xpconnect/wrappers/AddonWrapper.cpp#45-56
For better performance, we only interpose under certain conditions. I think you could try commenting out the |return true| there and see if things start to work.

Now that we don't have to worry about legacy add-ons, interposition will only be used for tests. So I think it would be find to remove this optimization.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, I'm really excited about this bug! Thanks for working on it.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/3-4/

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/3-4/

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Bill McCloskey (:billm) from comment #13)
> I suspect the problem is here:
> http://searchfox.org/mozilla-central/rev/
> 48ea452803907f2575d81021e8678634e8067fc2/js/xpconnect/wrappers/AddonWrapper.
> cpp#45-56
> For better performance, we only interpose under certain conditions. I think
> you could try commenting out the |return true| there and see if things start
> to work.
> 
> Now that we don't have to worry about legacy add-ons, interposition will
> only be used for tests. So I think it would be find to remove this
> optimization.

Thanks, that did the trick! Waiting to see what try has to say: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c29a4ea3228af50c639c994cf6897b6246bc584a&group_state=expanded

Comment 18

[Brian Grinstead [:bgrins]]

Some tests like browser_responsiveui_window_close.js are failing because the code is detecting the presence of a function on gBrowser before running [0] like:

    if (!this.mainWindow.gBrowser || !this.mainWindow.gBrowser.getNotificationBox) {
      return;
    }

This is handling a case where the XBL binding has been removed from the gBrowser element and takes the getNotificationBox function away with it, but when we call the destroy function on the class the getNotificationBox function is still there (and returns a notificationbox that doesn't have any bindings attached so we get errors like "nbox.getNotificationWithValue is not a function" [1]).  We'll have to see how many places rely on that behavior - it seems like we could instead set a `destroyed` attribute on gBrowser, or use some existing condition on the window to detect this case instead of relying on the XBL behavior.

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsivedesign/responsivedesign.jsm#755
[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c29a4ea3228af50c639c994cf6897b6246bc584a&group_state=expanded&selectedJob=125714170

Comment 19

[Brian Grinstead [:bgrins]]

If I try to null out gBrowser in onUnload there are some errors at shutdown, that are potentially other places dealing with a de-initialized gBrowser binding:

JavaScript error: resource:///modules/UpdateTopLevelContentWindowIDHelper.jsm, line 116: TypeError: aWindow.gBrowser is null
JavaScript error: resource:///modules/BrowserUsageTelemetry.jsm, line 604: TypeError: win.defaultView.gBrowser is null
JavaScript error: chrome://browser/content/browser.js, line 8329: TypeError: gBrowser is null

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 2 - Remove extra tabbrowser-tabbox binding

This is used to change one property on the tabbox, which can be done
instead by passing an option into the tabbox.

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

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 1 - Interpose all objects;

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

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/4-5/

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/4-5/

Comment 24

[Brian Grinstead [:bgrins]]

Another thing I found when looking into this: we have a binding called tabbrowser-tabbox [0] that extends tabbox and overwrites a single property ("tabs") so that it gets a different implementation than the default "tabs" property on tabbox [1]. This binding only applies to a single element - the child content inside of #tabbrowser [2].  Instead of declaring a new binding and applying it to that child, we could pass an attribute to the child to tell it about the tabbrowser and modify the "tabs" implementation at [1].

[0]: https://github.com/mozilla/gecko-dev/blob/89e125b817c5d493babbc58ea526be970bd3748e/browser/base/content/tabbrowser.xml#L5991-L5997
[1]: https://github.com/mozilla/gecko-dev/blob/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/content/widgets/tabbox.xml#L58-L66
[2]: https://github.com/mozilla/gecko-dev/blob/89e125b817c5d493babbc58ea526be970bd3748e/browser/base/content/tabbrowser.css#L5-L7

Comment 25

[Brian Grinstead [:bgrins]]

Something else I noticed that may be related to the debug leaks is that window.onunload doesn't get called in browser.xul at least in certain cases (and the TabBrowser.disconnectedCallback function is called from there). Presumably the XBL binding destructor is called no matter what when the DOM node gets removed.

For instance, if I do `./mach mochitest browser/base/content/test/sidebar/browser_sidebar_move.js --setpref browser.dom.window.dump.enabled=true` and add a dump on the first line of `gBrowserInit.onUnload`, it never gets called.  This is true both with and without patches from this bug applied.

However, the gBrowserInit.onUnload function does appear to be called in non-test scenarios like:

  * Start process
  * Open new window
  * Close the new window
  * gBrowserInit.onUnload gets called for the new window

and

  * Start process
  * Quit process
  * gBrowserInit.onUnload gets called for the initial window

Bill, are you aware of a case where window.onunload is expected to not fire and the test somehow hits it, or is this a bug?

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/5-6/

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

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

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/5-6/

Comment 30

[Brian Grinstead [:bgrins]]

Added a workaround for the debug leaks by keeping the tabbrowser binding around and using it only for the <constructor> and <destructor>. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a1ee885598058dece12c87466fc326c647c883&group_state=expanded

Comment 31

[Dão Gottwald [:dao]]

FWIW, this was already on my mental longer-term to-do list. We'll definitely want to do this, one way or another.

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/6-7/

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

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

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/6-7/

Comment 35

[Brian Grinstead [:bgrins]]

Pushed up the change that keeps a XBL binding only for construction / destruction / <content> to workaround the test leaks, along with various other fixes and changes to make updating the binding easier. Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84fffaeb0bc97034c20a9da475fae09804949ab5&group_state=expanded

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/7-8/

Comment 37

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172724/diff/3-4/

Comment 38

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/7-8/

Comment 39

[Brian Grinstead [:bgrins]]

Fixed out a few problems:
* Had to convert the <field id="tabbrowser"> in the tabbrowser-tabs binding to <property name="tabbrowser">. There were some tests where the field value was being cached before gBrowser was initialized
* There was still a case where the tabbrowser wasn't being interposed and also had to include this object in EventTargetParent.redirectEventTarget to get gBrowser.addEventListener working in tests
* Had to declare `var gBrowser = undefined` at the top of browser.js since otherwise bare references to gBrowser would throw in the web-panels.xul sidebar (this happened when opening the context menu on a web page opened in the sidebar)

Try is looking much better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=614c68fe122a70375ad8278ece6ecea77d5840bb&group_state=expanded. Still have a couple failing tests and some leaks on Windows and OSX but it's relatively green.

Comment 40

[Brian Grinstead [:bgrins]]

Ran into a tricky error when tracking down a debug leak by executing this in the Browser Console:

var win = window.open("about:blank", "", "width=100,height=100");

TypeError: this.tabbrowser is null  tabbrowser.xml:6435:9

  <anonymous> chrome://browser/content/tabbrowser.xml:6435:9
  tabs_XBL_Constructor chrome://global/content/bindings/tabbox.xml:265:1
  get chrome://browser/content/TabBrowser.js:52:37
  _updateNewTabVisibility chrome://browser/content/TabBrowser.js:6346:9
  TabBrowser chrome://browser/content/TabBrowser.js:556:5
  tabbrowser_XBL_Constructor chrome://browser/content/tabbrowser.xml:43:20

What is happening is that we have a constructor in the tabbrowser xbl binding doing:

  <constructor>
    gBrowser = new TabBrowser(this);
  </constructor>

This kicks off logic that used to be in the constructor, and includes a call to _updateNewTabVisibility. Inside there is ends up constructing the <tabs> XBL element due to accessing it for the first time. And in that constructor it ends up evaluating the "tabbrowser" property, which references gBrowser. But since we haven't finished running `new TabBrowser(this)` yet, gBrowser is null. This isn't a problem today because gBrowser is an alias to the DOM node (which exists during XBL construction). There are probably a couple of options here if we don't want to refactor the startup sequence:

1) Have a thinner constructor to set gBrowser and a separate init function that will trigger the call to updateNewTabVisibility
2) Make gBrowser a Custom Element instead, which will make it a DOM node again like in XBL.

To be fair, even without patches applied we get a different error:

TypeError: invalid 'in' operand browser tabbrowser.xml:2431:1
  _insertBrowser chrome://browser/content/tabbrowser.xml:2431:1
  getRelatedElement chrome://browser/content/tabbrowser.xml:7151:11
  set_selectedIndex chrome://global/content/bindings/tabbox.xml:406:31
  tabs_XBL_Constructor chrome://global/content/bindings/tabbox.xml:275:13
  <anonymous> chrome://browser/content/tabbrowser.xml:46:9
  _updateNewTabVisibility chrome://browser/content/tabbrowser.xml:5833:15
  tabbrowser_XBL_Constructor chrome://browser/content/tabbrowser.xml:5890:11

Comment 41

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 0.1 - Stop using the gBrowser name for view source windows

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

Comment 42

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/8-9/

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172724/diff/4-5/

Comment 44

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/8-9/

Comment 45

[Brian Grinstead [:bgrins]]

Other than debug leaks (which I believe are related to Comment 40), try is pretty green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56af24d65a396d7c26ccf26cbd630809597ba142&group_state=expanded

Comment 46

[Brian Grinstead [:bgrins]]

I think the issue in Comment 40 may be the same thing as Bug 1401846

Comment 47

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

yes! :) Glad to see you're on it, as I got stuck when tried to get this into 57 due to this bug :(

Comment 48

[Brian Grinstead [:bgrins]]

Attachment: tabbrowser-custom-element.patch

Have been trying an alternate approach of converting gBrowser into a Custom Element so we don't need to make as many changes (as it would still be a DOM node instead of a plain JS object). This patch is dependent on Bug 1404420.

Still lots of debug leaks, I think because unlike <destructor>, the `disconnectedCallback` doesn't seem to fire when the document is unloaded: https://treeherder.mozilla.org/#/jobs?repo=try&revision=997c10f6d37e9c285f573a237d78f97f0a2fdcef.

Talos shows some regressions and a couple of improvements: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c0940c698bc3908463f9f8c9a702edd361cab9db&newProject=try&newRevision=997c10f6d37e9c285f573a237d78f97f0a2fdcef&framework=1&showOnlyImportant=1

Comment 49

[Brian Grinstead [:bgrins]]

Latest try push (rebased, Custom Element implementation): https://treeherder.mozilla.org/#/jobs?repo=try&revision=a920cf8cea8bc409f12b15251312c2029d20d8b7

Comment 50

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #25)
> Something else I noticed that may be related to the debug leaks is that
> window.onunload doesn't get called in browser.xul at least in certain cases
> (and the TabBrowser.disconnectedCallback function is called from there).
> Presumably the XBL binding destructor is called no matter what when the DOM
> node gets removed.
> 
> For instance, if I do `./mach mochitest
> browser/base/content/test/sidebar/browser_sidebar_move.js --setpref
> browser.dom.window.dump.enabled=true` and add a dump on the first line of
> `gBrowserInit.onUnload`, it never gets called.  This is true both with and
> without patches from this bug applied.
> 
> However, the gBrowserInit.onUnload function does appear to be called in
> non-test scenarios like:
> 
>   * Start process
>   * Open new window
>   * Close the new window
>   * gBrowserInit.onUnload gets called for the new window
> 
> and
> 
>   * Start process
>   * Quit process
>   * gBrowserInit.onUnload gets called for the initial window
> 
> Bill, are you aware of a case where window.onunload is expected to not fire
> and the test somehow hits it, or is this a bug?

Just to follow up here - the unload event does end up firing even in the test harness if we do addEventListener("unload") in browser.js as in: https://hg.mozilla.org/try/rev/1974aef6c81031da29bebd908954ce201cbabe52#l4.12.

But making that change alone seems to cause some test failures on "unknown test url" i.e. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4a12be5163716a4613664ac90363e41205d53a0&selectedJob=139574030

Comment 51

[Brian Grinstead [:bgrins]]

Once addon interposition is removed in Bug 1412456 it will simplify migration to a class, since all of part 3 and the addoncompat portions of part 4 could be removed.

Comment 52

[Brian Grinstead [:bgrins]]

Note from standard8: it'd be better to use `// eslint-disable-next-line complexity` in front of addTab instead of `/* eslint-disable complexity */`

Comment 53

[Brian Grinstead [:bgrins]]

Comment on attachment 8915775 [details]
Bug 1392352 - Part 0.1 - Stop using the gBrowser name for view source windows

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

Comment 54

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/9-10/

Comment 55

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172724/diff/5-6/

Comment 56

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/9-10/

Comment 57

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 3 - hg cp tabbrowser.xml to tabbrowser.js;

This will allow us to preserve blame when this file gets rewritten to JS

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

Comment 58

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 4.2 - To fold: remove build instructions and reorganize things a bit;

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

Comment 59

[Brian Grinstead [:bgrins]]

Comment on attachment 8915775 [details]
Bug 1392352 - Part 0.1 - Stop using the gBrowser name for view source windows

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

Comment 60

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172724/diff/6-7/

Comment 61

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/10-11/

Comment 62

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/10-11/

Comment 63

[Brian Grinstead [:bgrins]]

I'm pretty sure the two part 0 patches can be removed once bug 1412456 and bug 1418403 land. Otherwise this is looking pretty good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=998e2e880b4c1b701c799cab1ac82669dad1fe3e&selectedJob=162077671

Comment 64

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172724/diff/7-8/

Comment 65

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/11-12/

Comment 66

[Brian Grinstead [:bgrins]]

Comment on attachment 8950800 [details]
Bug 1392352 - Part 3 - hg cp tabbrowser.xml to tabbrowser.js;

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

Comment 67

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/11-12/

Comment 68

[Brian Grinstead [:bgrins]]

Comment on attachment 8950801 [details]
Bug 1392352 - Part 4.2 - To fold: remove build instructions and reorganize things a bit;

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

Comment 69

[Brian Grinstead [:bgrins]]

Looks like there's a 3% session restore win https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b32d68eaac4424d496377e1fa7d34b9007b538ea&newProject=try&newRevision=9b35b4bcb9eda2dc25fb11f760919bc2c39f3146&framework=1&showOnlyImportant=1.

Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf10c93095016a687580ac4003618bc1f67371b

Comment 70

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 4.0 - Translate the tabbrowser implementation into a JS class

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

Comment 71

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172724/diff/8-9/

Comment 72

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/12-13/

Comment 73

[Brian Grinstead [:bgrins]]

Comment on attachment 8950800 [details]
Bug 1392352 - Part 3 - hg cp tabbrowser.xml to tabbrowser.js;

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

Comment 74

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/12-13/

Comment 75

[Brian Grinstead [:bgrins]]

Comment on attachment 8950801 [details]
Bug 1392352 - Part 4.2 - To fold: remove build instructions and reorganize things a bit;

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

Comment 76

[Brian Grinstead [:bgrins]]

Let me know if there's some way to make it easier to review the patches - I've tried to split it up as much as possible

Comment 77

[Kris Maglione [:kmag]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

https://reviewboard.mozilla.org/r/172724/#review226172

I'd have to think through all of the implications of this, which probably isn't worth it for a temporary hack.

Can you add a property named something like `requiresAddonInterpositions` to the `gBrowser` object, and only interpose plain objects if it's present and true?

Comment 78

[Dão Gottwald [:dao]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

https://reviewboard.mozilla.org/r/170858/#review226430

Comment 79

[Dão Gottwald [:dao]]

Comment on attachment 8950800 [details]
Bug 1392352 - Part 3 - hg cp tabbrowser.xml to tabbrowser.js;

https://reviewboard.mozilla.org/r/220032/#review226432

Comment 80

[Dão Gottwald [:dao]]

Comment on attachment 8950801 [details]
Bug 1392352 - Part 4.2 - To fold: remove build instructions and reorganize things a bit;

https://reviewboard.mozilla.org/r/220034/#review226434

Comment 81

[Dão Gottwald [:dao]]

Comment on attachment 8951011 [details]
Bug 1392352 - Part 4.0 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220266/#review226436

Comment 82

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Clearing review while I work on reformatting this to make the review easier

Comment 83

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 4.3 - To fold: delete the old XBL implementation of tabbrowser;

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

Comment 84

[Brian Grinstead [:bgrins]]

Comment on attachment 8915775 [details]
Bug 1392352 - Part 0.1 - Stop using the gBrowser name for view source windows

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186996/diff/3-4/

Comment 85

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172724/diff/9-10/

Comment 86

[Brian Grinstead [:bgrins]]

Comment on attachment 8899558 [details]
Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170858/diff/13-14/

Comment 87

[Brian Grinstead [:bgrins]]

Comment on attachment 8950800 [details]
Bug 1392352 - Part 3 - hg cp tabbrowser.xml to tabbrowser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220032/diff/3-4/

Comment 88

[Brian Grinstead [:bgrins]]

Comment on attachment 8951011 [details]
Bug 1392352 - Part 4.0 - Translate the tabbrowser implementation into a JS class

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

Comment 89

[Brian Grinstead [:bgrins]]

Comment on attachment 8899559 [details]
Bug 1392352 - Part 4.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170860/diff/13-14/

Comment 90

[Brian Grinstead [:bgrins]]

Comment on attachment 8950801 [details]
Bug 1392352 - Part 4.2 - To fold: remove build instructions and reorganize things a bit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220034/diff/3-4/

Comment 91

[Brian Grinstead [:bgrins]]

Comment on attachment 8901277 [details]
Bug 1392352 - Part 1 - Interpose all objects;

This series seriously confused reviewboard - this one doesn't need review. Let me see if I can close the review request and restart

Comment 92

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 0.1 - Stop using the gBrowser name for view source windows

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

Comment 93

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 1 - Interpose all objects;

We used to attempt interposition only for DOM instances and wrapped natives,
but now that tests are the only thing still using interposition this removes
the optimization.

This is needed so that `gBrowser` can continue to be interposed as a plain JS
object, until addon interposition is removed entirely (in Bug 1412456).

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

Comment 94

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 2 - Refer to the tabbrowser element as gBrowser instead of #content;

Since gBrowser is going to become a plain JS object instead of a DOM node,
we don't want any callers directly referring to the DOM node to get ahold of it.

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

Comment 95

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

This will allow us to preserve blame as best as possible when the implementation
of this binding gets rewritten to JS in the next changeset.

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

Comment 96

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

We keep the XBL binding around for <content>, <constructor>, and <destructor>. This can
eventually be migrated to a Custom Element once we have platform support, but in the meantime
this is a way to get the many thousands of LOC into a JS class.

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

Comment 97

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

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

Comment 98

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 2.2 - To fold: remove build instructions and reorganize things a bit;

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

Comment 99

[Brian Grinstead [:bgrins]]

Attachment: Bug 1392352 - Part 2.3 - To fold: delete the old XBL implementation of tabbrowser;

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

Comment 100

[Brian Grinstead [:bgrins]]

OK, this should be better now:

* I switched from using prettier which rewrote a lot of the tabbrowser code to using js_beautify which can be configured to handle more simple indentation-style changes.
* I also reordered the class so that the destructor happens after all the methods and the handlers are registered in a setupHandlers function below that (so it more closely fits with the XBL order). We may want to reorder these things in a follow-up.
* I split out the code that actually integrates with the class into a 4.1 so it's not inside the same review page

With these changes, the bulk of part 4 (class migration) is whitespace changes - but please have a close look at the parts that aren't.

Dão, sorry for the review spam - I didn't intend for it to re-request review for parts 3, 4.2, and 4.3 (which you already r+ed) and I'm not sure how that happened.

Comment 101

[Brian Grinstead [:bgrins]]

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

Comment 102

[Dão Gottwald [:dao]]

Comment on attachment 8951402 [details]
Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

https://reviewboard.mozilla.org/r/220682/#review226736

Comment 103

[Dão Gottwald [:dao]]

Comment on attachment 8951405 [details]
Bug 1392352 - Part 2.2 - To fold: remove build instructions and reorganize things a bit;

https://reviewboard.mozilla.org/r/220688/#review226754

Comment 104

[Dão Gottwald [:dao]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220684/#review226738

This looks good to me, but I stopped in the middle; there are enough nits that I think you might want to tweak how you use js_beautify.

::: browser/base/content/tabbrowser.js:55
(Diff revision 1)
> +
> +    this.closingTabsEnum = ({
> +      ALL: 0,
> +      OTHER: 1,
> +      TO_END: 2
> +    });

nit: remove (). AFAIK this was only needed because it wouldn't work in an XBL field otherwise.

::: browser/base/content/tabbrowser.js:521
(Diff revision 1)
> -                .map(tab => {
> +        .map(tab => {
> -                  const throbber =
> +          const throbber =
> -                    document.getAnonymousElementByAttribute(tab, "anonid", "tab-throbber");
> +            document.getAnonymousElementByAttribute(tab, "anonid", "tab-throbber");
> -                  return throbber ? throbber.getAnimations({ subtree: true }) : [];
> +          return throbber ? throbber.getAnimations({
> +            subtree: true
> +          }) : [];

I slightly prefer how this was one line before...

::: browser/base/content/tabbrowser.js:578
(Diff revision 1)
> -          // over all browsers.
> +    // over all browsers.
> -          if (!gMultiProcessBrowser) {
> +    if (!gMultiProcessBrowser) {
> -            let browser = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +      let browser = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> -                                 .getInterface(Ci.nsIWebNavigation)
> +        .getInterface(Ci.nsIWebNavigation)
> -                                 .QueryInterface(Ci.nsIDocShell)
> +        .QueryInterface(Ci.nsIDocShell)
> -                                 .chromeEventHandler;
> +        .chromeEventHandler;

This looked more readable to me with the original indentation.

::: browser/base/content/tabbrowser.js:762
(Diff revision 1)
>  
> -              this._callProgressListeners("onProgressChange",
> +        this._callProgressListeners("onProgressChange", [aWebProgress, aRequest,
> -                                          [aWebProgress, aRequest,
> -                                           aCurSelfProgress, aMaxSelfProgress,
> +          aCurSelfProgress, aMaxSelfProgress,
> -                                           aCurTotalProgress, aMaxTotalProgress]);
> +          aCurTotalProgress, aMaxTotalProgress
> +        ]);

ditto

::: browser/base/content/tabbrowser.js:873
(Diff revision 1)
> -                          SchedulePressure.stopMonitoring(window);
> +                    SchedulePressure.stopMonitoring(window);
> -                          continueMonitoring = false;
> +                    continueMonitoring = false;
> -                        }
> +                  }
> -                        return {continueMonitoring};
> +                  return {
> +                    continueMonitoring
> +                  };

ditto

::: browser/base/content/tabbrowser.js:884
(Diff revision 1)
> -                  if (this.mTab.selected) {
> +            if (this.mTab.selected) {
> -                    this.mTabBrowser.mIsBusy = true;
> +              this.mTabBrowser.mIsBusy = true;
> -                  }
> +            }
> -                }
> +          }
> -              } else if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
> +        } else if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
> -                         aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {
> +          aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {

ditto

::: browser/base/content/tabbrowser.js:956
(Diff revision 1)
> -              }
> +        }
>  
> -              if (ignoreBlank) {
> +        if (ignoreBlank) {
> -                this._callProgressListeners("onUpdateCurrentBrowser",
> +          this._callProgressListeners("onUpdateCurrentBrowser", [aStateFlags, aStatus, "", 0],
> -                                            [aStateFlags, aStatus, "", 0],
> -                                            true, false);
> +            true, false);

Here I'm not so sure. I would probably still format this as done originally, but your change wfm too.

::: browser/base/content/tabbrowser.js:966
(Diff revision 1)
> -              this._callProgressListeners("onStateChange",
> +        this._callProgressListeners("onStateChange", [aWebProgress, aRequest, aStateFlags, aStatus],
> -                                          [aWebProgress, aRequest, aStateFlags, aStatus],
> -                                          false);
> +          false);
>  
> -              if (aStateFlags & (nsIWebProgressListener.STATE_START |
> +        if (aStateFlags & (nsIWebProgressListener.STATE_START |
> -                                 nsIWebProgressListener.STATE_STOP)) {
> +            nsIWebProgressListener.STATE_STOP)) {

this is less readable IMHO

::: browser/base/content/tabbrowser.js:1066
(Diff revision 1)
> -              }
> +        }
>  
> -              if (!this.mBlank) {
> +        if (!this.mBlank) {
> -                this._callProgressListeners("onLocationChange",
> -                                            [aWebProgress, aRequest, aLocation,
> -                                             aFlags]);
> +          this._callProgressListeners("onLocationChange", [aWebProgress, aRequest, aLocation,
> +            aFlags
> +          ]);

ditto

::: browser/base/content/tabbrowser.js:1312
(Diff revision 1)
> -            gURLBar.setAttribute("switchingtabs", "true");
> +    gURLBar.setAttribute("switchingtabs", "true");
> -            window.addEventListener("MozAfterPaint", function() {
> +    window.addEventListener("MozAfterPaint", function() {
> -              gURLBar.removeAttribute("switchingtabs");
> +      gURLBar.removeAttribute("switchingtabs");
> -            }, {once: true});
> +    }, {
> +      once: true
> +    });

ditto

::: browser/base/content/tabbrowser.js:1340
(Diff revision 1)
> -            var listener = this._tabListeners.get(this.mCurrentTab);
> +    var listener = this._tabListeners.get(this.mCurrentTab);
> -            if (listener && listener.mStateFlags) {
> +    if (listener && listener.mStateFlags) {
> -              this._callProgressListeners(null, "onUpdateCurrentBrowser",
> -                                          [listener.mStateFlags, listener.mStatus,
> -                                           listener.mMessage, listener.mTotalProgress],
> +      this._callProgressListeners(null, "onUpdateCurrentBrowser", [listener.mStateFlags, listener.mStatus,
> +          listener.mMessage, listener.mTotalProgress
> +        ],
> -                                          true, false);
> +        true, false);

ditto

This seems to be a reoccurring problem with _callProgressListeners calls, so I'll stop mentioning it.

::: browser/base/content/tabbrowser.js:1679
(Diff revision 1)
>        }
>      }
>  
> -            return this._setTabLabel(aTab, title, { isContentTitle });
> -          ]]>
> -        </body>
> +    return this._setTabLabel(aTab, title, {
> +      isContentTitle
> +    });

prefer the original formatting

::: browser/base/content/tabbrowser.js:1743
(Diff revision 1)
>      var aNextTabParentId;
>      var aFocusUrlBar;
>      var aName;
>      if (arguments.length == 2 &&
>        typeof arguments[1] == "object" &&
>        !(arguments[1] instanceof Ci.nsIURI)) {

indentation

::: browser/base/content/tabbrowser.js:1767
(Diff revision 1)
> -              aOpener                   = params.opener;
> -              aOpenerBrowser            = params.openerBrowser;
> -              aCreateLazyBrowser        = params.createLazyBrowser;
> -              aNextTabParentId          = params.nextTabParentId;
> -              aFocusUrlBar              = params.focusUrlBar;
> -              aName                     = params.name;
> +      aOpener = params.opener;
> +      aOpenerBrowser = params.openerBrowser;
> +      aCreateLazyBrowser = params.createLazyBrowser;
> +      aNextTabParentId = params.nextTabParentId;
> +      aFocusUrlBar = params.focusUrlBar;
> +      aName = params.name;

Hmm, hmm, I kind of like the original format but don't feel strongly about it. Same in loadTabs and addTab.

::: browser/base/content/tabbrowser.js:2022
(Diff revision 1)
> -            aBrowser.webProgress.addProgressListener(filter, Ci.nsIWebProgress.NOTIFY_ALL);
> +    aBrowser.webProgress.addProgressListener(filter, Ci.nsIWebProgress.NOTIFY_ALL);
>  
> -            // Restore the securityUI state.
> +    // Restore the securityUI state.
> -            let securityUI = aBrowser.securityUI;
> +    let securityUI = aBrowser.securityUI;
> -            let state = securityUI ? securityUI.state
> -                                   : Ci.nsIWebProgressListener.STATE_IS_INSECURE;
> +    let state = securityUI ? securityUI.state :
> +      Ci.nsIWebProgressListener.STATE_IS_INSECURE;

ditto

::: browser/base/content/tabbrowser.js:2133
(Diff revision 1)
> -              return;
> +      return;
> -            }
> +    }
>  
> -            let remoteType =
> +    let remoteType =
> -              E10SUtils.getRemoteTypeForURI(BROWSER_NEW_TAB_URL,
> +      E10SUtils.getRemoteTypeForURI(BROWSER_NEW_TAB_URL,
> -                                            gMultiProcessBrowser);
> +        gMultiProcessBrowser);

This fits on one line now

::: browser/base/content/tabbrowser.js:2259
(Diff revision 1)
> -            browserContainer.appendChild(stack);
> +    browserContainer.appendChild(stack);
> -            browserContainer.setAttribute("flex", "1");
> +    browserContainer.setAttribute("flex", "1");
>  
> -            // Create the sidebar container
> +    // Create the sidebar container
> -            var browserSidebarContainer = document.createElementNS(NS_XUL,
> +    var browserSidebarContainer = document.createElementNS(NS_XUL,
> -                                                                   "hbox");
> +      "hbox");

ditto

::: browser/base/content/tabbrowser.js:2266
(Diff revision 1)
> -            browserSidebarContainer.appendChild(browserContainer);
> +    browserSidebarContainer.appendChild(browserContainer);
> -            browserSidebarContainer.setAttribute("flex", "1");
> +    browserSidebarContainer.setAttribute("flex", "1");
>  
> -            // Add the Message and the Browser to the box
> +    // Add the Message and the Browser to the box
> -            var notificationbox = document.createElementNS(NS_XUL,
> +    var notificationbox = document.createElementNS(NS_XUL,
> -                                                           "notificationbox");
> +      "notificationbox");

ditto

::: browser/base/content/tabbrowser.js:2330
(Diff revision 1)
> -                      // initializing the reload.
> +              // initializing the reload.
> -                      aTab.addEventListener("SSTabRestoring", () => {
> +              aTab.addEventListener("SSTabRestoring", () => {
> -                        browser[name](params);
> +                browser[name](params);
> -                      }, { once: true });
> +              }, {
> +                once: true
> +              });

I prefer this on one line

::: browser/base/content/tabbrowser.js:2401
(Diff revision 1)
> -            let { uriIsAboutBlank, remoteType, usingPreloadedContent } =
> +    let {
> +      uriIsAboutBlank,
> +      remoteType,
> +      usingPreloadedContent
> +    } =
> -                    aTab._browserParams;
> +    aTab._browserParams;

Ugh, this looks really wrong.

::: browser/base/content/tabbrowser.js:2508
(Diff revision 1)
>  
> -            this._createLazyBrowser(tab);
> +    this._createLazyBrowser(tab);
>  
> -            let evt = new CustomEvent("TabBrowserDiscarded", { bubbles: true });
> +    let evt = new CustomEvent("TabBrowserDiscarded", {
> +      bubbles: true
> +    });

This looks fine to me on one line.

::: browser/base/content/tabbrowser.js:2618
(Diff revision 1)
> -                t.setAttribute("label", gTabBrowserBundle.GetStringFromName("tabs.emptyTabTitle"));
> +        t.setAttribute("label", gTabBrowserBundle.GetStringFromName("tabs.emptyTabTitle"));
> -              } else {
> +      } else {
> -                // Set URL as label so that the tab isn't empty initially.
> +        // Set URL as label so that the tab isn't empty initially.
> -                this.setInitialTabTitle(t, aURI, { beforeTabOpen: true });
> +        this.setInitialTabTitle(t, aURI, {
> +          beforeTabOpen: true
> +        });

This looks fine to me on one line

::: browser/base/content/tabbrowser.js:2785
(Diff revision 1)
> -            t.dispatchEvent(evt);
> +    t.dispatchEvent(evt);
>  
> -            if (!usingPreloadedContent && aOriginPrincipal && aURI) {
> +    if (!usingPreloadedContent && aOriginPrincipal && aURI) {
> -              let {URI_INHERITS_SECURITY_CONTEXT} = Ci.nsIProtocolHandler;
> +      let {
> +        URI_INHERITS_SECURITY_CONTEXT
> +      } = Ci.nsIProtocolHandler;

one line please

Comment 105

[Dão Gottwald [:dao]]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

https://reviewboard.mozilla.org/r/220686/#review226762

::: browser/base/content/tabbrowser.xml:52
(Diff revision 1)
> +      </constructor>
> +      <destructor>
> +        gBrowser.disconnectedCallback();
> +      </destructor>
> +    </implementation>
> +  </binding>

Part 4.2 folded into this would have made the review  easier... I'll assume that the <content>...</content> part remains mostly untouched when both patches are folded together.

::: toolkit/components/addoncompat/multiprocessShims.js:119
(Diff revision 1)
>          return "RemoteBrowserElement";
>        }
> +    }
>  
> -      if (target.localName == "tabbrowser") {
> +    if (target.ownerGlobal && target === target.ownerGlobal.gBrowser) {
>          return "TabBrowserElement";

nit: indentation

Comment 106

[Dão Gottwald [:dao]]

Comment on attachment 8951406 [details]
Bug 1392352 - Part 2.3 - To fold: delete the old XBL implementation of tabbrowser;

https://reviewboard.mozilla.org/r/220690/#review226764

Comment 107

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220684/#review226930

I made a couple of changes to the script - I'll push up a new version that should hopefully fix the brace issues

::: browser/base/content/tabbrowser.js:55
(Diff revision 1)
> +
> +    this.closingTabsEnum = ({
> +      ALL: 0,
> +      OTHER: 1,
> +      TO_END: 2
> +    });

Done, updated the script to strip parens around `<fields>`

::: browser/base/content/tabbrowser.js:521
(Diff revision 1)
> -                .map(tab => {
> +        .map(tab => {
> -                  const throbber =
> +          const throbber =
> -                    document.getAnonymousElementByAttribute(tab, "anonid", "tab-throbber");
> +            document.getAnonymousElementByAttribute(tab, "anonid", "tab-throbber");
> -                  return throbber ? throbber.getAnimations({ subtree: true }) : [];
> +          return throbber ? throbber.getAnimations({
> +            subtree: true
> +          }) : [];

I found an option to preserve this as one line - this change should solve a lot of the style issues

::: browser/base/content/tabbrowser.js:578
(Diff revision 1)
> -          // over all browsers.
> +    // over all browsers.
> -          if (!gMultiProcessBrowser) {
> +    if (!gMultiProcessBrowser) {
> -            let browser = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +      let browser = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> -                                 .getInterface(Ci.nsIWebNavigation)
> +        .getInterface(Ci.nsIWebNavigation)
> -                                 .QueryInterface(Ci.nsIDocShell)
> +        .QueryInterface(Ci.nsIDocShell)
> -                                 .chromeEventHandler;
> +        .chromeEventHandler;

I can't find an option to keep this type of formatting. I can do manual cleanups on this but depending on how many rebases it takes I'd prefer to do that in a follow-up.

I think the bulk of the issues will be resolved with the brace change above.

::: browser/base/content/tabbrowser.js:1340
(Diff revision 1)
> -            var listener = this._tabListeners.get(this.mCurrentTab);
> +    var listener = this._tabListeners.get(this.mCurrentTab);
> -            if (listener && listener.mStateFlags) {
> +    if (listener && listener.mStateFlags) {
> -              this._callProgressListeners(null, "onUpdateCurrentBrowser",
> -                                          [listener.mStateFlags, listener.mStatus,
> -                                           listener.mMessage, listener.mTotalProgress],
> +      this._callProgressListeners(null, "onUpdateCurrentBrowser", [listener.mStateFlags, listener.mStatus,
> +          listener.mMessage, listener.mTotalProgress
> +        ],
> -                                          true, false);
> +        true, false);

Can we make eslint --fix put things in the indentation that we want here? I agree with the preference but this seems like something that would be better to enforce at the linting level

Comment 108

[Brian Grinstead [:bgrins]]

Comment on attachment 8951399 [details]
Bug 1392352 - Part 0.1 - Stop using the gBrowser name for view source windows

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

Comment 109

[Brian Grinstead [:bgrins]]

Comment on attachment 8951400 [details]
Bug 1392352 - Part 1 - Interpose all objects;

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

Comment 110

[Brian Grinstead [:bgrins]]

Comment on attachment 8951402 [details]
Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

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

Comment 111

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

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

Comment 112

[Brian Grinstead [:bgrins]]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

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

Comment 113

[Brian Grinstead [:bgrins]]

Comment on attachment 8951405 [details]
Bug 1392352 - Part 2.2 - To fold: remove build instructions and reorganize things a bit;

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

Comment 114

[Brian Grinstead [:bgrins]]

Comment on attachment 8951406 [details]
Bug 1392352 - Part 2.3 - To fold: delete the old XBL implementation of tabbrowser;

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

Comment 115

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

Clearing :dao review since I haven't addressed all the comments

Comment 116

[Brian Grinstead [:bgrins]]

Rebased on top of Bug 1438579 and with some converter script changes to better align with existing style (see Comment 107)

Comment 117

[Brian Grinstead [:bgrins]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #77)
> Comment on attachment 8901277 [details]
> Bug 1392352 - Part 1 - Interpose all objects;
> 
> https://reviewboard.mozilla.org/r/172724/#review226172
> 
> I'd have to think through all of the implications of this, which probably
> isn't worth it for a temporary hack.
> 
> Can you add a property named something like `requiresAddonInterpositions` to
> the `gBrowser` object, and only interpose plain objects if it's present and
> true?

I tried doing this but ended up with 'too much recursion' on some tests like browser_bug575561.js. I think that the call to JS_HasOwnProperty [0] when looking for "requiresAddonInterpositions" ends up triggering a call back into InterposeProperty. I'm probably missing something obvious - is there a better way to accomplish this? FWIW we also know the object will have been created via `new TabBrowser()` so if there's a way to check the constructor name that may be another option.

[0]: https://hg.mozilla.org/try/rev/3f36f17e40f11645588c2fdb89920451a42fb3ff#l1.21

Comment 118

[Dão Gottwald [:dao]]

A more fundamental thought on your approach: If we put this class in a jsm, we could share it across windows. Alternatively, we could make this a plain per-window object. This being a class that will only ever have one instance seems a bit pointless. I'm fine with initially landing it as-is, though.

Comment 119

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #118)
> A more fundamental thought on your approach: If we put this class in a jsm,
> we could share it across windows. Alternatively, we could make this a plain
> per-window object. This being a class that will only ever have one instance
> seems a bit pointless. I'm fine with initially landing it as-is, though.

I'm not sure about a JSM - tabbrowser relies on a bunch of window globals which would make it more difficult to migrate. Also, as per Comment 7 it may not actually be faster.

But converting this into a plain gBrowser object which gets `init` and `uninit` calls from the XBL binding makes a lot of sense. The main downside I can think of is that if a caller accessed gBrowser before the binding got attached it'd be harder to debug the problem (instead of "gBrowser is null" you'd get a non-obvious error inside of whatever was being called before `init` happened). But I still think it'd be an improvement - I can look into doing it that way next week.

Comment 120

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #119)
> But converting this into a plain gBrowser object which gets `init` and
> `uninit` calls from the XBL binding makes a lot of sense. The main downside
> I can think of is that if a caller accessed gBrowser before the binding got
> attached it'd be harder to debug the problem (instead of "gBrowser is null"
> you'd get a non-obvious error inside of whatever was being called before
> `init` happened). But I still think it'd be an improvement - I can look into
> doing it that way next week.

We could name the object differently and assign gBrowser only upon initialization...

Comment 121

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

https://reviewboard.mozilla.org/r/220686/#review228028

::: toolkit/components/addoncompat/multiprocessShims.js:119
(Diff revision 2)
>          return "RemoteBrowserElement";
>        }
> +    }
>  
> -      if (target.localName == "tabbrowser") {
> +    if (target.ownerGlobal && target === target.ownerGlobal.gBrowser) {
>          return "TabBrowserElement";

Nit: two spaces

Comment 122

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

https://reviewboard.mozilla.org/r/220686/#review228028

> Nit: two spaces

Whoops - looks like dao already caught that one. Dropping.

Comment 123

[Brian Grinstead [:bgrins]]

Comment on attachment 8951400 [details]
Bug 1392352 - Part 1 - Interpose all objects;

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

Comment 124

[Brian Grinstead [:bgrins]]

Comment on attachment 8951402 [details]
Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

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

Comment 125

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

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

Comment 126

[Brian Grinstead [:bgrins]]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

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

Comment 127

[Brian Grinstead [:bgrins]]

Comment on attachment 8951405 [details]
Bug 1392352 - Part 2.2 - To fold: remove build instructions and reorganize things a bit;

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

Comment 128

[Brian Grinstead [:bgrins]]

Comment on attachment 8951406 [details]
Bug 1392352 - Part 2.3 - To fold: delete the old XBL implementation of tabbrowser;

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

Comment 129

[Brian Grinstead [:bgrins]]

Pushed up the whitespace fix on 3.1 and removed part 0

Comment 130

[Dão Gottwald [:dao]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220684/#review228162

::: browser/base/content/tabbrowser.js
(Diff revision 3)
> -          ]]>
> -        </body>
> +  }
> +  shouldActivateDocShell(aBrowser) {
> -      </method>
> -
> -      <!--
> -        Returns true if a given browser's docshell should be active.

We should preserve these comments.

::: browser/base/content/tabbrowser.js:4262
(Diff revision 3)
>              browser.frameLoader.tabParent);
>          if (tab == this.requestedTab &&
>            canCheckDocShellState &&
>            state == this.STATE_LOADED &&
>            !browser.docShellIsActive &&
>            !this.minimizedOrFullyOccluded) {

indentation

::: browser/base/content/tabbrowser.js:4416
(Diff revision 3)
>  
> -                if (state == this.STATE_LOADED &&
> +          if (state == this.STATE_LOADED &&
> -                    !this.maybeVisibleTabs.has(tab) &&
> +            !this.maybeVisibleTabs.has(tab) &&
> -                    tab !== this.lastVisibleTab &&
> +            tab !== this.lastVisibleTab &&
> -                    tab !== this.loadingTab &&
> +            tab !== this.loadingTab &&
> -                    tab !== this.requestedTab) {
> +            tab !== this.requestedTab) {

indentation

::: browser/base/content/tabbrowser.js:4731
(Diff revision 3)
> -              this.preActions();
> +        this.preActions();
>  
> -              if (event.type == "MozLayerTreeReady") {
> +        if (event.type == "MozLayerTreeReady") {
> -                this.onLayersReady(event.originalTarget);
> +          this.onLayersReady(event.originalTarget);
> -              } if (event.type == "MozAfterPaint") {
> +        }
> +        if (event.type == "MozAfterPaint") {

this should be else if instead

::: browser/base/content/tabbrowser.js:4738
(Diff revision 3)
> -              } else if (event.type == "MozLayerTreeCleared") {
> +        } else if (event.type == "MozLayerTreeCleared") {
> -                this.onLayersCleared(event.originalTarget);
> +          this.onLayersCleared(event.originalTarget);
> -              } else if (event.type == "TabRemotenessChange") {
> +        } else if (event.type == "TabRemotenessChange") {
> -                this.onRemotenessChange(event.target);
> +          this.onRemotenessChange(event.target);
> -              } else if (event.type == "sizemodechange" ||
> +        } else if (event.type == "sizemodechange" ||
> -                         event.type == "occlusionstatechange") {
> +          event.type == "occlusionstatechange") {

indentation

::: browser/base/content/tabbrowser.js:4944
(Diff revision 3)
> -          }
> +    }
>  
> -          if (AppConstants.platform != "macosx") {
> +    if (AppConstants.platform != "macosx") {
> -            if (aEvent.ctrlKey && !aEvent.shiftKey && !aEvent.metaKey &&
> +      if (aEvent.ctrlKey && !aEvent.shiftKey && !aEvent.metaKey &&
> -                aEvent.keyCode == KeyEvent.DOM_VK_F4 &&
> +        aEvent.keyCode == KeyEvent.DOM_VK_F4 &&
> -                !this.mCurrentTab.pinned) {
> +        !this.mCurrentTab.pinned) {

indentation

::: browser/base/content/tabbrowser.js:5311
(Diff revision 3)
>  
> -          Services.prefs.removeObserver("accessibility.typeaheadfind", this);
> +    Services.prefs.removeObserver("accessibility.typeaheadfind", this);
> -        ]]>
> +  }
> -      </destructor>
>  
> -      <field name="_soundPlayingAttrRemovalTimer">0</field>
> +  setupHandlers() {

let's call this _setupEventListeners or _addEventListeners

Comment 131

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220684/#review228162

> let's call this _setupEventListeners or _addEventListeners

I think I prefer `_setupEventListeners`, just to avoid any possible confusion with `addEventListener`.

Comment 132

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220684/#review228424

Hey bgrinstead,

Hoooo that was a doozy of a review! Thanks for doing everything you could to make this simple for us - this could easily have been a fully green file that we needed to hand-compare against the original, so the fact that you did so much to make it easy to compare within MozReview was a huge help.

A few notes below. I think my main concern is just preserving documentation, and maybe a thing can be lazified. The rest is just style-stuff that I don't feel super strong about.

::: browser/base/content/tabbrowser.js:43
(Diff revision 3)
> +    XPCOMUtils.defineLazyServiceGetters(this, {
> +      _unifiedComplete: ["@mozilla.org/autocomplete/search;1?name=unifiedcomplete", "mozIPlacesAutoComplete"],
> +      serializationHelper: ["@mozilla.org/network/serialization-helper;1", "nsISerializationHelper"],
> +    });
> +
> +    this.mURIFixup = Services.uriFixup;

If I remember correctly, XBL <field>'s are lazily evaluated, and this here is not, meaning that we're going to be kicking off the nsIURIFixup service a lot sooner than we did before.

Is that intentional, or can we re-lazify this?

And, come to think of it, that kinda applies to all of the fields that are being defined here... all of them used to be lazy. Do any of them kick off services or anything? If so, can we re-lazify them?

::: browser/base/content/tabbrowser.js:406
(Diff revision 3)
> +
> +  set tabMinWidth(val) {
> +    this.tabContainer.style.setProperty("--tab-min-width", val + "px");
> +    return val;
> +  }
> +  isFindBarInitialized(aTab) {

While we're here, we should probably try to keep things consistently spaced, so I guess we should put some newlines around this, and the other functions down below, unless they're being put together for reasons I can't see.

::: browser/base/content/tabbrowser.js:750
(Diff revision 3)
> -              this._callProgressListeners("onProgressChange",
> +        this._callProgressListeners("onProgressChange", [aWebProgress, aRequest,
> -                                          [aWebProgress, aRequest,
> -                                           aCurSelfProgress, aMaxSelfProgress,
> +          aCurSelfProgress, aMaxSelfProgress,
> -                                           aCurTotalProgress, aMaxTotalProgress]);
> +          aCurTotalProgress, aMaxTotalProgress
> +        ]);

If you're going to do this, you might as well maybe just do:

```js
this._callProgressListeners("onProgressChange", [
  aWebProgress, aRequest,
  aCurSelfProgress, aMaxSelfProgress,
  aCurTotalProgress, aMaxTotalProgress
]);
```

or

```js
this._callProgressListeners("onProgressChange", [
  aWebProgress,
  aRequest,
  aCurSelfProgress,
  aMaxSelfProgress,
  aCurTotalProgress,
  aMaxTotalProgress,
]);
```

Though I guess I don't feel super strongly about which - just that the original here seems mildly awkward.

::: browser/base/content/tabbrowser.js:1053
(Diff revision 3)
> -                this._callProgressListeners("onLocationChange",
> -                                            [aWebProgress, aRequest, aLocation,
> -                                             aFlags]);
> +          this._callProgressListeners("onLocationChange", [aWebProgress, aRequest, aLocation,
> +            aFlags
> +          ]);

Like above, feels kinda awkward to have these elements split up over two lines like this and to have `aFlags` dangling like this...

Or is this some kind of ESLint style we're trying to converge on? If so, I can let it go - so long as we become consistent.

::: browser/base/content/tabbrowser.js:1596
(Diff revision 3)
> -          // ensure bool if undefined
> +    // ensure bool if undefined
> -          state.camera = !!state.camera;
> +    state.camera = !!state.camera;
> -          state.microphone = !!state.microphone;
> +    state.microphone = !!state.microphone;
> -          return state;
> +    return state;
> -        ]]></body>
> -      </method>
> +  }
> +  setTabTitleLoading(aTab) {} setInitialTabTitle(aTab, aTitle, aOptions) {

I think something went wrong here.

::: browser/base/content/tabbrowser.js
(Diff revision 3)
> -      <!--
> -        `_createLazyBrowser` will define properties on the unbound lazy browser
> -        which correspond to properties defined in XBL which will be bound to
> -        the browser when it is inserted into the document.  If any of these
> -        properties are accessed by consumers, `_insertBrowser` is called and
> -        the browser is inserted to ensure that things don't break.  This list
> -        provides the names of properties that may be called while the browser
> -        is in its unbound (lazy) state.
> -      -->

This seems like important information to preserve.

::: browser/base/content/tabbrowser.js:3741
(Diff revision 3)
> -              this.moveTabToStart();
> -          ]]>
> -        </body>
> -      </method>
> -
> -      <!-- Adopts a tab from another browser window, and inserts it at aIndex -->
> +      this.moveTabToStart();
> +  }
> +  adoptTab(aTab, aIndex, aSelectTab) {
> +    // Swap the dropped tab with a new one we create and then close
> +    // it in the other window (making it seem to have moved between
> +    // windows). We also ensure that the tab we create to swap into has

I think there's value in this comment - maybe let's keep it.

::: browser/base/content/tabbrowser.js
(Diff revision 3)
> -        <parameter name="aTab"/><!-- can be from a different window as well -->
> -        <parameter name="aRestoreTabImmediately"/><!-- can defer loading of the tab contents -->

Let's try to preserve these notes for these arguments.

::: browser/base/content/tabbrowser.js
(Diff revision 3)
> -      <!--
> -        List of browsers whose docshells must be active in order for print preview
> -        to work.
> -      -->

Can this comment be moved to where \_printPreviewBrowsers got moved to?

::: browser/base/content/tabbrowser.js
(Diff revision 3)
> -      <!--
> -        The tab switcher is responsible for asynchronously switching
> -        tabs in e10s. It waits until the new tab is ready (i.e., the
> -        layer tree is available) before switching to it. Then it
> -        unloads the layer tree for the old tab.
> -
> -        The tab switcher is a state machine. For each tab, it
> -        maintains state about whether the layer tree for the tab is
> -        available, being loaded, being unloaded, or unavailable. It
> -        also keeps track of the tab currently being displayed, the tab
> -        it's trying to load, and the tab the user has asked to switch
> -        to. The switcher object is created upon tab switch. It is
> -        released when there are no pending tabs to load or unload.
> -
> -        The following general principles have guided the design:
> -
> -        1. We only request one layer tree at a time. If the user
> -        switches to a different tab while waiting, we don't request
> -        the new layer tree until the old tab has loaded or timed out.
> -
> -        2. If loading the layers for a tab times out, we show the
> -        spinner and possibly request the layer tree for another tab if
> -        the user has requested one.
> -
> -        3. We discard layer trees on a delay. This way, if the user is
> -        switching among the same tabs frequently, we don't continually
> -        load the same tabs.
> -
> -        It's important that we always show either the spinner or a tab
> -        whose layers are available. Otherwise the compositor will draw
> -        an entirely black frame, which is very jarring. To ensure this
> -        never happens when switching away from a tab, we assume the
> -        old tab might still be drawn until a MozAfterPaint event
> -        occurs. Because layout and compositing happen asynchronously,
> -        we don't have any other way of knowing when the switch
> -        actually takes place. Therefore, we don't unload the old tab
> -        until the next MozAfterPaint event.

Yeah, we definitely want to preserve this.

Comment 133

[Brian Grinstead [:bgrins]]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #132)
> ::: browser/base/content/tabbrowser.js:43
> (Diff revision 3)
> > +    XPCOMUtils.defineLazyServiceGetters(this, {
> > +      _unifiedComplete: ["@mozilla.org/autocomplete/search;1?name=unifiedcomplete", "mozIPlacesAutoComplete"],
> > +      serializationHelper: ["@mozilla.org/network/serialization-helper;1", "nsISerializationHelper"],
> > +    });
> > +
> > +    this.mURIFixup = Services.uriFixup;
> 
> If I remember correctly, XBL <field>'s are lazily evaluated, and this here
> is not, meaning that we're going to be kicking off the nsIURIFixup service a
> lot sooner than we did before.
> 
> Is that intentional, or can we re-lazify this?
> 
> And, come to think of it, that kinda applies to all of the fields that are
> being defined here... all of them used to be lazy. Do any of them kick off
> services or anything? If so, can we re-lazify them?

You do remember correctly, and we can definitely re-lazify them if needed. I actually used to have it do all of them lazily but for most of the fields they initialize plain old objects so it didn't really matter and just made things more verbose. At least for mURIFixup I'll put that in a lazyGetter or maybe in the lazyServiceGetters if eslint lets me.

Comment 134

[Brian Grinstead [:bgrins]]

I'll look into preserving comments in the script. I'd prefer to not manually copy them over (especially across rebases) but they are worth saving so I'll do so if needed.

Comment 135

[Brian Grinstead [:bgrins]]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #132)
> ::: browser/base/content/tabbrowser.js:1596
> (Diff revision 3)
> > -          // ensure bool if undefined
> > +    // ensure bool if undefined
> > -          state.camera = !!state.camera;
> > +    state.camera = !!state.camera;
> > -          state.microphone = !!state.microphone;
> > +    state.microphone = !!state.microphone;
> > -          return state;
> > +    return state;
> > -        ]]></body>
> > -      </method>
> > +  }
> > +  setTabTitleLoading(aTab) {} setInitialTabTitle(aTab, aTitle, aOptions) {
> 
> I think something went wrong here.

Huh, this is an empty function telling us to remove it post-57: https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/browser/base/content/tabbrowser.xml#1598-1602. I fixed up the formatting, but we should also just remove it (filed Bug 1440508 for that).

Comment 136

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #134)
> I'll look into preserving comments in the script. I'd prefer to not manually
> copy them over (especially across rebases) but they are worth saving so I'll
> do so if needed.

Potch just added comment support to the xml library the scripts are using, so I think this'll be possible (https://github.com/potch/xmlom/commit/6da304d7735dcb85537dcab22e3c948c13791f9b).

Comment 137

[Brian Grinstead [:bgrins]]

Since the last green try push, a new perma-orange due to a XBL constructor race has shown up on a few tests, like: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7603d53beb2dae50e374283ea8079f4be9c51721&selectedJob=163569107.

I can reproduce the error locally by running this in the Browser Console: `openDialog("chrome://browser/content/browser.xul", "", "chrome,all=no","http://example.com/")`

  <anonymous> chrome://browser/content/tabbrowser.xml:189:9
  tabs_XBL_Constructor chrome://global/content/bindings/tabbox.xml:245:1
  TabBrowser/< chrome://browser/content/tabbrowser.js:34:14
  get resource://gre/modules/XPCOMUtils.jsm:194:21
  _updateNewTabVisibility chrome://browser/content/tabbrowser.js:5213:9
  TabBrowser chrome://browser/content/tabbrowser.js:144:5
  tabbrowser_XBL_Constructor chrome://browser/content/tabbrowser.xml:47:20

What basically happens is during the `gBrowser = new TabBrowser()` call, we end up accessing `this.tabContainer` in `_updateNewTabVisibility`. This triggers the construction of the tabs XBL constructor, which asks for `this.tabbox` (from the `tabbrowser-tabs` field which asks for `this.tabbrowser.tabbox`.  The call to `this.tabbrowser` then asks for `gBrowser`, which isn't yet initialized because we haven't finished the call to `new TabBrowser()`!

I don't yet know why this works today without these patches applied, or why it was working a week ago with them.

Comment 138

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #137)
> I don't yet know why this works today without these patches applied, or why
> it was working a week ago with them.

Stylo-chrome, maybe? It's just a pref (layout.css.servo.chrome.enabled) you can flip off again in about:config, so should be easy to check.

Comment 139

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #137)
> What basically happens is during the `gBrowser = new TabBrowser()` call, we
> end up accessing `this.tabContainer` in `_updateNewTabVisibility`. This
> triggers the construction of the tabs XBL constructor, which asks for
> `this.tabbox` (from the `tabbrowser-tabs` field which asks for
> `this.tabbrowser.tabbox`.  The call to `this.tabbrowser` then asks for
> `gBrowser`, which isn't yet initialized because we haven't finished the call
> to `new TabBrowser()`!

This sounds a bit like bug 1436351 comment 11. Maybe try removing the hack that landed there.

Comment 140

[Brian Grinstead [:bgrins]]

Tried both of the ideas from Comments 138 and 139 and unfortunately neither fixed the issue. For my own reference for looking further into this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53a56a101b5fbbecc4ff3328aca8c3848060ba5b is the last green try push I can find (~1 week ago).

Comment 141

[Brian Grinstead [:bgrins]]

Oh! Somehow I lost this change in part 3.1 which converts "tabbrowser" from a field to a property for the tabbrowser-tabs binding: https://hg.mozilla.org/try/rev/77c350817e61f3d5822fa2a95c9dc1ac37570162#l5.86. Restoring that change locally fixes browser_394759_behavior.js.

Comment 142

[Brian Grinstead [:bgrins]]

Clearing the needinfo since we discussed how to check for the interposition property on IRC and I filed Bug 1440949 to do this

Comment 143

[Brian Grinstead [:bgrins]]

Comment on attachment 8951402 [details]
Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220682/diff/3-4/

Comment 144

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220684/diff/3-4/

Comment 145

[Brian Grinstead [:bgrins]]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220686/diff/3-4/

Comment 146

[Brian Grinstead [:bgrins]]

Comment on attachment 8951405 [details]
Bug 1392352 - Part 2.2 - To fold: remove build instructions and reorganize things a bit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220688/diff/3-4/

Comment 147

[Brian Grinstead [:bgrins]]

Comment on attachment 8951406 [details]
Bug 1392352 - Part 2.3 - To fold: delete the old XBL implementation of tabbrowser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220690/diff/3-4/

Comment 148

[Brian Grinstead [:bgrins]]

Posted a new version:

- Removed the addon interposition patch (moved to bug 1440949).
- Migrated XML comments for methods / fields / properties to jsdoc style comments.
- Formatting changes. I've tried to make as many as possible in the script itself so it will apply to future bindings, but it's hard to capture things like parameter alignment in function calls (which is sometimes inconsistent within the file). The manual steps I'm taking can be seen at https://reviewboard-hg.mozilla.org/gecko/rev/81f0afefefb5#l1.106. Let me know if I've missed something. Hopefully we can find a way to enforce this formatting with eslint so there's no ambiguity - but I definitely don't want to pull that into this bug :).

Comment 149

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220684/#review229116

Just some minor things, but I think this works for me. Thanks for your work!

Oh, we might also want to send mail to firefox-dev about this change.

::: browser/base/content/tabbrowser.js:197
(Diff revision 4)
> +    /**
> +     * The tab switcher is responsible for asynchronously switching
> +     * tabs in e10s. It waits until the new tab is ready (i.e., the
> +     * layer tree is available) before switching to it. Then it
> +     * unloads the layer tree for the old tab.
> +     *
> +     * The tab switcher is a state machine. For each tab, it
> +     * maintains state about whether the layer tree for the tab is
> +     * available, being loaded, being unloaded, or unavailable. It
> +     * also keeps track of the tab currently being displayed, the tab
> +     * it's trying to load, and the tab the user has asked to switch
> +     * to. The switcher object is created upon tab switch. It is
> +     * released when there are no pending tabs to load or unload.
> +     *
> +     * The following general principles have guided the design:
> +     *
> +     * 1. We only request one layer tree at a time. If the user
> +     * switches to a different tab while waiting, we don't request
> +     * the new layer tree until the old tab has loaded or timed out.
> +     *
> +     * 2. If loading the layers for a tab times out, we show the
> +     * spinner and possibly request the layer tree for another tab if
> +     * the user has requested one.
> +     *
> +     * 3. We discard layer trees on a delay. This way, if the user is
> +     * switching among the same tabs frequently, we don't continually
> +     * load the same tabs.
> +     *
> +     * It's important that we always show either the spinner or a tab
> +     * whose layers are available. Otherwise the compositor will draw
> +     * an entirely black frame, which is very jarring. To ensure this
> +     * never happens when switching away from a tab, we assume the
> +     * old tab might still be drawn until a MozAfterPaint event
> +     * occurs. Because layout and compositing happen asynchronously,
> +     * we don't have any other way of knowing when the switch
> +     * actually takes place. Therefore, we don't unload the old tab
> +     * until the next MozAfterPaint event.
> +     */

I'd say this makes more sense down near where the switcher is implemented, near \_getSwitcher().

::: browser/base/content/tabbrowser.js:299
(Diff revision 4)
> +    let messageManager = window.getGroupMessageManager("browsers");
> +
> +    let remote = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIWebNavigation)
> +                       .QueryInterface(Ci.nsILoadContext)
> +      .useRemoteTabs;

Busted indentation

::: browser/base/content/tabbrowser.js:5114
(Diff revision 4)
> -      </method>
> +  goBack() {
> -
> -      <!-- BEGIN FORWARDED BROWSER PROPERTIES.  IF YOU ADD A PROPERTY TO THE BROWSER ELEMENT
> -           MAKE SURE TO ADD IT HERE AS WELL. -->
> -      <property name="canGoBack"
> -                onget="return this.mCurrentBrowser.canGoBack;"
> -                readonly="true"/>
> -
> -      <property name="canGoForward"
> -                onget="return this.mCurrentBrowser.canGoForward;"
> -                readonly="true"/>
> -
> -      <method name="goBack">
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.goBack();
> +    return this.mCurrentBrowser.goBack();
> -          ]]>
> -        </body>
> -      </method>
> +  }
> +
> +  goForward() {
> -
> -      <method name="goForward">
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.goForward();
> +    return this.mCurrentBrowser.goForward();
> -          ]]>
> -        </body>
> -      </method>
> +  }
> +
> +  reload() {
> -
> -      <method name="reload">
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.reload();
> +    return this.mCurrentBrowser.reload();
> -          ]]>
> -        </body>
> -      </method>
> +  }
> +
> +  reloadWithFlags(aFlags) {
> -
> -      <method name="reloadWithFlags">
> -        <parameter name="aFlags"/>
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.reloadWithFlags(aFlags);
> +    return this.mCurrentBrowser.reloadWithFlags(aFlags);
> -          ]]>
> -        </body>
> -      </method>
> +  }
> +
> +  stop() {
> -
> -      <method name="stop">
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.stop();
> +    return this.mCurrentBrowser.stop();
> -          ]]>
> -        </body>
> -      </method>
> -
> -      <!-- throws exception for unknown schemes -->
> -      <method name="loadURI">
> +  }
> +
> +  /**
> +   * throws exception for unknown schemes
> +   */
> +  loadURI(aURI, aReferrerURI, aCharset) {
> -        <parameter name="aURI"/>
> -        <parameter name="aReferrerURI"/>
> -        <parameter name="aCharset"/>
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.loadURI(aURI, aReferrerURI, aCharset);
> +    return this.mCurrentBrowser.loadURI(aURI, aReferrerURI, aCharset);
> -          ]]>
> -        </body>
> -      </method>
> -
> -      <!-- throws exception for unknown schemes -->
> -      <method name="loadURIWithFlags">
> +  }
> +
> +  /**
> +   * throws exception for unknown schemes
> +   */
> +  loadURIWithFlags(aURI, aFlags, aReferrerURI, aCharset, aPostData) {
> -        <parameter name="aURI"/>
> -        <parameter name="aFlags"/>
> -        <parameter name="aReferrerURI"/>
> -        <parameter name="aCharset"/>
> -        <parameter name="aPostData"/>
> -        <body>
> -          <![CDATA[
> -            // Note - the callee understands both:
> +    // Note - the callee understands both:
> -            // (a) loadURIWithFlags(aURI, aFlags, ...)
> +    // (a) loadURIWithFlags(aURI, aFlags, ...)
> -            // (b) loadURIWithFlags(aURI, { flags: aFlags, ... })
> +    // (b) loadURIWithFlags(aURI, { flags: aFlags, ... })
> -            // Forwarding it as (a) here actually supports both (a) and (b),
> +    // Forwarding it as (a) here actually supports both (a) and (b),
> -            // so you can call us either way too.
> +    // so you can call us either way too.
> -            return this.mCurrentBrowser.loadURIWithFlags(aURI, aFlags, aReferrerURI, aCharset, aPostData);
> +    return this.mCurrentBrowser.loadURIWithFlags(aURI, aFlags, aReferrerURI, aCharset, aPostData);
> -          ]]>
> -        </body>
> -      </method>
> +  }
> +
> +  goHome() {
> -
> -      <method name="goHome">
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.goHome();
> +    return this.mCurrentBrowser.goHome();
> -          ]]>
> -        </body>
> -      </method>
> +  }
> +
> +  gotoIndex(aIndex) {
> -
> -      <property name="homePage">
> -        <getter>
> -          <![CDATA[
> -            return this.mCurrentBrowser.homePage;
> -          ]]>
> -        </getter>
> -        <setter>
> -          <![CDATA[
> -            this.mCurrentBrowser.homePage = val;
> -            return val;
> -          ]]>
> -        </setter>
> -      </property>
> -
> -      <method name="gotoIndex">
> -        <parameter name="aIndex"/>
> -        <body>
> -          <![CDATA[
> -            return this.mCurrentBrowser.gotoIndex(aIndex);
> +    return this.mCurrentBrowser.gotoIndex(aIndex);
> -          ]]>
> +  }

These should probably go up nearer the top of the file where the other forwarded functions (and the big warning message about forwarded functions) are.

Comment 150

[Brian Grinstead [:bgrins]]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #149)
> Oh, we might also want to send mail to firefox-dev about this change.

I'll add something to the agenda for the desktop meeting tomorrow and will also send something out to the list before we land anything.

By the way, I was wondering if we should try to land this week or wait until after the merge in two weeks (on 3-12). As I see it, the pro to waiting is that this would have a full cycle in nightly before going to beta. The cons are that:
1) it'll require another set of rebases
2) uplifts to beta for tabbrowser will be more of a pain (since the change in m-c would affect the JS class and would need to be ported to XBL).

I don't mind doing another rebase after the merge, so the main thing to balance would be the extra time on nightly vs the extra uplift pain.

Comment 151

[:Gijs (out for now; he/him)]

Doing this sooner rather than later seems like a win. It's very unlikely for us to still hit issues now that are severe enough that we'd still want to uplift to 52esr, and 59 has also almost shipped at this point, so landing this on nightly for this esr to avoid uplift issues with less severe issues for the 60 and 60esr cycle seems like the best course of action. 3 weeks on nightly will shake out critical functional issues that managed to get past automated testing, so I'm not too worried about baking time for these patches.

Comment 152

[Mike Conley (:mconley) (:⚙️)]

I agree - especially regarding ESR 60. Let's try to get this in before the freeze.

Comment 153

[Brian Grinstead [:bgrins]]

Comment on attachment 8951402 [details]
Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220682/diff/4-5/

Comment 154

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220684/diff/4-5/

Comment 155

[Brian Grinstead [:bgrins]]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220686/diff/4-5/

Comment 156

[Brian Grinstead [:bgrins]]

Comment on attachment 8951405 [details]
Bug 1392352 - Part 2.2 - To fold: remove build instructions and reorganize things a bit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220688/diff/4-5/

Comment 157

[Brian Grinstead [:bgrins]]

Comment on attachment 8951406 [details]
Bug 1392352 - Part 2.3 - To fold: delete the old XBL implementation of tabbrowser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220690/diff/4-5/

Comment 158

[Brian Grinstead [:bgrins]]

Rebased, and issues from Comment 149 addressed

Comment 159

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #152)
> I agree - especially regarding ESR 60. Let's try to get this in before the
> freeze.

+1

Comment 160

[Brian Grinstead [:bgrins]]

OK, looks like I need to do another rebase before attempting to land. I'll wait for your review before doing that in the hopes of only doing one more rebase, since it takes a while.

Comment 161

[Dão Gottwald [:dao]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

https://reviewboard.mozilla.org/r/220684/#review229580

::: browser/base/content/tabbrowser.js:853
(Diff revision 5)
> -                return false;
> +          return false;
>  
> -              // Don't show progress indicators in tabs for about: URIs
> +        // Don't show progress indicators in tabs for about: URIs
> -              // pointing to local resources.
> +        // pointing to local resources.
> -              if ((aRequest instanceof Ci.nsIChannel) &&
> +        if ((aRequest instanceof Ci.nsIChannel) &&
> -                  this.mTabBrowser._isLocalAboutURI(aRequest.originalURI, aRequest.URI)) {
> +          this.mTabBrowser._isLocalAboutURI(aRequest.originalURI, aRequest.URI)) {

indentation

::: browser/base/content/tabbrowser.js:870
(Diff revision 5)
> -              // If the state has STATE_STOP, and no requests were in flight, then this
> +        // If the state has STATE_STOP, and no requests were in flight, then this
> -              // must be the initial "stop" for the initial about:blank document.
> +        // must be the initial "stop" for the initial about:blank document.
> -              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
> +        const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
> -              if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
> +        if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
> -                  this.mRequestCount == 0 &&
> +          this.mRequestCount == 0 &&
> -                  !aLocation) {
> +          !aLocation) {

indentation

::: browser/base/content/tabbrowser.js:880
(Diff revision 5)
> -              return location == "about:blank";
> +        return location == "about:blank";
> -            },
> +      },
>  
> -            onProgressChange(aWebProgress, aRequest,
> +      onProgressChange(aWebProgress, aRequest,
> -                             aCurSelfProgress, aMaxSelfProgress,
> +        aCurSelfProgress, aMaxSelfProgress,
> -                             aCurTotalProgress, aMaxTotalProgress) {
> +        aCurTotalProgress, aMaxTotalProgress) {

indentation

::: browser/base/content/tabbrowser.js:897
(Diff revision 5)
> -                                           aCurTotalProgress, aMaxTotalProgress]);
> +                                     aCurTotalProgress, aMaxTotalProgress]);
> -            },
> +      },
>  
> -            onProgressChange64(aWebProgress, aRequest,
> +      onProgressChange64(aWebProgress, aRequest,
> -                               aCurSelfProgress, aMaxSelfProgress,
> +        aCurSelfProgress, aMaxSelfProgress,
> -                               aCurTotalProgress, aMaxTotalProgress) {
> +        aCurTotalProgress, aMaxTotalProgress) {

indentation

::: browser/base/content/tabbrowser.js:927
(Diff revision 5)
> -              // from here forward. Similarly, if we conclude that this state change
> +        // from here forward. Similarly, if we conclude that this state change
> -              // is one that we shouldn't be ignoring, then stop ignoring.
> +        // is one that we shouldn't be ignoring, then stop ignoring.
> -              if ((ignoreBlank &&
> +        if ((ignoreBlank &&
> -                   aStateFlags & nsIWebProgressListener.STATE_STOP &&
> +            aStateFlags & nsIWebProgressListener.STATE_STOP &&
> -                   aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) ||
> +            aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) ||
> -                  !ignoreBlank && this.mBlank) {
> +          !ignoreBlank && this.mBlank) {

indentation

::: browser/base/content/tabbrowser.js:946
(Diff revision 5)
> -                // the count of open requests should now be 0
> +          // the count of open requests should now be 0
> -                this.mRequestCount = 0;
> +          this.mRequestCount = 0;
> -              }
> +        }
>  
>          if (aStateFlags & nsIWebProgressListener.STATE_START &&
>            aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {

indentation

::: browser/base/content/tabbrowser.js:954
(Diff revision 5)
>              // like about:home and about:privatebrowsing arrive with nsIRequest
>              // pointing to their resolved jar: or file: URIs.
>              if (!(originalLocation && gInitialPages.includes(originalLocation.spec) &&
>                  originalLocation != "about:blank" &&
>                  this.mBrowser.initialPageLoadedFromURLBar != originalLocation.spec &&
>                  this.mBrowser.currentURI && this.mBrowser.currentURI.spec == "about:blank")) {

indentation

::: browser/base/content/tabbrowser.js:975
(Diff revision 5)
>              this.mTab.removeAttribute("crashed");
>            }
>  
> -                if (this._shouldShowProgress(aRequest)) {
> +          if (this._shouldShowProgress(aRequest)) {
> -                  if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING) &&
> +            if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING) &&
> -                      aWebProgress && aWebProgress.isTopLevel) {
> +              aWebProgress && aWebProgress.isTopLevel) {

indentation

::: browser/base/content/tabbrowser.js:988
(Diff revision 5)
> -                        gBrowser.tabContainer._schedulePressureCount = gBrowser.schedulePressureDefaultCount;
> +                  gBrowser.tabContainer._schedulePressureCount = gBrowser.schedulePressureDefaultCount;
> -                        gBrowser.tabContainer.setAttribute("schedulepressure", "true");
> +                  gBrowser.tabContainer.setAttribute("schedulepressure", "true");
> -                      },
> +                },
> -                      lowPressureFn() {
> +                lowPressureFn() {
> -                        if (!gBrowser.tabContainer._schedulePressureCount ||
> +                  if (!gBrowser.tabContainer._schedulePressureCount ||
> -                            --gBrowser.tabContainer._schedulePressureCount <= 0) {
> +                    --gBrowser.tabContainer._schedulePressureCount <= 0) {

indentation

::: browser/base/content/tabbrowser.js:1012
(Diff revision 5)
> -                  if (this.mTab.selected) {
> +            if (this.mTab.selected) {
> -                    this.mTabBrowser.mIsBusy = true;
> +              this.mTabBrowser.mIsBusy = true;
> -                  }
> +            }
> -                }
> +          }
> -              } else if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
> +        } else if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
> -                         aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {
> +          aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {

indentation

::: browser/base/content/tabbrowser.js:1026
(Diff revision 5)
> -                  // Only animate the "burst" indicating the page has loaded if
> +            // Only animate the "burst" indicating the page has loaded if
> -                  // the top-level page is the one that finished loading.
> +            // the top-level page is the one that finished loading.
> -                  if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> +            if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> -                      Components.isSuccessCode(aStatus) &&
> +              Components.isSuccessCode(aStatus) &&
> -                      !this.mTabBrowser.tabAnimationsInProgress &&
> +              !this.mTabBrowser.tabAnimationsInProgress &&
> -                      Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> +              Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

indentation

::: browser/base/content/tabbrowser.js:1067
(Diff revision 5)
> -                    // since these pages get their favicon set in browser code to
> +              // since these pages get their favicon set in browser code to
> -                    // improve perceived performance.
> +              // improve perceived performance.
> -                    let isNewTab = originalLocation &&
> +              let isNewTab = originalLocation &&
> -                      (originalLocation.spec == "about:newtab" ||
> +                (originalLocation.spec == "about:newtab" ||
> -                       originalLocation.spec == "about:privatebrowsing" ||
> +                  originalLocation.spec == "about:privatebrowsing" ||
> -                       originalLocation.spec == "about:home");
> +                  originalLocation.spec == "about:home");

indentation(?)

::: browser/base/content/tabbrowser.js:1097
(Diff revision 5)
> -              this._callProgressListeners("onStateChange",
> +        this._callProgressListeners("onStateChange",
> -                                          [aWebProgress, aRequest, aStateFlags, aStatus],
> +                                    [aWebProgress, aRequest, aStateFlags, aStatus],
> -                                          false);
> +                                    false);
>  
> -              if (aStateFlags & (nsIWebProgressListener.STATE_START |
> +        if (aStateFlags & (nsIWebProgressListener.STATE_START |
> -                                 nsIWebProgressListener.STATE_STOP)) {
> +            nsIWebProgressListener.STATE_STOP)) {

indentation (maybe move everthing after & to the next line)

::: browser/base/content/tabbrowser.js:1108
(Diff revision 5)
> -              this.mStatus = aStatus;
> +        this.mStatus = aStatus;
> -            },
> +      },
> -            /* eslint-enable complexity */
> +      /* eslint-enable complexity */
>  
> -            onLocationChange(aWebProgress, aRequest, aLocation,
> +      onLocationChange(aWebProgress, aRequest, aLocation,
> -                             aFlags) {
> +        aFlags) {

indentation

::: browser/base/content/tabbrowser.js:1125
(Diff revision 5)
> -                // Another reason to clear the userTypedValue is if this was an anchor
> +          // Another reason to clear the userTypedValue is if this was an anchor
> -                // navigation initiated by the user.
> +          // navigation initiated by the user.
> -                if (this.mBrowser.didStartLoadSinceLastUserTyping() ||
> +          if (this.mBrowser.didStartLoadSinceLastUserTyping() ||
> -                    ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
> +            ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
> -                     aLocation.spec != "about:blank") ||
> +              aLocation.spec != "about:blank") ||
> -                    (isSameDocument && this.mBrowser.inLoadURI)) {
> +            (isSameDocument && this.mBrowser.inLoadURI)) {

indentation

::: browser/base/content/tabbrowser.js:1136
(Diff revision 5)
> -                // the load results in an error page, it's possible that there
> +          // the load results in an error page, it's possible that there
> -                // isn't any (STATE_IS_NETWORK & STATE_STOP) state to cause busy
> +          // isn't any (STATE_IS_NETWORK & STATE_STOP) state to cause busy
> -                // attribute being removed. In this case we should remove the
> +          // attribute being removed. In this case we should remove the
> -                // attribute here.
> +          // attribute here.
> -                if ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
> +          if ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
> -                    this.mTab.hasAttribute("busy")) {
> +            this.mTab.hasAttribute("busy")) {

indentation

::: browser/base/content/tabbrowser.js:1172
(Diff revision 5)
> -                // though we're pointed at an about:blank. Also don't clear it
> +          // though we're pointed at an about:blank. Also don't clear it
> -                // if onLocationChange was triggered by a pushState or a
> +          // if onLocationChange was triggered by a pushState or a
> -                // replaceState (bug 550565) or a hash change (bug 408415).
> +          // replaceState (bug 550565) or a hash change (bug 408415).
> -                if (!this.mTab.hasAttribute("pending") &&
> +          if (!this.mTab.hasAttribute("pending") &&
> -                    aWebProgress.isLoadingDocument &&
> +            aWebProgress.isLoadingDocument &&
> -                    !isSameDocument) {
> +            !isSameDocument) {

indentation

::: browser/base/content/tabbrowser.js:1229
(Diff revision 5)
>  
> -            QueryInterface(aIID) {
> +      QueryInterface(aIID) {
> -              if (aIID.equals(Components.interfaces.nsIWebProgressListener) ||
> +        if (aIID.equals(Components.interfaces.nsIWebProgressListener) ||
> -                  aIID.equals(Components.interfaces.nsIWebProgressListener2) ||
> +          aIID.equals(Components.interfaces.nsIWebProgressListener2) ||
> -                  aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
> +          aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
> -                  aIID.equals(Components.interfaces.nsISupports))
> +          aIID.equals(Components.interfaces.nsISupports))

indentation

::: browser/base/content/tabbrowser.js:1364
(Diff revision 5)
> -            // add the scheme and host to the title to prevent spoofing.
> +    // add the scheme and host to the title to prevent spoofing.
> -            // XXX https://bugzilla.mozilla.org/show_bug.cgi?id=22183#c239
> +    // XXX https://bugzilla.mozilla.org/show_bug.cgi?id=22183#c239
> -            try {
> +    try {
> -              if (docElement.getAttribute("chromehidden").includes("location")) {
> +      if (docElement.getAttribute("chromehidden").includes("location")) {
> -                var uri = this.mURIFixup.createExposableURI(
> +        var uri = this.mURIFixup.createExposableURI(
> -                            aBrowser.currentURI);
> +          aBrowser.currentURI);

probably fits on one line

::: browser/base/content/tabbrowser.js:1439
(Diff revision 5)
> -                 !window.isFullyOccluded);
> +          !window.isFullyOccluded);
> -            }
> +    }
>  
> -            var updateBlockedPopups = false;
> +    var updateBlockedPopups = false;
> -            if ((oldBrowser.blockedPopups && !newBrowser.blockedPopups) ||
> +    if ((oldBrowser.blockedPopups && !newBrowser.blockedPopups) ||
> -                (!oldBrowser.blockedPopups && newBrowser.blockedPopups))
> +      (!oldBrowser.blockedPopups && newBrowser.blockedPopups))

indentation

::: browser/base/content/tabbrowser.js:1478
(Diff revision 5)
>  
> -            var listener = this._tabListeners.get(this.mCurrentTab);
> +    var listener = this._tabListeners.get(this.mCurrentTab);
> -            if (listener && listener.mStateFlags) {
> +    if (listener && listener.mStateFlags) {
> -              this._callProgressListeners(null, "onUpdateCurrentBrowser",
> +      this._callProgressListeners(null, "onUpdateCurrentBrowser",
> -                                          [listener.mStateFlags, listener.mStatus,
> +                                  [listener.mStateFlags, listener.mStatus,
> -                                           listener.mMessage, listener.mTotalProgress],
> +                                  listener.mMessage, listener.mTotalProgress],

indentation

::: browser/base/content/tabbrowser.js:1490
(Diff revision 5)
> -              oldTab.updateLastAccessed();
> +      oldTab.updateLastAccessed();
>  
> -              let oldFindBar = oldTab._findBar;
> +      let oldFindBar = oldTab._findBar;
> -              if (oldFindBar &&
> +      if (oldFindBar &&
> -                  oldFindBar.findMode == oldFindBar.FIND_NORMAL &&
> +        oldFindBar.findMode == oldFindBar.FIND_NORMAL &&
> -                  !oldFindBar.hidden)
> +        !oldFindBar.hidden)

indentation

::: browser/base/content/tabbrowser.js:1515
(Diff revision 5)
> -            if (this.mCurrentTab.hasAttribute("busy") && !this.mIsBusy) {
> +    if (this.mCurrentTab.hasAttribute("busy") && !this.mIsBusy) {
> -              this.mIsBusy = true;
> +      this.mIsBusy = true;
> -              this._callProgressListeners(null, "onStateChange",
> +      this._callProgressListeners(null, "onStateChange",
> -                                          [webProgress, null,
> +                                  [webProgress, null,
> -                                           nsIWebProgressListener.STATE_START |
> +                                  nsIWebProgressListener.STATE_START |
> -                                           nsIWebProgressListener.STATE_IS_NETWORK, 0],
> +                                  nsIWebProgressListener.STATE_IS_NETWORK, 0],

indentation

::: browser/base/content/tabbrowser.js:1526
(Diff revision 5)
> -            if (!this.mCurrentTab.hasAttribute("busy") && this.mIsBusy) {
> +    if (!this.mCurrentTab.hasAttribute("busy") && this.mIsBusy) {
> -              this.mIsBusy = false;
> +      this.mIsBusy = false;
> -              this._callProgressListeners(null, "onStateChange",
> +      this._callProgressListeners(null, "onStateChange",
> -                                          [webProgress, null,
> +                                  [webProgress, null,
> -                                           nsIWebProgressListener.STATE_STOP |
> +                                  nsIWebProgressListener.STATE_STOP |
> -                                           nsIWebProgressListener.STATE_IS_NETWORK, 0],
> +                                  nsIWebProgressListener.STATE_IS_NETWORK, 0],

indentation

::: browser/base/content/tabbrowser.js:1672
(Diff revision 5)
> -          if (gURLBar.focused && newBrowser.userTypedValue) {
> +      if (gURLBar.focused && newBrowser.userTypedValue) {
> -            return;
> +        return;
> -          }
> +      }
>  
> -          if (!window.fullScreen || isTabEmpty(newTab)) {
> +      if (!window.fullScreen || isTabEmpty(newTab)) {
> -            focusAndSelectUrlBar();
> +        focusAndSelectUrlBar();

indentation

::: browser/base/content/tabbrowser.js:1679
(Diff revision 5)
> -          }
> +      }
> -        }
> +    }
>  
> -        // Focus the find bar if it was previously focused for that tab.
> +    // Focus the find bar if it was previously focused for that tab.
> -        if (gFindBarInitialized && !gFindBar.hidden &&
> +    if (gFindBarInitialized && !gFindBar.hidden &&
> -            this.selectedTab._findBarFocused) {
> +      this.selectedTab._findBarFocused) {

indentation

::: browser/base/content/tabbrowser.js:1687
(Diff revision 5)
> -        }
> +    }
>  
> -        // Don't focus the content area if something has been focused after the
> +    // Don't focus the content area if something has been focused after the
> -        // tab switch was initiated.
> +    // tab switch was initiated.
> -        if (gMultiProcessBrowser &&
> +    if (gMultiProcessBrowser &&
> -            document.activeElement != document.documentElement)
> +      document.activeElement != document.documentElement)

indentation

::: browser/base/content/tabbrowser.js:1885
(Diff revision 5)
>      var aNextTabParentId;
>      var aFocusUrlBar;
>      var aName;
>      if (arguments.length == 2 &&
>        typeof arguments[1] == "object" &&
>        !(arguments[1] instanceof Ci.nsIURI)) {

indentation

::: browser/base/content/tabbrowser.js:2073
(Diff revision 5)
> -            }
> +    }
>  
> -            // Abort if we're not going to change anything
> +    // Abort if we're not going to change anything
> -            let currentRemoteType = aBrowser.getAttribute("remoteType");
> +    let currentRemoteType = aBrowser.getAttribute("remoteType");
> -            if (isRemote == aShouldBeRemote && !aOptions.newFrameloader &&
> +    if (isRemote == aShouldBeRemote && !aOptions.newFrameloader &&
> -                (!isRemote || currentRemoteType == aOptions.remoteType)) {
> +      (!isRemote || currentRemoteType == aOptions.remoteType)) {

indentation

::: browser/base/content/tabbrowser.js:2281
(Diff revision 5)
> -              return;
> +      return;
> -            }
> +    }
>  
> -            let remoteType =
> +    let remoteType =
> -              E10SUtils.getRemoteTypeForURI(BROWSER_NEW_TAB_URL,
> +      E10SUtils.getRemoteTypeForURI(BROWSER_NEW_TAB_URL,
> -                                            gMultiProcessBrowser);
> +        gMultiProcessBrowser);

might fit on one line

::: browser/base/content/tabbrowser.js:2405
(Diff revision 5)
> -            browserContainer.appendChild(stack);
> +    browserContainer.appendChild(stack);
> -            browserContainer.setAttribute("flex", "1");
> +    browserContainer.setAttribute("flex", "1");
>  
> -            // Create the sidebar container
> +    // Create the sidebar container
> -            var browserSidebarContainer = document.createElementNS(NS_XUL,
> +    var browserSidebarContainer = document.createElementNS(NS_XUL,
> -                                                                   "hbox");
> +      "hbox");

ditto

::: browser/base/content/tabbrowser.js:2412
(Diff revision 5)
> -            browserSidebarContainer.appendChild(browserContainer);
> +    browserSidebarContainer.appendChild(browserContainer);
> -            browserSidebarContainer.setAttribute("flex", "1");
> +    browserSidebarContainer.setAttribute("flex", "1");
>  
> -            // Add the Message and the Browser to the box
> +    // Add the Message and the Browser to the box
> -            var notificationbox = document.createElementNS(NS_XUL,
> +    var notificationbox = document.createElementNS(NS_XUL,
> -                                                           "notificationbox");
> +      "notificationbox");

ditto

::: browser/base/content/tabbrowser.js:2538
(Diff revision 5)
> -                delete browser[name];
> +        delete browser[name];
> -              }
> +      }
> -            }
> +    }
>  
> -            let { uriIsAboutBlank, remoteType, usingPreloadedContent } =
> +    let { uriIsAboutBlank, remoteType, usingPreloadedContent } =
> -                    aTab._browserParams;
> +    aTab._browserParams;

indentation (might fit on one line too)

::: browser/base/content/tabbrowser.js:2597
(Diff revision 5)
> -                tab.selected ||
> +      tab.selected ||
> -                tab.closing ||
> +      tab.closing ||
> -                this._windowIsClosing ||
> +      this._windowIsClosing ||
> -                !aBrowser.isConnected ||
> +      !aBrowser.isConnected ||
> -                !aBrowser.isRemoteBrowser ||
> +      !aBrowser.isRemoteBrowser ||
> -                !aBrowser.permitUnload(permitUnloadFlags).permitUnload) {
> +      !aBrowser.permitUnload(permitUnloadFlags).permitUnload) {

indentation

::: browser/base/content/tabbrowser.js:2671
(Diff revision 5)
>      var aNoInitialLabel;
>      var aFocusUrlBar;
>      var aName;
>      if (arguments.length == 2 &&
>        typeof arguments[1] == "object" &&
>        !(arguments[1] instanceof Ci.nsIURI)) {

indentation

::: browser/base/content/tabbrowser.js:2913
(Diff revision 5)
> -            if (!usingPreloadedContent && aOriginPrincipal && aURI) {
> +    if (!usingPreloadedContent && aOriginPrincipal && aURI) {
> -              let {URI_INHERITS_SECURITY_CONTEXT} = Ci.nsIProtocolHandler;
> +      let { URI_INHERITS_SECURITY_CONTEXT } = Ci.nsIProtocolHandler;
> -              // Unless we know for sure we're not inheriting principals,
> +      // Unless we know for sure we're not inheriting principals,
> -              // force the about:blank viewer to have the right principal:
> +      // force the about:blank viewer to have the right principal:
> -              if (!aURIObject ||
> +      if (!aURIObject ||
> -                  (doGetProtocolFlags(aURIObject) & URI_INHERITS_SECURITY_CONTEXT)) {
> +        (doGetProtocolFlags(aURIObject) & URI_INHERITS_SECURITY_CONTEXT)) {

indentation

::: browser/base/content/tabbrowser.js:2954
(Diff revision 5)
>      }
>  
> -            // If we're opening a tab related to the an existing tab, move it
> +    // If we're opening a tab related to the an existing tab, move it
> -            // to a position after that tab.
> +    // to a position after that tab.
> -            if (openerTab &&
> +    if (openerTab &&
> -                Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {
> +      Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {

indentation

::: browser/base/content/tabbrowser.js:3116
(Diff revision 5)
> -            }
> +    }
>  
> -            // Telemetry stopwatches may already be running if removeTab gets
> +    // Telemetry stopwatches may already be running if removeTab gets
> -            // called again for an already closing tab.
> +    // called again for an already closing tab.
> -            if (!TelemetryStopwatch.running("FX_TAB_CLOSE_TIME_ANIM_MS", aTab) &&
> +    if (!TelemetryStopwatch.running("FX_TAB_CLOSE_TIME_ANIM_MS", aTab) &&
> -                !TelemetryStopwatch.running("FX_TAB_CLOSE_TIME_NO_ANIM_MS", aTab)) {
> +      !TelemetryStopwatch.running("FX_TAB_CLOSE_TIME_NO_ANIM_MS", aTab)) {

indentation

::: browser/base/content/tabbrowser.js:3127
(Diff revision 5)
> -            window.maybeRecordAbandonmentTelemetry(aTab, "tabClosed");
> +    window.maybeRecordAbandonmentTelemetry(aTab, "tabClosed");
>  
> -            // Handle requests for synchronously removing an already
> +    // Handle requests for synchronously removing an already
> -            // asynchronously closing tab.
> +    // asynchronously closing tab.
> -            if (!animate &&
> +    if (!animate &&
> -                aTab.closing) {
> +      aTab.closing) {

indentation

::: browser/base/content/tabbrowser.js:3152
(Diff revision 5)
> -                aTab.pinned ||
> +      aTab.pinned ||
> -                aTab.hidden ||
> +      aTab.hidden ||
> -                this._removingTabs.length > 3 /* don't want lots of concurrent animations */ ||
> +      this._removingTabs.length > 3 /* don't want lots of concurrent animations */ ||
> -                aTab.getAttribute("fadein") != "true" /* fade-in transition hasn't been triggered yet */ ||
> +      aTab.getAttribute("fadein") != "true" /* fade-in transition hasn't been triggered yet */ ||
> -                window.getComputedStyle(aTab).maxWidth == "0.1px" /* fade-in transition hasn't moved yet */ ||
> +      window.getComputedStyle(aTab).maxWidth == "0.1px" /* fade-in transition hasn't moved yet */ ||
> -                !this.animationsEnabled) {
> +      !this.animationsEnabled) {

indentation

::: browser/base/content/tabbrowser.js:3168
(Diff revision 5)
> -            aTab.removeAttribute("fadein");
> +    aTab.removeAttribute("fadein");
> -            aTab.removeAttribute("bursting");
> +    aTab.removeAttribute("bursting");
>  
> -            setTimeout(function(tab, tabbrowser) {
> +    setTimeout(function(tab, tabbrowser) {
> -              if (tab.parentNode &&
> +      if (tab.parentNode &&
> -                  window.getComputedStyle(tab).maxWidth == "0.1px") {
> +        window.getComputedStyle(tab).maxWidth == "0.1px") {

indentation

::: browser/base/content/tabbrowser.js:3229
(Diff revision 5)
>  
> -              // Closing the tab and replacing it with a blank one is notably slower
> +      // Closing the tab and replacing it with a blank one is notably slower
> -              // than closing the window right away. If the caller opts in, take
> +      // than closing the window right away. If the caller opts in, take
> -              // the fast path.
> +      // the fast path.
> -              if (closeWindow &&
> +      if (closeWindow &&
> -                  aCloseWindowFastpath &&
> +        aCloseWindowFastpath &&

indentation

::: browser/base/content/tabbrowser.js:3450
(Diff revision 5)
> -            }
> +    }
>  
> -            if (aTab.owner &&
> +    if (aTab.owner &&
> -                !aTab.owner.hidden &&
> +      !aTab.owner.hidden &&
> -                !aTab.owner.closing &&
> +      !aTab.owner.closing &&
> -                Services.prefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {
> +      Services.prefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {

indentation

::: browser/base/content/tabbrowser.js:3488
(Diff revision 5)
> -        <body>
> -          <![CDATA[
> -            // Do not allow transfering a private tab to a non-private window
> +    // Do not allow transfering a private tab to a non-private window
> -            // and vice versa.
> +    // and vice versa.
> -            if (PrivateBrowsingUtils.isWindowPrivate(window) !=
> +    if (PrivateBrowsingUtils.isWindowPrivate(window) !=
> -                PrivateBrowsingUtils.isWindowPrivate(aOtherTab.ownerGlobal))
> +      PrivateBrowsingUtils.isWindowPrivate(aOtherTab.ownerGlobal))

indentation

::: browser/base/content/tabbrowser.js:3807
(Diff revision 5)
> -        <parameter name="aTab"/>
> -        <parameter name="aSource"/>
> -        <body>
> -        <![CDATA[
> -          if (!aTab.hidden && !aTab.pinned && !aTab.selected &&
> +    if (!aTab.hidden && !aTab.pinned && !aTab.selected &&
> -              !aTab.closing && !aTab._sharingState) {
> +      !aTab.closing && !aTab._sharingState) {

indentation

::: browser/base/content/tabbrowser.js:4028
(Diff revision 5)
> -        <parameter name="aEvent"/>
> -        <body>
> -          <![CDATA[
> -            var direction = window.getComputedStyle(this.parentNode).direction;
> -            if ((direction == "ltr" && aEvent.keyCode == KeyEvent.DOM_VK_RIGHT) ||
> +    if ((direction == "ltr" && aEvent.keyCode == KeyEvent.DOM_VK_RIGHT) ||
> -                (direction == "rtl" && aEvent.keyCode == KeyEvent.DOM_VK_LEFT))
> +      (direction == "rtl" && aEvent.keyCode == KeyEvent.DOM_VK_LEFT))

indentation

::: browser/base/content/tabbrowser.js:4070
(Diff revision 5)
> -              return this._switcher.shouldActivateDocShell(aBrowser);
> +      return this._switcher.shouldActivateDocShell(aBrowser);
> -            }
> +    }
> -            return (aBrowser == this.selectedBrowser &&
> +    return (aBrowser == this.selectedBrowser &&
> -                    window.windowState != window.STATE_MINIMIZED &&
> +        window.windowState != window.STATE_MINIMIZED &&
> -                    !window.isFullyOccluded) ||
> +        !window.isFullyOccluded) ||
> -                   this._printPreviewBrowsers.has(aBrowser);
> +      this._printPreviewBrowsers.has(aBrowser);

indentation(?)

::: browser/base/content/tabbrowser.js:4403
(Diff revision 5)
> -              this.log("Requested tab is remote?: " + requestedBrowser.isRemoteBrowser);
> +        this.log("Requested tab is remote?: " + requestedBrowser.isRemoteBrowser);
>  
> -              // Figure out which tab we actually want visible right now.
> +        // Figure out which tab we actually want visible right now.
> -              let showTab = null;
> +        let showTab = null;
> -              if (requestedTabState != this.STATE_LOADED &&
> +        if (requestedTabState != this.STATE_LOADED &&
> -                  this.lastVisibleTab && this.loadTimer && !shouldBeBlank) {
> +          this.lastVisibleTab && this.loadTimer && !shouldBeBlank) {

indentation

::: browser/base/content/tabbrowser.js:4602
(Diff revision 5)
> -              }
> +        }
>  
> -              // If we're not loading anything, try loading the requested tab.
> +        // If we're not loading anything, try loading the requested tab.
> -              let requestedState = this.getTabState(this.requestedTab);
> +        let requestedState = this.getTabState(this.requestedTab);
> -              if (!this.loadTimer && !this.minimizedOrFullyOccluded &&
> +        if (!this.loadTimer && !this.minimizedOrFullyOccluded &&
> -                  (requestedState == this.STATE_UNLOADED ||
> +          (requestedState == this.STATE_UNLOADED ||

indentation

::: browser/base/content/tabbrowser.js:4683
(Diff revision 5)
>  
> -                if (state == this.STATE_LOADED &&
> +          if (state == this.STATE_LOADED &&
> -                    !this.maybeVisibleTabs.has(tab) &&
> +            !this.maybeVisibleTabs.has(tab) &&
> -                    tab !== this.lastVisibleTab &&
> +            tab !== this.lastVisibleTab &&
> -                    tab !== this.loadingTab &&
> +            tab !== this.loadingTab &&
> -                    tab !== this.requestedTab) {
> +            tab !== this.requestedTab) {

indentation

::: browser/base/content/tabbrowser.js:4863
(Diff revision 5)
>  
> -            activateBrowserForPrintPreview(browser) {
> +      activateBrowserForPrintPreview(browser) {
> -              let tab = this.tabbrowser.getTabForBrowser(browser);
> +        let tab = this.tabbrowser.getTabForBrowser(browser);
> -              let state = this.getTabState(tab);
> +        let state = this.getTabState(tab);
> -              if (state != this.STATE_LOADING &&
> +        if (state != this.STATE_LOADING &&
> -                  state != this.STATE_LOADED) {
> +          state != this.STATE_LOADED) {

indentation (would also fit on one line)

::: browser/base/content/tabbrowser.js:4885
(Diff revision 5)
> -              // up the tab makes no sense.
> +        // up the tab makes no sense.
> -              if (this.minimizedOrFullyOccluded ||
> +        if (this.minimizedOrFullyOccluded ||
> -                  !tab.linkedPanel ||
> +          !tab.linkedPanel ||
> -                  tab.closing ||
> +          tab.closing ||
> -                  !tab.linkedBrowser.isRemoteBrowser ||
> +          !tab.linkedBrowser.isRemoteBrowser ||
> -                  !tab.linkedBrowser.frameLoader.tabParent) {
> +          !tab.linkedBrowser.frameLoader.tabParent) {

indentation

::: browser/base/content/tabbrowser.js:4894
(Diff revision 5)
> -              // Similarly, if the tab is already in STATE_LOADING or
> +        // Similarly, if the tab is already in STATE_LOADING or
> -              // STATE_LOADED somehow, there's no point in trying to
> +        // STATE_LOADED somehow, there's no point in trying to
> -              // warm it up.
> +        // warm it up.
> -              let state = this.getTabState(tab);
> +        let state = this.getTabState(tab);
> -              if (state === this.STATE_LOADING ||
> +        if (state === this.STATE_LOADING ||
> -                  state === this.STATE_LOADED) {
> +          state === this.STATE_LOADED) {

indentation

::: browser/base/content/tabbrowser.js:4996
(Diff revision 5)
> -              this.preActions();
> +        this.preActions();
>  
> -              if (event.type == "MozLayerTreeReady") {
> +        if (event.type == "MozLayerTreeReady") {
> -                this.onLayersReady(event.originalTarget);
> +          this.onLayersReady(event.originalTarget);
> -              } if (event.type == "MozAfterPaint") {
> +        }
> +        if (event.type == "MozAfterPaint") {

should be else if (or a switch statement, actually)

::: browser/base/content/tabbrowser.js:5003
(Diff revision 5)
> -              } else if (event.type == "MozLayerTreeCleared") {
> +        } else if (event.type == "MozLayerTreeCleared") {
> -                this.onLayersCleared(event.originalTarget);
> +          this.onLayersCleared(event.originalTarget);
> -              } else if (event.type == "TabRemotenessChange") {
> +        } else if (event.type == "TabRemotenessChange") {
> -                this.onRemotenessChange(event.target);
> +          this.onRemotenessChange(event.target);
> -              } else if (event.type == "sizemodechange" ||
> +        } else if (event.type == "sizemodechange" ||
> -                         event.type == "occlusionstatechange") {
> +          event.type == "occlusionstatechange") {

indentation

::: browser/base/content/tabbrowser.js:5179
(Diff revision 5)
> -          }
> +    }
>  
> -          if (AppConstants.platform != "macosx") {
> +    if (AppConstants.platform != "macosx") {
> -            if (aEvent.ctrlKey && !aEvent.shiftKey && !aEvent.metaKey &&
> +      if (aEvent.ctrlKey && !aEvent.shiftKey && !aEvent.metaKey &&
> -                aEvent.keyCode == KeyEvent.DOM_VK_F4 &&
> +        aEvent.keyCode == KeyEvent.DOM_VK_F4 &&
> -                !this.mCurrentTab.pinned) {
> +        !this.mCurrentTab.pinned) {

indentation

::: browser/base/content/tabbrowser.js:5254
(Diff revision 5)
> -          } else {
> +    } else {
> -            label = tab._fullLabel || tab.getAttribute("label");
> +      label = tab._fullLabel || tab.getAttribute("label");
> -            if (AppConstants.NIGHTLY_BUILD &&
> +      if (AppConstants.NIGHTLY_BUILD &&
> -                tab.linkedBrowser &&
> +        tab.linkedBrowser &&
> -                tab.linkedBrowser.isRemoteBrowser &&
> +        tab.linkedBrowser.isRemoteBrowser &&
> -                tab.linkedBrowser.frameLoader) {
> +        tab.linkedBrowser.frameLoader) {

indentation

::: browser/base/content/tabbrowser.js:5476
(Diff revision 5)
> -        <parameter name="aNode"/>
> -        <parameter name="aNextNode"/>
> -        <parameter name="aContainer"/>
> -        <body><![CDATA[
> -          if (aContainer.ownerDocument == document &&
> +    if (aContainer.ownerDocument == document &&
> -              aContainer.id == "TabsToolbar") {
> +      aContainer.id == "TabsToolbar") {

indentation

::: browser/base/content/tabbrowser.js:5483
(Diff revision 5)
> -      <method name="onAreaNodeRegistered">
> +  onAreaNodeRegistered(aArea, aContainer) {
> -        <parameter name="aArea"/>
> -        <parameter name="aContainer"/>
> -        <body><![CDATA[
> -          if (aContainer.ownerDocument == document &&
> +    if (aContainer.ownerDocument == document &&
> -              aArea == "TabsToolbar") {
> +      aArea == "TabsToolbar") {

indentation

Comment 162

[Brian Grinstead [:bgrins]]

Comment on attachment 8951402 [details]
Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220682/diff/5-6/

Comment 163

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220684/diff/5-6/

Comment 164

[Brian Grinstead [:bgrins]]

Comment on attachment 8951404 [details]
Bug 1392352 - Part 2.1 - To fold: use JS tabbrowser implementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220686/diff/5-6/

Comment 165

[Brian Grinstead [:bgrins]]

Comment on attachment 8951405 [details]
Bug 1392352 - Part 2.2 - To fold: remove build instructions and reorganize things a bit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220688/diff/5-6/

Comment 166

[Brian Grinstead [:bgrins]]

Comment on attachment 8951406 [details]
Bug 1392352 - Part 2.3 - To fold: delete the old XBL implementation of tabbrowser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220690/diff/5-6/

Comment 167

[Brian Grinstead [:bgrins]]

Pushed up a rebased version on top of Bug 1432509.  Will go through indentation from comment 161 now.

Comment 168

[Brian Grinstead [:bgrins]]

OK, most of these indentation issues were fixed by searching for '&&\n' and tabbing over the subsequent conditions two spaces. I'm waiting for a try push and if that looks good I'm going to fold and push, then deal with any remaining updates in follow ups in the interest of not needing to rebase this again.

Comment 169

[Brian Grinstead [:bgrins]]

Comment on attachment 8951402 [details]
Bug 1392352 - Part 1 - hg cp tabbrowser.xml to tabbrowser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220682/diff/6-7/

Comment 170

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220684/diff/6-7/

Comment 171

[Brian Grinstead [:bgrins]]

Comment on attachment 8951403 [details]
Bug 1392352 - Part 2 - Translate the tabbrowser implementation into a JS class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220684/diff/7-8/

Comment 172

[Brian Grinstead [:bgrins]]

Try push with folded patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad083b5f37650e68deb0a17403b2baf85eb8ea31.

Comment 173

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5a63c04d672
Part 1 - hg cp tabbrowser.xml to tabbrowser.js;r=dao
https://hg.mozilla.org/integration/autoland/rev/d5be59a9f5fb
Part 2 - Translate the tabbrowser implementation into a JS class;r=mconley,r=dao CLOSED TREE

Comment 174

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f5a63c04d672
https://hg.mozilla.org/mozilla-central/rev/d5be59a9f5fb