Get rid of content-targetable usage in tabbrowser.xml

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: Gijs)

Tracking

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

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

This is now just noise as far as anything in Gecko and Firefox is concerned.  See bug 1310796.

Unfortunately, actually changing this breaks marionette horribly, because in testing/marionette/driver.js in the GeckoDriver.prototype.registerBrowser function we have:

  if (be.getAttribute("type") != "content") {

and then it actually does something useful.  So it's explicitly ignoring things that have type="content" and then listeners don't get set up for those, messages get lost, and the right things don't happen and the test go interesting colors.

Andreas, you seem to have both code and review blame for this code.  Why is that check needed, exactly?
Flags: needinfo?(ato)
I suspect it's trying to select for tabbrowser browsers as opposed to other frames, such as ones in panels (esp. add-on ones). It should do that differently - specifically, be.getTabBrowser() should return the gBrowser for that window only for the browsers that actually live in the tabbrowser.
Comment hidden (mozreview-request)
To be clear, r?ato for the marionette change - the tabbrowser.xml change was proposed by bz in bug 1310796 and r+'d by me already. :-)
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1322414
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(updated because apparently we pass non-XUL-browser frames (like HTML <iframe>s!) to this method, and it needs to cope with that)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8817636 [details]
Bug 1322609 - use getTabBrowser() instead of a type attribute check in marionette,

https://reviewboard.mozilla.org/r/97852/#review98888

::: testing/marionette/driver.js:432
(Diff revision 2)
> -  if (be.getAttribute("type") != "content") {
> +  if (this.appName != "Firefox" || be.namespaceURI != XUL_NS ||
> +      be.nodeName != "browser" || be.getTabBrowser()) {

I wonder if this if-condition can be removed in its entirety?

I ran the Mn test suite (`./mach marionette-test --gecko-log -`) with some debugging output and couldn’t find a single instance where the if-condition didn’t pass and the `<xul:browser>` wasn’t registered.

The browser’s `type` attribute when calling this would most commonly be `content-primary`, passing the test in the old code:

    if (be.getAttribute("type") != "content") {
      // pass
    }

… but would occasionally be `content-targetable` when dealing with chrome dialogues.

You mentioned `<iframe>`, do you remember which test passed that in?

I also think some of the conditions in your patch are wrong:

(1) `this.appName` is always `"Firefox"` except when it’s `"Fennec"`.  I don’t think it’s relevant to test if the current product _isn’t_ Firefox?

(2) As far as I could tell, the `be.namespaceURI` is _always_ equivalent to `XUL_NS`.

(3) I also couldn’t find a single case where `be.nodeName` wasn’t `"xul:browser"`, so this is the test that causes the if-condition to pass because of the ORs.

(4) `be.getTabBrowser()` also consistently returned `<xul:tabbrowser>`.
Attachment #8817636 - Flags: review?(ato) → review-
(In reply to :Gijs Kruitbosch from comment #1)
> I suspect it's trying to select for tabbrowser browsers as opposed to other
> frames, such as ones in panels (esp. add-on ones). It should do that
> differently - specifically, be.getTabBrowser() should return the gBrowser
> for that window only for the browsers that actually live in the tabbrowser.

I think that was its original purpose, but I can’t find any evidence that any random browser/frame calls this any longer.  I suspect each relevant browser calls `Marionette:register` explicitly, causing `GeckoDriver#registerBrowser` to run:

    https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/driver.js#L2761

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> Andreas, you seem to have both code and review blame for this code.  Why is that check needed, exactly?

Just to be clear I didn’t write this code, but I may have moved it in the past.
Flags: needinfo?(ato)
(Assignee)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8817636 [details]
Bug 1322609 - use getTabBrowser() instead of a type attribute check in marionette,

https://reviewboard.mozilla.org/r/97852/#review98894

::: testing/marionette/driver.js:432
(Diff revision 2)
> -  if (be.getAttribute("type") != "content") {
> +  if (this.appName != "Firefox" || be.namespaceURI != XUL_NS ||
> +      be.nodeName != "browser" || be.getTabBrowser()) {

I'd be happy to remove it, but I don't know why it was added - you wrote the patch, I think. :-)

About the conditions:
- in Fennec, we don't use the tabbrowser widget, so there isn't a tabbrowser for the <browser> widgets (if we even get some). Hence the check for non-Firefox.
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f97f1e80441be132ff71adbab11a5&selectedJob=32461678  test_switch_remote_frame.py TestSwitchRemoteFrame.test_remote_frame uses an HTML iframe, which of course isn't a XUL element, or a XUL <browser> specifically, and so won't have a getTabBrowser() method.

If you don't think it makes a difference, we can remove the condition entirely. I don't know why it was added here in the first place, if my original supposition here was wrong...
As a side note I filed bug 1323185 yesterday which is about getting tab handling support added for Fennec. So it's indeed a totally different story.

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8817636 [details]
Bug 1322609 - use getTabBrowser() instead of a type attribute check in marionette,

https://reviewboard.mozilla.org/r/97852/#review98894

> I'd be happy to remove it, but I don't know why it was added - you wrote the patch, I think. :-)
> 
> About the conditions:
> - in Fennec, we don't use the tabbrowser widget, so there isn't a tabbrowser for the <browser> widgets (if we even get some). Hence the check for non-Firefox.
> - https://treeherder.mozilla.org/#/jobs?repo=try&revision=68681056de2f97f1e80441be132ff71adbab11a5&selectedJob=32461678  test_switch_remote_frame.py TestSwitchRemoteFrame.test_remote_frame uses an HTML iframe, which of course isn't a XUL element, or a XUL <browser> specifically, and so won't have a getTabBrowser() method.
> 
> If you don't think it makes a difference, we can remove the condition entirely. I don't know why it was added here in the first place, if my original supposition here was wrong...

> I'd be happy to remove it, but I don't know why it was added - you wrote the patch, I think. :-)

Just to be clear, I definitely didn’t write this.  But I am trying to decipher what is the meaning of this code (-:

> - in Fennec, we don't use the tabbrowser widget, so there isn't a tabbrowser for the `<browser>` widgets (if we even get some). Hence the check for non-Firefox.

OK.

> test_switch_remote_frame.py TestSwitchRemoteFrame.test_remote_frame uses an HTML iframe, which of course isn't a XUL element, or a XUL `<browser>` specifically, and so won't have a getTabBrowser() method.

OK.

To summarise:

(1) `this.appName` needed because Fennec does not have `<xul:tabbrowser>`.
(2) `be.namespaceURI != XUL_NS` needed because test_remote_frame passes in `<iframe>`.
(3) `be.nodeName != "browser"` needed because some elements, amongst those the Pocket panel, contains `<xul:iframe>` elements.
(4) `be.getTabBrowser()` to preserve backwards compatibility by only including those top-level browsers that are linked to a tabbrowser (e.g. not standalone `<xul:browser>`s).

Could I trouble you to add a comment explaining this?

Comment 11

2 years ago
mozreview-review
Comment on attachment 8817636 [details]
Bug 1322609 - use getTabBrowser() instead of a type attribute check in marionette,

https://reviewboard.mozilla.org/r/97852/#review98916
Attachment #8817636 - Flags: review- → review+
After this patch lands I intended to follow up on this by cleaning up the browser registration code.  We have a related bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1311041) where we want to migrate from using outerWindowID for tracking browsers to using permanentKey.  We also have a desire to move away from maintaining a cache of ID-to-browser-reference because of the maintenance burden this carries with it.

Comment 13

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4616e2bb5fc4
use getTabBrowser() instead of a type attribute check in marionette, r=ato

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4616e2bb5fc4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

2 years ago
Blocks: 1324121
You need to log in before you can comment on or make changes to this bug.