Closed Bug 522351 Opened 12 years ago Closed 12 years ago

Only do multi-process tabs when an attribute explicitly asks for it

Categories

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

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file, 1 obsolete file)

Only do the remote thing with <browser remote="true"> (or <iframe remote="true">)

This will let me debug Fennec and still run DOM inspector.
This works, but by itself makes the test app non-functional because there are various things in frameloader and subdocumentframe which expect docshell-getting to succeed. I'll put the bits necessary for making this work again in bug 522352
Attachment #406747 - Flags: review?(bzbarsky)
So this allows web pages to set mRemoteFrame to true.  Do we handle that case at least somewhat sanely (e.g. show nothing or some such)?  I guess this can be ok as a quick hack but longer-term we'd want to not expose that to content...

General style note: in content code, we brace one-line if/else bodies.

Still trying to sort through the new logic to convince myself it's ok.
bz, though web pages could set mRemoteFrame to true, they wouldn't actually be able to create the TabParent because of the docshell check in TryNewProcess, so they would just fail to load. And yeah, this is a quick hack.
OK.  So substantive issues:

1)  Why are we getting rid of the mTriedNewProcess boolean?  Does TryNewProcess
    deal ok with an existing mChildProcess?  It doesn't seem to....
2)  This change at the end of CreateDocShell:

>-    return NS_ERROR_FAILURE;
>+    mDocShell = nsnull;

    seems like it might regress bug 472312, no?

Minor issues:

1)  MaybeCreateDocShell() is probably a better name.
2)  Good catch on the missing return in UpdateBaseWindowPositionAndSize. 
    Somewhat surprised that compiled... maybe on Linux only?
3)  Please brace single line if bodies in content code.
4)  MaybeCreateDocShell() should early-return if mRemoteFrame, right?
> 2)  This change at the end of CreateDocShell:
> 
> >-    return NS_ERROR_FAILURE;
> >+    mDocShell = nsnull;
> 
>     seems like it might regress bug 472312, no?

Hrm, that sucks. I had CreateDocShell returning void and signaling failure with a null mDocShell. Do I need to re-add an nsresult from MaybeCreateDocShell?
That's probably safer, yes.  :(
This addresses issues, I think. MaybeCreateDocShell is now documented and the nsresult return value is restored. mTriedNewProcess is replaced by a null mChildProcess check.
Attachment #406747 - Attachment is obsolete: true
Attachment #408465 - Flags: review?(bzbarsky)
Attachment #406747 - Flags: review?(bzbarsky)
Comment on attachment 408465 [details] [diff] [review]
Only do remote tabs explicitly, rev. 2

>+++ b/content/base/src/nsFrameLoader.cpp
> nsFrameLoader::ReallyStartLoading()
>-  rv = EnsureDocShell();
>   NS_ENSURE_SUCCESS(rv, rv);

Nix that NS_ENSURE_SUCCESS too.

>+nsFrameLoader::ShouldUseRemoteProcess()
>+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);

PRBool remoteDisabled = nsContentUtils::GetBoolPref("dom.ipc.tabs.disabled");

(defaults to false; you can pass in the default as the second arg explicitly if you want to make that clear.

>+  PRBool remoteEnabled = PR_FALSE;

PRBool remoteEnabled = nsContentUtils::GetBoolPref("dom.ipc.tabs.enabled");

>+nsFrameLoader::MaybeCreateDocShell()
>+    // Don't allow subframe loads in external reference documents
>   if (doc->GetDisplayDocument()) {
>-    // Don't allow subframe loads in external reference documents

Don't move the comment, please.

> nsFrameLoader::CheckForRecursiveLoad(nsIURI* aURI)
>+  rv = MaybeCreateDocShell();
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }
>+  if (!mDocShell) {

Assert !mRemoteFrame before the !mDocShell check?

>+++ b/content/base/src/nsFrameLoader.h
>+   * If we are a IPC frame, set mRemoteFrame. Otherwise, create and

"an IPC"?

r=bzbarsky with those nits
Attachment #408465 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/projects/electrolysis/rev/80adc4f2bad0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.