Remove mTargetableShells from nsContentTreeOwner

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: mystor, Assigned: bz)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments)

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.

Updated

10 months ago
Priority: -- → P3
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.
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.
Keywords: dev-doc-needed
Created attachment 8817306 [details] [diff] [review]
part 1.  Get rid of GetTargetableShellCount in favor of a more explicit GetTabCount
Attachment #8817306 - Flags: review?(michael)
Attachment #8817306 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

9 months ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8817307 [details] [diff] [review]
part 2.  Get rid of the now-unused nsXULWindow::mTargetableShells
Attachment #8817307 - Flags: review?(michael)
Created attachment 8817308 [details] [diff] [review]
part 3.  Remove the aTargetable argument of contentShellAdded
Attachment #8817308 - Flags: review?(michael)
Created attachment 8817309 [details] [diff] [review]
part 4.  Get rid of content-targetable values for the 'type' attribute.  Just use 'content' instead
Attachment #8817309 - Flags: review?(gijskruitbosch+bugs)
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 &&.
Attachment #8817306 - Flags: review?(michael) → review+
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 ^.^.
Attachment #8817307 - Flags: review?(michael) → review+
> 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.
Attachment #8817308 - Flags: review?(michael) → review+

Comment 10

9 months ago
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...
Attachment #8817306 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 11

9 months ago
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?
Attachment #8817309 - Flags: review?(gijskruitbosch+bugs) → review+
> 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.
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

9 months ago
(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...
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

9 months ago
(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

9 months ago
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?
Flags: needinfo?(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.
Blocks: 1322609
(Assignee)

Updated

9 months ago
Flags: needinfo?(bzbarsky)
And yes, running Marionette tests is nicer now.  Finding where it sticks the log is a bit of a pain, still.  ;)

Comment 20

9 months ago
Pushed by bzbarsky@mozilla.com:
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

9 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

9 months ago
Blocks: 1322414
(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.
I did find where it goes by default.  The obvious improvement from my pov would be to default to the "--gecko-log -" behavior.
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).
Keywords: dev-doc-needed
Is there an annotation we're using for "needs documentation, but not from the MDN team"?
Flags: needinfo?(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.
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.