1310796 - Remove mTargetableShells from nsContentTreeOwner

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

Follow up from bug 1303196

> I believe after this mTargetableShells is write-only, so we can nix it and all 
> its management.  We should also be able to nix GetTargetableShellCount(), the 
> aTargetable argument to ContentShellAdded, the special-casing of "content-
> targetable" in the callers of ContentShellAdded, and the use of "content-
> targetable" in our chrome.  Followup bug, please.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So it's not _entirely_ dead, looks like.  The sole remaining consumer is nsGlobalWindow::CanMoveResizeWindows, which wants to know the number of tabs in a window.  But we should be able to make its impl more like the e10s one, I think, and use nsIXULBrowserWindow::GetTabCount.

Comment 2

[Boris Zbarsky [:bzbarsky]]

We'll need to update at least https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/browser.type and https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/iframe and maybe other docs.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Get rid of GetTargetableShellCount in favor of a more explicit GetTabCount

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Get rid of the now-unused nsXULWindow::mTargetableShells

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the aTargetable argument of contentShellAdded

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Get rid of content-targetable values for the 'type' attribute. Just use 'content' instead

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8817306 [details] [diff] [review]
part 1.  Get rid of GetTargetableShellCount in favor of a more explicit GetTabCount

Review of attachment 8817306 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIDocShellTreeOwner.idl
@@ +99,5 @@
> +    /*
> +     * Gets the number of tabs currently open in our window, assuming
> +     * this tree owner has such a concept.
> +     */
> +    readonly attribute unsigned long tabCount;

Indentation here isn't consistent with the rest of this file, which appears to use tabs.

