1209621 - Add a way to get TabParent for content-primary tab

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently one can get docshell in non-e10s, but in e10s we need TabParent.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7de6f10370cd


The added/removed API is a bit silly, but tried to model is based on the contentShellAdded/Removed.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8667594 [details] [diff] [review]
wip

mconley, perhaps you could review this.
As I said TabParentAdded, is a bit ugly, but following ContentShellAdded model

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667594 [details] [diff] [review]
wip

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

::: dom/base/nsFrameLoader.cpp
@@ -2716,5 @@
>                                  nsIAtom*     aAttribute,
>                                  int32_t      aModType,
>                                  const nsAttrValue* aOldValue)
>  {
> -  MOZ_ASSERT(mObservingOwnerContent);

Can you explain why the mObservingOwnerContent assertion is no longer needed?

@@ +3027,5 @@
> +      return;
> +    }
> +
> +    if (!mObservingOwnerContent) {
> +      mOwnerContent->AddMutationObserver(this);

Why do we need to add this?

Won't we just enter AttributeChanged, and then exit early because mDocShell is nullptr for the remote frameloader?

::: dom/base/test/chrome/file_bug1209621.xul
@@ +24,5 @@
> +  }
> +
> +  function run() {
> +    var docshell = window.getInterface(Ci.nsIDocShell);
> +    ok(docshell);

If possible, let's give these some useful message, like
ok(docshell, "Window should have a docshell");

I know it's a bit pedantic, but it's easier to see the flow of what's being tested, especially as the test runs.

::: embedding/browser/nsDocShellTreeOwner.cpp
@@ +335,5 @@
>      return mTreeOwner->ContentShellAdded(aContentShell, aPrimary, aTargetable,
>                                           aID);
>  
>    if (aPrimary) {
>      mPrimaryContentShell = aContentShell;

A minor suggestion - while we've got a number of places here where you've ensured that we null out the other mPrimaryX when setting mPrimaryY, it'd be nice if we could add some stronger guarantees.

Perhaps we should add private SetPrimaryTabParent and SetPrimaryContentShell methods to ensure that the right member is set and the other is null?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

> >                                  nsIAtom*     aAttribute,
> >                                  int32_t      aModType,
> >                                  const nsAttrValue* aOldValue)
> >  {
> > -  MOZ_ASSERT(mObservingOwnerContent);
> 
> Can you explain why the mObservingOwnerContent assertion is no longer needed?
> 
I think it is just useless, but I can keep it



> @@ +3027,5 @@
> > +      return;
> > +    }
> > +
> > +    if (!mObservingOwnerContent) {
> > +      mOwnerContent->AddMutationObserver(this);
> 
> Why do we need to add this?
Because we don't other observe attribute changes on remote browsers
We need to add mutation observer in order to get AttributeChanged called.

> Won't we just enter AttributeChanged, and then exit early because mDocShell
> is nullptr for the remote frameloader?
No, not without AddMutationObserver call



>If possible, let's give these some useful message, like
>ok(docshell, "Window should have a docshell");
Sure (I'm lazy with tests since they anyway take always more time to write than the actual code)


> @@ +335,5 @@
> >      return mTreeOwner->ContentShellAdded(aContentShell, aPrimary, aTargetable,
> >                                           aID);
> >  
> >    if (aPrimary) {
> >      mPrimaryContentShell = aContentShell;
> 
> A minor suggestion - while we've got a number of places here where you've
> ensured that we null out the other mPrimaryX when setting mPrimaryY, it'd be
> nice if we could add some stronger guarantees.
> 
> Perhaps we should add private SetPrimaryTabParent and SetPrimaryContentShell
> methods to ensure that the right member is set and the other is null?
mPrimaryTabParent is set in one place and mPrimaryContentShell also. We just have two different classes
dealing with those, but they are totally different things (nsDocShellTreeOwner is part of the embedding API).
So I don't really see need for helper methods for things which happen only once.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8668111 [details] [diff] [review]
v2

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

LGTM - thanks smaug!

::: docshell/base/nsIDocShellTreeOwner.idl
@@ +68,5 @@
> +	void tabParentAdded(in nsITabParent aTab, in boolean aPrimary);
> +	void tabParentRemoved(in nsITabParent aTab);
> +
> +	/*
> +	 In multiprocess case we may not have primaryContentShell but

Nit - busted indentation with the block you've added. I actually prefer your style, but we might as well be consistent with the rest of the commentary in here until we fix it en masse.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +indentation fix

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a473909f5759

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a473909f5759