Migrate the tabbrowser XBL binding to a JS class

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 16 obsolete attachments)

59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
dao
: review+
Details
(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
(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?
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
Also, I'm really excited about this bug! Thanks for working on it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
(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
(Assignee)

Comment 18

2 years ago
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
(Assignee)

Comment 19

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 years ago
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
(Assignee)

Comment 25

2 years ago
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?
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

2 years ago
Depends on: 1394975
(Assignee)

Updated

2 years ago
See Also: → bug 575258
Assignee: nobody → bgrinstead
(Assignee)

Updated

2 years ago
Duplicate of this bug: 575254
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8901276 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
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
Component: General → Tabbed Browser
FWIW, this was already on my mental longer-term to-do list. We'll definitely want to do this, one way or another.
Priority: P5 → P2
Summary: Investigate migration of the tabbrowser XBL binding to a JS class → Migrate the tabbrowser XBL binding to a JS class
(Assignee)

Updated

2 years ago
Blocks: 1397874
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 39

2 years ago
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.
(Assignee)

Comment 40

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 46

2 years ago
I think the issue in Comment 40 may be the same thing as Bug 1401846
See Also: → bug 1401846
yes! :) Glad to see you're on it, as I got stuck when tried to get this into 57 due to this bug :(
(Assignee)

Comment 48

a year ago
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
(Assignee)

Updated

a year ago
Depends on: 1409784
(Assignee)

Updated

a year ago
Depends on: 1408572
(Assignee)

Comment 50

a year ago
(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

Updated

a year ago
See Also: → bug 1422747
(Assignee)

Comment 51

a year ago
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.
Depends on: 1412456
Flags: needinfo?(bill.mccloskey)
(Assignee)

Comment 52

a year ago
Note from standard8: it'd be better to use `// eslint-disable-next-line complexity` in front of addTab instead of `/* eslint-disable complexity */`
Depends on: 1436361
(Assignee)

Updated

a year ago
Attachment #8919812 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 63

a year ago
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
Depends on: 1418403
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 76

a year ago
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

a year ago
mozreview-review
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?
Attachment #8901277 - Flags: review?(kmaglione+bmo) → review-

Comment 78

a year ago
mozreview-review
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
Attachment #8899558 - Flags: review?(dao+bmo) → review+

Comment 79

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

https://reviewboard.mozilla.org/r/220032/#review226432
Attachment #8950800 - Flags: review?(dao+bmo) → review+

Comment 80

a year ago
mozreview-review
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
Attachment #8950801 - Flags: review?(dao+bmo) → review+

Comment 81

a year ago
mozreview-review
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
Attachment #8951011 - Flags: review?(dao+bmo) → review+
(Assignee)

Updated

a year ago
Depends on: 1438579
(Assignee)

Comment 82

a year ago
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
Attachment #8899559 - Flags: review?(mconley)
Attachment #8899559 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 91

a year ago
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
Attachment #8901277 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Attachment #8915775 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8901277 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8899558 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8950800 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8951011 - Attachment is obsolete: true
Attachment #8951011 - Flags: review?(mconley)
(Assignee)

Updated

a year ago
Attachment #8899559 - Attachment is obsolete: true
Attachment #8899559 - Flags: review?(mconley)
Attachment #8899559 - Flags: review?(dao+bmo)
(Assignee)

Updated

a year ago
Attachment #8950801 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8951396 - Attachment is obsolete: true
Attachment #8951396 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 100

a year ago
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 102

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

https://reviewboard.mozilla.org/r/220682/#review226736
Attachment #8951402 - Flags: review?(dao+bmo) → review+

Comment 103

a year ago
mozreview-review
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
Attachment #8951405 - Flags: review?(dao+bmo) → review+

Comment 104

a year ago
mozreview-review
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
Attachment #8951403 - Flags: review?(dao+bmo)

Comment 105

a year ago
mozreview-review
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
Attachment #8951404 - Flags: review?(dao+bmo) → review+

Comment 106

a year ago
mozreview-review
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
Attachment #8951406 - Flags: review?(dao+bmo) → review+
Blocks: 1436361
No longer depends on: 1436361
(Assignee)

Comment 107

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8951401 - Attachment is obsolete: true
(Assignee)

Comment 115

a year ago
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
Attachment #8951403 - Flags: review?(dao+bmo)
(Assignee)

Comment 116

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

Comment 117

a year ago
(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
Flags: needinfo?(kmaglione+bmo)
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.
(Assignee)

Comment 119

a year ago
(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.
(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

a year ago
mozreview-review
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
Attachment #8951404 - Flags: review?(mconley) → review+

Comment 122

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8951399 - Attachment is obsolete: true
(Assignee)

Comment 129

a year ago
Pushed up the whitespace fix on 3.1 and removed part 0

Comment 130

a year ago
mozreview-review
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
Attachment #8951403 - Flags: review?(dao+bmo)

Comment 131

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
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.
Attachment #8951403 - Flags: review?(mconley) → review-
(Assignee)

Comment 133

a year ago
(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.
(Assignee)

Comment 134

a year ago
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.
(Assignee)

Updated

a year ago
See Also: → bug 1440508
(Assignee)

Comment 135

a year ago
(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).
(Assignee)

Comment 136

a year ago
(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).
(Assignee)

Comment 137

a year ago
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.
(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.
(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.
(Assignee)

Comment 140

a year ago
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).
(Assignee)

Comment 141

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 1440949
(Assignee)

Updated

a year ago
No longer depends on: 1412456
(Assignee)

Comment 142

a year ago
Clearing the needinfo since we discussed how to check for the interposition property on IRC and I filed Bug 1440949 to do this
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8951400 - Attachment is obsolete: true
(Assignee)

Comment 148

a year ago
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

a year ago
mozreview-review
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.
Attachment #8951403 - Flags: review?(mconley) → review+
(Assignee)

Comment 150

a year ago
(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.
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.
I agree - especially regarding ESR 60. Let's try to get this in before the freeze.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 158

a year ago
Rebased, and issues from Comment 149 addressed
(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
(Assignee)

Comment 160

a year ago
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

a year ago
mozreview-review
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
Attachment #8951403 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 167

a year ago
Pushed up a rebased version on top of Bug 1432509.  Will go through indentation from comment 161 now.
(Assignee)

Comment 168

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8951404 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8951405 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8951406 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 173

a year ago
Pushed by archaeopteryx@coole-files.de:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5a63c04d672
https://hg.mozilla.org/mozilla-central/rev/d5be59a9f5fb
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Blocks: 1441816
(Assignee)

Updated

a year ago
See Also: → bug 1441935
Depends on: 1442564
Blocks: 1442651
Blocks: 1442564
No longer depends on: 1442564
See Also: bug 1401846
See Also: bug 1422747
Depends on: 1443453
Depends on: 1493343
No longer depends on: 1493343
You need to log in before you can comment on or make changes to this bug.