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 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]

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

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

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 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 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.

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

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

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

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.

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/747bdd512c39 Override loadURI on tabbrowser browsers with an expando property;r=dao