1442058 - Remove the tabbrowser-browser-* bindings

Status: RESOLVED FIXED

(Firefox :: General, task, P3)

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

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

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

https://reviewboard.mozilla.org/r/224154/#review230112


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/browser.xml:149
(Diff revision 1)
>  
>              var aReferrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_UNSET;
>              var aTriggeringPrincipal;
>  
>              // Check for loadURIWithFlags(uri, { ... });
>              var params = arguments[1];

Error: 'params' is already defined. [eslint: no-redeclare]

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

https://reviewboard.mozilla.org/r/224154/#review230114


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/browser.xml:149
(Diff revision 2)
>  
>              var aReferrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_UNSET;
>              var aTriggeringPrincipal;
>  
>              // Check for loadURIWithFlags(uri, { ... });
>              var params = arguments[1];

Error: 'params' is already defined. [eslint: no-redeclare]

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

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

Comment 6

[Tim Nguyen :ntim]

Looks like this broke the responsive mode: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0995d0475c06a7f77bd4fd96ffb761d0fe73cd91

Will investigate.

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

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

Comment 8

[Tim Nguyen :ntim]

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f70786e1072da677236b5ba60db826af1166335

Comment 9

[:Gijs (he/him)]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

https://reviewboard.mozilla.org/r/224154/#review230218

I don't understand the reasoning behind this change, and none is given in the comments on this bug.

I also think it's wrong, because now we'll have toolkit code that depends on a function (`_loadURIWithFlags`) that's defined in browser.js.

Instead, if we really wanted to do this, I think we should assign the `loadURIWithFlags` override as an expando property when creating new browser instances (and on the initial browser when constructing the tabbrowxer). That would be simpler, actually remove the binding overhead here, and not violate any toolkit/browser separation.

Still alternatively, I would actually prefer we more seriously reduce the number of `load some URL` methods we have on browser/docshell/nsiwebnavigation. But there we go.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

https://reviewboard.mozilla.org/r/224154/#review230340

The RDM change by itself seems reasonable and could be done independently, so I broke this out to its own bug.  Thanks for bringing it up!

I defer to Gijs on whether the binding change here is good or not.  Since he seems to feel it's not, I'll cancel my review for now.

::: devtools/client/responsive.html/browser/tunnel.js
(Diff revision 4)
> -      // The XBL binding for remote browsers uses the message manager for many actions in
> -      // the UI and that works well here, since it gives us one main thing we need to
> -      // route to the inner browser (the messages), instead of having to tweak many
> -      // different browser properties.  It is safe to alter a XBL binding dynamically.
> -      // The content within is not reloaded.
> -      outer.style.MozBinding = "url(chrome://browser/content/tabbrowser.xml" +

Looking at this now, it's unclear why this line is here at all, since CSS seems to do the same thing.

It feels a bit unrelated to this bug, so I filed bug 1442334 to hopefully remove it while retaining some of the comments around it.

Comment 11

[ExE Boss]

Comment on attachment 8954968 [details]
Bug 1442058 - Fold tabbrowser-browser-* bindings into browser binding.

https://reviewboard.mozilla.org/r/224154/#review232748

::: toolkit/content/widgets/browser.xml:16
(Diff revision 4)
>    <binding id="browser" extends="xul:browser" role="outerdoc">
>      <content clickthrough="never">
>        <children/>
>      </content>
>      <implementation type="application/javascript" implements="nsIObserver, nsIDOMEventListener, nsIMessageListener, nsIBrowser">
> +

I’m pretty sure that this empty line is unnecessary.

Comment 12

[Tim Nguyen :ntim]

Not going to work on this for now. Going to focus on cleaning other stuff around the browser binding instead.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: Bug 1442058 - Override loadURI on tabbrowser browsers with an expando property;r=dao

This allows us to remove the XBL binding inheritance, simplifying the tree
of bindings under "browser" and deleting two bindings in the process

Comment 14

[Brian Grinstead [:bgrins]]

Going to try doing this with expando properties instead of folding into the toolkit browser bindings

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8999749 [details]
Bug 1442058 - Override loadURI on tabbrowser browsers with an expando property;r=dao

Dão Gottwald [::dao] has approved the revision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/747bdd512c39
Override loadURI on tabbrowser browsers with an expando property;r=dao

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/747bdd512c39