Remove the tabbrowser-browser-* bindings

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: ntim, Assigned: bgrins)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

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

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

Comment 6

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

Will investigate.
(Reporter)

Updated

a year ago
Summary: Fold tabbrowser-browser-* bindings into browser-* bindings → Fold tabbrowser-browser-* bindings into browser binding
Comment hidden (mozreview-request)

Comment 9

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

a year ago
mozreview-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)
Depends on: 1442334
(Assignee)

Updated

a year ago
Blocks: war-on-xbl
Priority: -- → P3
(Reporter)

Updated

a year ago
Blocks: 1444760
(Reporter)

Updated

a year ago
No longer blocks: 1444760
Depends on: 1444760

Comment 11

a year ago
mozreview-review
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.
(Reporter)

Comment 12

a year ago
Not going to work on this for now. Going to focus on cleaning other stuff around the browser binding instead.
Assignee: ntim.bugs → nobody
(Reporter)

Updated

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

Comment 13

8 months ago
This allows us to remove the XBL binding inheritance, simplifying the tree
of bindings under "browser" and deleting two bindings in the process
(Assignee)

Comment 14

8 months ago
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+
(Reporter)

Updated

8 months ago
Attachment #8954968 - Attachment is obsolete: true

Comment 16

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

Comment 17

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/747bdd512c39
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Reporter)

Updated

8 months ago
Depends on: 1444760
You need to log in before you can comment on or make changes to this bug.