1322609 - Get rid of content-targetable usage in tabbrowser.xml

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[:Gijs (he/him)]

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 2

[:Gijs (he/him)]

Attachment: Bug 1322609 - use getTabBrowser() instead of a type attribute check in marionette,

Review commit: https://reviewboard.mozilla.org/r/97852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97852/

Comment 3

[:Gijs (he/him)]

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

Comment 4

[:Gijs (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97852/diff/1-2/

Comment 5

[:Gijs (he/him)]

(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

[Andreas Tolfsen ❲:ato❳]

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

Comment 7

[Andreas Tolfsen ❲:ato❳]

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

Comment 8

[:Gijs (he/him)]

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

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Andreas Tolfsen ❲:ato❳]

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

[Andreas Tolfsen ❲:ato❳]

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

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

Comment 12

[Andreas Tolfsen ❲:ato❳]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4616e2bb5fc4
use getTabBrowser() instead of a type attribute check in marionette, r=ato

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4616e2bb5fc4