::: dom/base/nsGlobalWindow.cpp
@@ +7021,5 @@
>      } else {
>        nsCOMPtr<nsIDocShellTreeOwner> treeOwner = GetTreeOwner();
> +      if (treeOwner &&
> +          NS_FAILED(treeOwner->GetTabCount(&itemCount))) {
> +        itemCount = 0;

This seems a bit confusing, despite it being correct. I think it would be nicer if we could write it more like:

if (!treeOwner || NS_FAILED(treeOwner->GetTabCount(&itemCount))) {
  // This has different semantics in isolation, but itemCount is already 0 if treeOwner is falsey.
  itemCount = 0;
}

just because it means that we don't flip-flop between using negative and positive reasoning between the two sides of the &&.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8817307 [details] [diff] [review]
part 2.  Get rid of the now-unused nsXULWindow::mTargetableShells

Review of attachment 8817307 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpfe/appshell/nsXULWindow.cpp
@@ -1734,5 @@
>      if (mPrimaryContentShell == aContentShell)
>        mPrimaryContentShell = nullptr;
>    }
>  
> -  if (aTargetable) {

I was going to point out that this parameter wasn't necessary anymore, but I suppose that's what the 3rd patch is for ^.^.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Indentation here isn't consistent with the rest of this file, which appears to use tabs.

Yeah, indentation in the rest of the file is ... mixed.  I can try to use tabs, I guess.  It'll still suck with some of the other lines, like where we want to line up arguments.  But OK.

> I think it would be nicer if we could write it more like:

OK, will do.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8817306 [details] [diff] [review]
part 1.  Get rid of GetTargetableShellCount in favor of a more explicit GetTabCount

Review of attachment 8817306 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +7021,5 @@
>      } else {
>        nsCOMPtr<nsIDocShellTreeOwner> treeOwner = GetTreeOwner();
> +      if (treeOwner &&
> +          NS_FAILED(treeOwner->GetTabCount(&itemCount))) {
> +        itemCount = 0;

What mystor said, but can we mark this infallible in the idl file? Then we just don't need to check, or do I misunderstand our guarantees about that? (very possible!)

::: embedding/browser/nsDocShellTreeOwner.cpp
@@ +482,3 @@
>  {
>    if (mTreeOwner) {
> +    mTreeOwner->GetTabCount(aResult);

because it seems we don't pass on the rv here already...

Comment 11

[:Gijs (he/him)]

Comment on attachment 8817309 [details] [diff] [review]
part 4.  Get rid of content-targetable values for the 'type' attribute.  Just use 'content' instead

Review of attachment 8817309 [details] [diff] [review]:
-----------------------------------------------------------------

OK... so for bug 1322414 do I just move the primary state to its own bool attr? Is that still useful, as you've kept it here?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> but can we mark this infallible in the idl file? 

No, because it's not infallible: it's calling into JS, which can always throw.  We could convert exception to 0 closer to those callsites, I suppose, but it seems pretty weird to do that.

> because it seems we don't pass on the rv here already...

Good catch.  We should!

> OK... so for bug 1322414 do I just move the primary state to its own bool attr?

That seems reasonable, yes.

> Is that still useful, as you've kept it here?

It's still used (e.g. for the sizing we do for window.open("", "", "height=100,width=200"), as far as I can tell.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So one thing that confuses me here is that part 4 causes various test failures which don't show up with just parts 1-3 applied.  Compare https://treeherder.mozilla.org/#/jobs?repo=try&revision=b807b3cc2cbe83e85042c405092dd3817c6f1096&selectedJob=32376196 (parts 1-3) and https://treeherder.mozilla.org/#/jobs?repo=try&revision=666a3cb5c96382e0d502fdf10b07c8976a86da78&selectedJob=32375946 (also part 4).  Lots of marionette failures in the latter, at first glance... (for Mn and en-US tests).

I guess I'll try to do some more try pushes to try to narrow it down.

Comment 14

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> So one thing that confuses me here is that part 4 causes various test
> failures which don't show up with just parts 1-3 applied.  Compare
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b807b3cc2cbe83e85042c405092dd3817c6f1096&selectedJob=3
> 2376196 (parts 1-3) and
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=666a3cb5c96382e0d502fdf10b07c8976a86da78&selectedJob=3
> 2375946 (also part 4).  Lots of marionette failures in the latter, at first
> glance... (for Mn and en-US tests).
> 
> I guess I'll try to do some more try pushes to try to narrow it down.

I looked at this earlier and have no idea what's going on, sorry. :-(

Can you reproduce locally? It's a bit unclear from the logs what's happening...

Comment 15

[Boris Zbarsky [:bzbarsky]]

And it's specifically the tabbrowser.xml changes that cause the Marionette breakage.  Compare https://treeherder.mozilla.org/#/jobs?repo=try&revision=310f1f42cbcac0170e4187086da2008456312e63 to https://treeherder.mozilla.org/#/jobs?repo=try&revision=440497eb928a667494d09441f75dd384de0b30a3

I haven't tried reproducing locally yet; I have no idea how to run the l10n tests and running the marionette tests was a huge PITA last I tried, so I want to make sure I have a few hours to devote to it.  I just figured I'd check with you in case you happened to know something offhand.  ;)

Comment 16

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #15)
> And it's specifically the tabbrowser.xml changes that cause the Marionette
> breakage.  Compare
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=310f1f42cbcac0170e4187086da2008456312e63 to
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=440497eb928a667494d09441f75dd384de0b30a3
> 
> I haven't tried reproducing locally yet; I have no idea how to run the l10n
> tests and running the marionette tests was a huge PITA last I tried, so I
> want to make sure I have a few hours to devote to it.  I just figured I'd
> check with you in case you happened to know something offhand.  ;)

Marionette should just be:

./mach marionette-test testing/marionette/harness/marionette/tests/unit/test_about_pages.py

I can repro locally, but unfortunately that brings me no closer to knowing what's broken... don't see any relevant JS errors right now. Will see if a js debugger helps.

Comment 17

[:Gijs (he/him)]

OK, so, progress: it's breaking because marionette registers a frame script in a window, and expects it to be loaded individually for every following frame:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#366

and that fails to happen after these patches (ie we don't load a second/third/nth copy of the script for the second/third/nth tab (ie child docshell) we open). I haven't looked at why. Does that help, bz?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm going to spin out the tabbrowser.xml part of part 4 into bug 1322609.  That bug also explains why the tabbrowser.xml changes made marionette unhappy.

Comment 19

[Boris Zbarsky [:bzbarsky]]

And yes, running Marionette tests is nicer now.  Finding where it sticks the log is a bit of a pain, still.  ;)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5616da76211
part 1.  Get rid of GetTargetableShellCount in favor of a more explicit GetTabCount.  r=mystor,gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6b91cd71b1
part 2.  Get rid of the now-unused nsXULWindow::mTargetableShells.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/110d3d8ffb94
part 3.  Remove the aTargetable argument of contentShellAdded.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/6395d5edfd15
part 4.  Get rid of 'content-targetable' values for the 'type' attribute everywhere except tabbrowser.xml.  Just use 'content' instead.  r=gijs

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5616da76211
https://hg.mozilla.org/mozilla-central/rev/5e6b91cd71b1
https://hg.mozilla.org/mozilla-central/rev/110d3d8ffb94
https://hg.mozilla.org/mozilla-central/rev/6395d5edfd15

Comment 22

[Andreas Tolfsen ❲:ato❳]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #19)
> And yes, running Marionette tests is nicer now.  Finding where it sticks the
> log is a bit of a pain, still.  ;)

If you mean the Gecko log, you can redirect it to stdout this way:

    % ./mach marionette-test --gecko-log - TESTFILE

If you change - to gecko.log it will write it to a file.

You can also pass -v and -vv for more verbose Marionette-specific logging.

Please let me know if we can improve this in any way.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I did find where it goes by default.  The obvious improvement from my pov would be to default to the "--gecko-log -" behavior.

Comment 24

[Chris Mills [:cmills]]

Removing dev-doc-needed from this — the MDN team unfortunately no longer has the time to work on all the areas on the site, so we've chosen to prioritise web platform and deprioritise some of the other areas (XUL being one of those).

Comment 25

[Boris Zbarsky [:bzbarsky]]

Is there an annotation we're using for "needs documentation, but not from the MDN team"?

Comment 26

[Chris Mills [:cmills]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #25)
> Is there an annotation we're using for "needs documentation, but not from
> the MDN team"?

This is a good question. I'll discuss this with my team, then report back.