Remove the tabbrowser-browser-* bindings

RESOLVED FIXED in Firefox 63

Status

()

task
P3
normal
RESOLVED FIXED
Last year
12 days 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)

Reporter

Description

Last year
No description provided.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

Last year
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

Last year
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

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

Will investigate.
Reporter

Updated

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

Comment 9

Last year
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

Last year
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

Last year
Blocks: war-on-xbl
Priority: -- → P3
Reporter

Updated

Last year
Blocks: 1444760
Reporter

Updated

Last year
No longer blocks: 1444760
Depends on: 1444760

Comment 11

Last year
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

Last year
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

Last year
No longer depends on: 1444760
Assignee

Comment 13

10 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

10 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

Updated

10 months ago
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

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

Comment 16

10 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/747bdd512c39
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Reporter

Updated

10 months ago
Depends on: 1444760
Reporter

Updated

12 days ago
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.