Closed Bug 1190245 Opened 7 years ago Closed 7 years ago

Some more refactoring of nsFrameLoader

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kanru, Assigned: kanru)

Details

Attachments

(3 files)

Make the flow of DocShell creation / FrameScript loading / BrowserAPI initialization more clear
Assignee: nobody → kchen
Attachment #8642242 - Flags: review?(bugs)
Don't rely on MaybeCreateDocShell to set mRemoteFrame so we don't have
to call MaybeCreateDocShell in weird places. Instead we set mRemoteFrame
early in the nsFrameLoader constructor. This should still allow us to
switch default remoteness dynamically.

Should we get rid of the "MaybeCreateDocShell succeeded, but null mDocShell" assertion?
Attachment #8642243 - Flags: review?(bugs)
Load frame script and initialize browser-api at the end of (1) DocShell
creation (2) RemoteBrowser creation (3) SetRemoteBrowser
Attachment #8642244 - Flags: review?(bugs)
Attachment #8642242 - Flags: review?(bugs) → review+
Comment on attachment 8642243 [details] [diff] [review]
Part 2, Make the MaybeCreateDocShell using code path easier to follow

>+nsFrameLoader::IsRemoteFrame()
>+{
>+  if (mRemoteFrame) {
>+    NS_ASSERTION(!mDocShell, "Find a remote frame with a DocShell");
s/Find/Found/ ?
I wonder if this could be even MOZ_ASSERT so that we'd get a crash on debug builds if this ever happens

> nsFrameLoader::CheckForRecursiveLoad(nsIURI* aURI)
> {
>   nsresult rv;
> 
>+  NS_ASSERTION(!IsRemoteFrame(),
>+               "Shouldn't call CheckForRecursiveLoad on remote frames.");
MOZ_ASSERT here



Thanks for doing this.
Attachment #8642243 - Flags: review?(bugs) → review+
Comment on attachment 8642244 [details] [diff] [review]
Part 3, Make frame script loading and browser-api initialization easier to follow

>-  InitializeBrowserAPI();
>-  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>-  if (os) {
>-    os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
>-                        "inprocess-browser-shown", nullptr);
>-  }
>-
>-  if (OwnerIsBrowserOrAppFrame() && mMessageManager) {
>-    mMessageManager->LoadFrameScript(
>-      NS_LITERAL_STRING("chrome://global/content/BrowserElementChild.js"),
>-      /* allowDelayedLoad = */ true,
>-      /* aRunInGlobalScope */ true);
>+  if (OwnerIsBrowserOrAppFrame()) {
>     // For inproc frames, set the docshell properties.
>     nsCOMPtr<nsIDocShellTreeItem> item = do_GetInterface(docShell);
>     nsAutoString name;
>     if (mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name)) {
>       item->SetName(name);
>     }
>     mDocShell->SetFullscreenAllowed(
>       mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::allowfullscreen) ||
>@@ -1854,16 +1835,25 @@ nsFrameLoader::MaybeCreateDocShell()
>           nullptr);
>       } else {
>         nsCOMPtr<nsILoadContext> context = do_GetInterface(mDocShell);
>         context->SetUsePrivateBrowsing(true);
>       }
>     }
>   }
> 
>+  ReallyLoadFrameScripts();
>+  InitializeBrowserAPI();
>+
>+  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>+  if (os) {
>+    os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
>+                        "inprocess-browser-shown", nullptr);
>+  }
So you change ordering here by moving BrowserElementChild.js loading to happen in InitializeBrowserAPI(), so
that script is now loaded before inprocess-browser-shown.
But I think that change makes sense

>@@ -2233,17 +2223,20 @@ nsFrameLoader::TryRemoteBrowser()
> 
>   if (mOwnerContent->AttrValueIs(kNameSpaceID_None,
>                                  nsGkAtoms::mozpasspointerevents,
>                                  nsGkAtoms::_true,
>                                  eCaseMatters)) {
>     unused << mRemoteBrowser->SendSetUpdateHitRegion(true);
>   }
> 
>-  return true;
>+  ReallyLoadFrameScripts();
>+  InitializeBrowserAPI();
>+
This also changes behavior, since currently nsFrameLoader::ShowRemoteFrame ends up
calling mRemoteBrowser->Show(size, parentIsActive); before EnsureMessageManager.
But, ReallyStartLoading should be in practice be called before ShowRemoteFrame, so again I think the change makes sense.



> nsFrameLoader::SetRemoteBrowser(nsITabParent* aTabParent)
> {
>   MOZ_ASSERT(!mRemoteBrowser);
>   mRemoteFrame = true;
>   mRemoteBrowser = TabParent::GetFrom(aTabParent);
>   mChildID = mRemoteBrowser ? mRemoteBrowser->Manager()->ChildID() : 0;
>+  ReallyLoadFrameScripts();
>+  InitializeBrowserAPI();
>   ShowRemoteFrame(ScreenIntSize(0, 0));

Do you need the method calls here? ShowRemoteFrame should call TryRemoteBrowser
Answer to this and remove the method calls if possible, and then r+
Attachment #8642244 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8642244 [details] [diff] [review]
> Part 3, Make frame script loading and browser-api initialization easier to
> follow
> 
> > nsFrameLoader::SetRemoteBrowser(nsITabParent* aTabParent)
> > {
> >   MOZ_ASSERT(!mRemoteBrowser);
> >   mRemoteFrame = true;
> >   mRemoteBrowser = TabParent::GetFrom(aTabParent);
> >   mChildID = mRemoteBrowser ? mRemoteBrowser->Manager()->ChildID() : 0;
> >+  ReallyLoadFrameScripts();
> >+  InitializeBrowserAPI();
> >   ShowRemoteFrame(ScreenIntSize(0, 0));
> 
> Do you need the method calls here? ShowRemoteFrame should call TryRemoteBrowser
> Answer to this and remove the method calls if possible, and then r+

ShowRemoteFrame doesn't call TryRemoteBrowser because we already have mRemoteBrowser
assigned. The old code ends up calling ReallyLoadFrameScripts and InitializeBrowserAPI
in ShowRemoteFrame which looks very strange to me so I moved it to this place. So now
we load frame scripts after mRemoteBrowser is created or assigned consistently.
You need to log in before you can comment on or make changes to this bug.