Closed Bug 1442058 Opened 3 years ago Closed 2 years ago

Remove the tabbrowser-browser-* bindings

Categories

(Firefox :: General, task, P3)

task

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: ntim, Assigned: bgrins)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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.
Summary: Fold tabbrowser-browser-* bindings into browser-* bindings → Fold tabbrowser-browser-* bindings into browser binding
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.
Attachment #8954968 - Flags: review?(gijskruitbosch+bugs) → review-
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.
Attachment #8954968 - Flags: review?(jryans)
Blocks: war-on-xbl
Priority: -- → P3
Blocks: 1444760
No longer blocks: 1444760
Depends on: 1444760
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.
Assignee: ntim.bugs → nobody
No longer depends on: 1444760
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
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Summary: Fold tabbrowser-browser-* bindings into browser binding → Remove the tabbrowser-browser-* bindings
Attachment #8999749 - Attachment description: Bug 1442058 - Override loadURI on tabbrowser browsers with an expando property → Bug 1442058 - Override loadURI on tabbrowser browsers with an expando property;r=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.
Attachment #8999749 - Flags: review+
Attachment #8954968 - Attachment is obsolete: true
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/747bdd512c39
Override loadURI on tabbrowser browsers with an expando property;r=dao
https://hg.mozilla.org/mozilla-central/rev/747bdd512c39
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1444760
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.