Add a way to get TabParent for content-primary tab

RESOLVED FIXED in Firefox 44

Status

()

Core
Document Navigation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

36 Branch
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(3 attachments)

Currently one can get docshell in non-e10s, but in e10s we need TabParent.
Created attachment 8667594 [details] [diff] [review]
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 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
Attachment #8667594 - Flags: review?(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?
Attachment #8667594 - Flags: review?(mconley)
> >                                  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.
Created attachment 8668111 [details] [diff] [review]
v2
Attachment #8668111 - Flags: review?(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.
Attachment #8668111 - Flags: review?(mconley) → review+
Created attachment 8668523 [details] [diff] [review]
+indentation fix

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a473909f5759
https://hg.mozilla.org/mozilla-central/rev/a473909f5759
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.