Comment on attachment 8482967 [details] [diff] [review] 1061864p1.patch Review of attachment 8482967 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +3134,5 @@ > + if (NS_SUCCEEDED(parentAsDocShell->GetIsPrerendered(&value))) > + { > + if(value) > + { > + SetInPrerendering(); Nit: four space indentation in this file. @@ +5775,5 @@ > > NS_IMETHODIMP > +nsDocShell::SetInPrerendering() > +{ > + mIsPrerendered = true; Please MOZ_ASSERT that mIsPrerendered is false before you set it to true here to make sure people don't call SetInPrerendering by mistake on a docshell which is already in prerendering. ::: docshell/base/nsIDocShell.idl @@ +620,5 @@ > /** > + * Puts the docshell in prerendering mode > + */ > + [noscript] void SetInPrerendering(); > + readonly attribute boolean isPrerendered; Please rev the uuid. Also, please explain in the comment why the setter method is noscript.

Comment on attachment 8482969 [details] [diff] [review] 1061864p2.patch Review of attachment 8482969 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsIFrameLoader.idl @@ +226,5 @@ > > /** > + * Puts the FrameLoaderOwner in prerendering mode. > + */ > + void setInPrerendering(); Please rev the uuids for both of these interfaces. ::: content/base/src/nsElementFrameLoaderOwner.cpp @@ +112,5 @@ > // Strangely enough, this method doesn't actually ensure that the > // frameloader exists. It's more of a best-effort kind of thing. > mFrameLoader = nsFrameLoader::Create(thisElement, mNetworkCreated); > + if(mIsPrerendered) > + { Nit: opening brace should go on the previous line. Also, whitespace before ( please. @@ +141,5 @@ > > +NS_IMETHODIMP > +nsElementFrameLoaderOwner::SetInPrerendering() > +{ > + NS_ASSERTION(!mFrameLoader,"Please call SetInPrerendering before frameLoader is created"); Please use MOZ_ASSERT. This assertion should be fatal. @@ +147,5 @@ > + if(mFrameLoader) > + { > + nsresult rv = mFrameLoader->SetInPrerendering(); > + NS_ENSURE_SUCCESS(rv,rv); > + } With the above assertion, you can remove this if block, right? ::: content/base/src/nsFrameLoader.cpp @@ +296,5 @@ > > +NS_IMETHODIMP > +nsFrameLoader::SetInPrerendering() > +{ > + NS_ASSERTION(!mDocShell,"Please call SetInPrerendering before docShell is created"); Nit: MOZ_ASSERT. ::: content/base/src/nsFrameLoader.h @@ +331,5 @@ > // enables us to detect whether the frame has moved documents during > // a reframe, so that we know not to restore the presentation. > nsCOMPtr<nsIDocument> mContainerDocWhileDetached; > > + bool mIsPrerendered = false; Hmm, is this C++? ;-) Please initialize in the constructor. Also, make this a 1 bit field like the below. ::: content/xul/content/src/nsXULElement.cpp @@ +1625,5 @@ > nsresult > +nsXULElement::SetInPrerendering() > +{ > + nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader(); > + NS_ASSERTION(!frameLoader,"Please call SetInPrerendering before frameLoader is created"); MOZ_ASSERT. @@ +1632,5 @@ > + if(frameLoader) > + { > + nsresult rv = frameLoader->SetInPrerendering(); > + NS_ENSURE_SUCCESS(rv,rv); > + } This if block can be removed as well, right?

Comment on attachment 8482971 [details] [diff] [review] 1061864p3.patch Review of attachment 8482971 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xul/content/test/chrome.ini @@ +1,5 @@ > [DEFAULT] > support-files = > 398289-resource.xul > file_bug236853.rdf > + doc.html Nit: perhaps give this a better name, such as 1061864.html. ::: content/xul/content/test/test_bug1061864_1.xul @@ +18,5 @@ > + > + function RunTest() > + { > + var parentIframe = document.createElement("iframe"); > + var frameLoaderOwner = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner); FWIW, this QueryInterface should be no-op, so you can just remove it I think. @@ +22,5 @@ > + var frameLoaderOwner = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner); > + frameLoaderOwner.setInPrerendering(); > + parentIframe.onload = function() { > + var docShell = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell; > + ok (docShell.isPrerendered == 1, "this passes"); Nit: ok(docShell.isPrerendered, "The docshell should be in prerendering mode"); ::: content/xul/content/test/test_bug1061864_2.xul @@ +23,5 @@ > + frameLoaderOwner.setInPrerendering(); > + parentIframe.setAttribute("src", "doc.html"); > + parentIframe.onload = function() { > + var childIframe = parentIframe.contentDocument.getElementById("childiframe"); > + console.log(childIframe); Remove this please. @@ +24,5 @@ > + parentIframe.setAttribute("src", "doc.html"); > + parentIframe.onload = function() { > + var childIframe = parentIframe.contentDocument.getElementById("childiframe"); > + console.log(childIframe); > + var childDocShell = childIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell; Again, the QIs should be no-op I think. @@ +25,5 @@ > + parentIframe.onload = function() { > + var childIframe = parentIframe.contentDocument.getElementById("childiframe"); > + console.log(childIframe); > + var childDocShell = childIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell; > + ok (childDocShell.isPrerendered == 1, "this passes"); Ditto. ::: content/xul/content/test/test_bug1061864_3.xul @@ +22,5 @@ > + var frameLoaderOwner = parentIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner); > + parentIframe.setAttribute("src", "doc.html"); > + parentIframe.onload = function() { > + var childIframe = parentIframe.contentDocument.getElementById("childiframe"); > + console.log(childIframe); Ditto. @@ +24,5 @@ > + parentIframe.onload = function() { > + var childIframe = parentIframe.contentDocument.getElementById("childiframe"); > + console.log(childIframe); > + var childDocShell = childIframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell; > + ok (childDocShell.isPrerendered == 0, "this passes"); Ditto. (Note that boolean IDL attributes map to JS true/false values.)

Comment on attachment 8484547 [details] [diff] [review] 1061864p1.patch >+ if (NS_SUCCEEDED(parentAsDocShell->GetIsPrerendered(&value))) >+ { >+ if(value) >+ { >+ SetInPrerendering(); >+ } >+ } > if (NS_FAILED(parentAsDocShell->GetAllowDNSPrefetch(&value))) { > value = false; > } Please keep { in the same line as 'if' >@@ -613,16 +613,23 @@ interface nsIDocShell : nsIDocShellTreeI > /** > * Sets whether a docshell is active. An active docshell is one that is > * visible, and thus is not a good candidate for certain optimizations > * like image frame discarding. Docshells are active unless told otherwise. > */ > attribute boolean isActive; > > /** >+ * Puts the docshell in prerendering mode. noscript because we want only >+ * native code to be able to put a docshell in prerendering. >+ */ >+ [noscript] void SetInPrerendering(); >+ readonly attribute boolean isPrerendered; >+ >+ /** I don't quite understand the terminology. In C++ setter is SetInPrerendering but getter is GetIsPrerendered. The member variable is mIsPrerendered. Why the difference In..ing vs. Is...ed ?

Also, what is the plan there. Will mIsPrerendered stay true once it is set to true? But don't we want to move prerendered docshells to be visible at some, at which point I'd assume mIsPrerendered should become false again.

Comment on attachment 8484549 [details] [diff] [review] 1061864p2.patch > /** >+ * Puts the frameloader in prerendering mode. >+ */ >+ void setInPrerendering(); Again the same terminology question. What is the difference between In..ing vs Is..ed >+[scriptable, uuid(c4abebcf-55f3-47d4-af15-151311971255)] > interface nsIFrameLoaderOwner : nsISupports > { > /** > * The frame loader owned by this nsIFrameLoaderOwner > */ > readonly attribute nsIFrameLoader frameLoader; > [noscript, notxpcom] alreadyAddRefed_nsFrameLoader GetFrameLoader(); > > /** >+ * Puts the FrameLoaderOwner in prerendering mode. >+ */ >+ void setInPrerendering(); ditto >+ if(mIsPrerendered) >+ { >+ mFrameLoader->SetInPrerendering(); And here again. Member variable talks about IsPrerendered, but we then call SetInPrerendering. It is possible that the terminology issue needs just some explanation. > nsElementFrameLoaderOwner(mozilla::dom::FromParser aFromParser) > : mNetworkCreated(aFromParser == mozilla::dom::FROM_PARSER_NETWORK) >+ , mIsPrerendered(false) > , mBrowserFrameListenersRegistered(false) > , mFrameLoaderCreationDisallowed(false) > { > } > > virtual ~nsElementFrameLoaderOwner(); > > NS_DECL_NSIFRAMELOADEROWNER >@@ -65,13 +66,14 @@ protected: > > /** > * True when the element is created by the parser using the > * NS_FROM_PARSER_NETWORK flag. > * If the element is modified, it may lose the flag. > */ > bool mNetworkCreated; > >+ bool mIsPrerendered; So, this is fine since there aren't too many nsElementFrameLoaderOwner objects. (html:iframes in general) >+++ b/content/xul/content/src/nsXULElement.h ... >+ bool mIsPrerendered; But this is not ok. We don't want to increase memory usage of each XUL element 4 or 8 bytes. Which iframe/browser element do we want to set prerendered btw? only xul:browser for tabs? Couldn't we just use then some attribute as a flag?

Comment on attachment 8484550 [details] [diff] [review] 1061864p3.patch So this might need some updates based on what happens to the other patches.

(In reply to Olli Pettay [:smaug] from comment #11) > Also, what is the plan there. Will mIsPrerendered stay true once it is set > to true? Yes. > But don't we want to move prerendered docshells to be visible at some, at > which point I'd > assume mIsPrerendered should become false again. No, that will happen by swapping the frameloaders.

(In reply to Olli Pettay [:smaug] from comment #12) > >+++ b/content/xul/content/src/nsXULElement.h > ... > >+ bool mIsPrerendered; > But this is not ok. We don't want to increase memory usage of each XUL > element 4 or 8 bytes. > > Which iframe/browser element do we want to set prerendered btw? only > xul:browser for tabs? Yes. > Couldn't we just use then some attribute as a flag? We could I suppose, but I would like to avoid that if possible, since the attribute would be visible on the content node... Are you fine with making mBindingParent a tagged pointer and use its lowest bit here?

content code can't use xul:browser. in XULElement couldn't we move the flag to slots? We will have slots anyway in case there is an element which may create frameloader.

(In reply to Olli Pettay [:smaug] from comment #16) > content code can't use xul:browser. I was thinking of add-ons. > in XULElement couldn't we move the flag to slots? We will have slots anyway > in case there is > an element which may create frameloader. Yes, you're right. We should be able to put it on the slot, I think.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on Thu/Fri) from comment #14) > > But don't we want to move prerendered docshells to be visible at some, at > > which point I'd > > assume mIsPrerendered should become false again. > > No, that will happen by swapping the frameloaders. I don't understand this. Frameloader owns the docshell. What does swapping help here?

Comment on attachment 8491922 [details] [diff] [review] 1061864p1.patch So this is ok for initial state, but as far as I see, we'll need to have some way to clear mIsPrerendered. Another patch for that?

Comment on attachment 8491924 [details] [diff] [review] 1061864p2.patch >+ if (mIsPrerendered) >+ { { should be in the previous line >+ if (mIsPrerendered) >+ { ditto >+nsXULElement::SetIsPrerendered() >+{ >+ nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader(); >+ MOZ_ASSERT(!frameLoader,"Please call SetIsPrerendered before frameLoader is created"); >+ nsXULSlots *slots= static_cast<nsXULSlots*>(Slots()); >+ if (slots) { >+ slots->mIsPrerendered = true; >+ } >+ return NS_OK; >+} Since this method can be called from normal chrome JS, I'd prefer not having MOZ_ASSERT, but throw some exception (INVALID_STATE or some such) in case there is frameLoader already. >+ nsresult SetIsPrerendered(); This method could be void >+ [ChromeOnly] >+ void setIsPrerendered(); So this could use Throws ...but. I think we should just some some dom attribute as a flag. This is all about chrome js/XUL. Addons can anyway see all the properties, [ChromeOnly] doesn't make it non-addon thing.

Comment on attachment 8491926 [details] [diff] [review] 1061864p3.patch So this would need to be updated for the dom attribute approach. The main reason I prefer dom attribute over an explicit flag is memory consumption.

Discussed with Ehsan on IRC and we do need to have a way to set a docshell non-prerendered. Prerendered flag would be cleared during frameloader swapping.

Comment on attachment 8544163 [details] [diff] [review] Part 2: Add a prerendered flag to nsIFrameLoader and nsIFrameLoaderOwner This is for non-e10s only, but I guess that is fine for now.

https://hg.mozilla.org/mozilla-central/rev/9ea0c21e8e33 https://hg.mozilla.org/mozilla-central/rev/75a20fb5f864 https://hg.mozilla.org/mozilla-central/rev/d07e602fd385