Closed Bug 1315105 Opened 8 years ago Closed 8 years ago

Implement Framework for <link rel=prerender>

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: nika, Assigned: nika)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 20 obsolete files)

Part 1: Add support for prerendering PartialSHistories to GroupedSHistory,
27.89 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
Part 2: Implement <link rel=prerender> behind a pref,
54.62 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
Part 4: Add a test for basic prerendering functionality,
6.58 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
Part 3: Update close_dependent_tabs / grouped_shistory_crossproc test for new prerendering behavior
6.29 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
This is a bug for an initial framework implementation of support for <link rel=prerender> behind a preference, which can be used as an initial building block upon which full prerendering support and other prerendering features can be built and tested.
This is a super early version of what I was imagining in the etherpad as an architecture for our initial <link rel=prerender> implementation. I'd appreciate some feedback as to whether you think the basic architecture makes sense, and worries which you have about how well it will support the things we want to do with prerendering.
Attachment #8807336 - Flags: feedback?(sawang)
Depends on: 1314792
Comment on attachment 8807336 [details] [diff] [review] Implement the basic framework for <link rel=prerender> behind a pref Review of attachment 8807336 [details] [diff] [review]: ----------------------------------------------------------------- Yup I think it's a good start for us. Now I'm nervous that I don't have any progress yet on my side :P ::: browser/base/content/tab-content.js @@ +651,5 @@ > + return; > + } > + > + let id = ++this.idMonotonic; > + sendAsyncMessage("Prerender:Start", { I would prefer to make both the function and messsage name to indicate that it may not start immediately and may not start at all. "Prerender:Request", maybe? BTW our proposal was to start with at most one prerendered page globally (rather than per-document). Not sure if it really makes any difference for our experiments. ::: browser/base/content/tabbrowser.xml @@ +2784,5 @@ > + if (!aCurrentTab.childTabs) { > + aCurrentTab.childTabs = []; > + } > + aCurrentTab.childTabs.push(aPrerenderedTab); > + aPrerenderedTab.parentTab = aCurrentTab; Maybe we don't need this tab connection yet. We can implement it in another bug when we have more clear plan on that. @@ +2913,5 @@ > + <body> > + <![CDATA[ > + this._swapBrowserDocShells(aOurTab, aOtherBrowser); > + if (aOurTab.selected) > + this.updateCurrentBrowser(true); I remember I put this line so the titlebar and favicon would be updated but somehow not working anymore... @@ +4764,5 @@ > + dump("[prerender] skip prerender on hidden tab\n"); > + break; > + } > + > + if (browser.canGoForward || browser.skipPrerender) { I forgot why I have browser.skipPrerender here... I think we should remove it for now. ::: browser/modules/ContentLinkHandler.jsm @@ +146,5 @@ > + if (!link.ownerDocument.prerenderHref(BrowserUtils.makeURI(link.href))) { > + dump("[prerendering]: Prerendering failed for some reason which I will figure out later\n"); > + } > + } > + break; Since it's calling to the document, would it be better to put it to where prefetch / preconnect are handled [1][2]? [1] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/base/nsContentSink.cpp#693 [2] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/base/Link.cpp#79 ::: docshell/base/nsDocShell.cpp @@ +13870,5 @@ > + > + rv = browserChrome3->SwitchToPrerenderedDocument(aURI, mCurrentURI, nullptr, ev); > + if (NS_FAILED(rv)) { > + return NS_DispatchToCurrentThread(ev); > + } Sometime in the future we may move this to InternalLoad or so since we need to fire beforeunload event and check if user permits unload... It can be done after groupedshitory related bugs are all implemented so for now I think it's OK.
Attachment #8807336 - Flags: feedback?(sawang) → feedback+
This is a very initial version of prerendering based on samael's work with session history. The idea behind this patch is to land an initial implementation behind a pref which we can build on top of. The following are a subset of the known problems with the current implementation which will need to be fixed in other bugs before this is ready to be prefed on by default: * Does not interact correctly with Session History * Indefinitely leaks browser objects related to history * If a document is prerendered and not used, it is not cleaned up * browsers which are part of a prerendered document's history are never cleaned up * Browsers are not moved correctly when dragged between windows. * The tab icon and title are not updated correctly when a prerendered page is navigated to * Unload is not called correctly on the page which is being left when performing a prerender navigation * The visibility of a prerendering document is not set correctly * Prerendering is not canceled when a document performs a non-idepmotent action * Long press on the back button does not show the correct history * Prerendering is not canceled when the <link> element is removed * Doesn't cleanly handle situations where some browsers end up dying (say due to a content process crash). samael, it would be cool if you could take another look and let me know if you think this is in a state where we would want to try to land it behind a pref.
Attachment #8807714 - Flags: feedback?(sawang)
Attachment #8807336 - Attachment is obsolete: true
I've thrown together a quick patch which implements prerender on top of the above patch using tab switching instead of swapping of frameloaders. It also uses SessionStore to prime the new process with the current session history. It's not great, and it doesn't support bfcache (although I believe that with some work it could be made to), but i thought I should bring something like it up as an option. The advantage of this technique is that it could require changing less things which people expect not to break. Namely: 1. every process has the complete session history 2. Frameloaders inside of browsers aren't swapped without the frontend's knowledge This makes it really easy to do things like fall back to being process-locked (because, say, a new window was opened), which is nice. It also means that session history already works in the prototype (kinda - ignore the fact that we purple links too early and update places too early, which is a problem we will have to solve with any design). If we decided to work with a design similar to this one, I would want to start without worrying about how we will get the cross process bfcache working, and get the correctness down and working. We would need to start working on things like ensuring that places doesn't get updated until you actually visit the page, and that we correctly set up the state for prerendering pages, and that prerendering is correctly stopped when we are done. Once we got that correctness out of the way, we would then go back and figure out how to support bfcache to get it in shipping shape - I'm not sure exactly what that solution would end up looking like, but I am confident that we could get SessionStore to update the SHistory in the original process to be up to date, and then swap back to it. Olli and Samael, you two have thought a lot more about your session history work than I have. I know that this is not a good implementation by any means, but I want to make sure that we have considered every option, and this has always been the option that stood out to me as the most dead-simple wrt session history.
Attachment #8808335 - Flags: feedback?(sawang)
Attachment #8808335 - Flags: feedback?(bugs)
https://github.com/mystor/mozilla-central/tree/prerender <- This github branch has my current working tree for that idea. I'm going to keep looking into how to adapt SessionStore to work with GroupedSHistory, but I thought it might be worthwhile to throw this idea together.
Comment on attachment 8807714 [details] [diff] [review] Implement the basic framework for <link rel=prerender> behind a pref It looks what we want for now. BTW the process lock stuff is new to me. Would you give me some hints on that?
Attachment #8807714 - Flags: feedback?(sawang) → feedback+
Oh. So it's about window.opener breakage. Umm... yes keep complete history list in each process would solve this issue.
Olli mentioned about potential issue of docshellIDs with session store before so I tried random navigations between web content and about:config, then checked docshellIDs in root session history entries. There were 5 root entries and the docshellIDs were 1, 8, 1, 9, 1 respectively while mRootDocShell->mHistoryID was 10. So docshellIDs were messed up after session restore. I don't know how bad it indicates and I have no idea why it's not causing problems now...(or it is?)
Docshell IDs were causing issues at least to iframe handling, IIRC. And for consistency we should just make them work correctly. Updating the top level docshell ID should be easy.
(In reply to Samael Wang [:freesamael][:sawang] from comment #6) > It looks what we want for now. BTW the process lock stuff is new to me. > Would you give me some hints on that? The process lock stuff is stuff which I added for the Large-Allocation header. It checks that there are no other content documents which could have a reference to this document which would prevent it from crossing process boundaries. I decided to factor it out of the logic for the Large-Allocation header so we could use it for prerendering as well.
(In reply to Olli Pettay [:smaug] from comment #9) > Docshell IDs were causing issues at least to iframe handling, IIRC. And for > consistency we should just > make them work correctly. Updating the top level docshell ID should be easy. I'll file a bug to fix that behavior and fix it in the session restore logic. I feel like it is unnecessary to store the docshell for toplevel session history entries, so I will probably simply avoid doing that.
Attachment #8808335 - Attachment is obsolete: true
Attachment #8808335 - Flags: feedback?(sawang)
Attachment #8808335 - Flags: feedback?(bugs)
> ::: docshell/base/nsDocShell.cpp > @@ +13870,5 @@ > > + > > + rv = browserChrome3->SwitchToPrerenderedDocument(aURI, mCurrentURI, nullptr, ev); > > + if (NS_FAILED(rv)) { > > + return NS_DispatchToCurrentThread(ev); > > + } > > Sometime in the future we may move this to InternalLoad or so since we need > to fire beforeunload event and check if user permits unload... Just a note for follow-up. I'm thinking this [1] is a possible place we'll eventually want to invoke SwitchToPrerenderedDocument from. Right after PermitUnload checked and NotifyUnloadAccepted logged, and before actually trying to stop the document and save presentation. The open question is what should we put as aFailure callback to make it resume from the place... [1] http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/docshell/base/nsDocShell.cpp#10504
Hey Michael, This is a possible solution to address comment 12. It's a patch based on your patch (I built it on top of your github prerender branch) + bug 1310768. It should be able to handle onbeforeunload case. It would be great if we can merge and land a patch for this bug before leaving Hawaii.
Depends on: 1320997
(In reply to Samael Wang [:freesamael][:sawang] from comment #13) > Created attachment 8815187 [details] [diff] [review] > Invoke SwitchToPrerenderedDocument from InternalLoad > > Hey Michael, > > This is a possible solution to address comment 12. It's a patch based on > your patch (I built it on top of your github prerender branch) + bug > 1310768. It should be able to handle onbeforeunload case. > > It would be great if we can merge and land a patch for this bug before > leaving Hawaii. I agree that that would be excellent. I'm working on trying to polish this up and write some tests for it today. The code is pretty buggy and needs some improvements so that's what I'm handling first. Thanks for the about:blank content viewer stuff too.
MozReview-Commit-ID: A5bwSy8NkH3
Attachment #8816269 - Flags: review?(bugs)
Attachment #8816269 - Flags: feedback?(sawang)
MozReview-Commit-ID: ARET98o1FTU
Attachment #8816270 - Flags: review?(bugs)
Attachment #8816270 - Flags: feedback?(sawang)
MozReview-Commit-ID: Ic0CWUbLPuq
Attachment #8816271 - Flags: review?(ehsan)
MozReview-Commit-ID: JYIpE8aeJRX
Attachment #8816272 - Flags: review?(bugs)
Attachment #8807714 - Attachment is obsolete: true
I didn't fold in your patch for firing the event in a different location, but I believe that that patch should still apply on top of the patches which I wrote. I can fold it in later if we decide that that is important.
Comment on attachment 8816272 [details] [diff] [review] Part 4(v1): Add a test for basic prerendering functionality >+ is(gBrowser.tabs.length, 3); >+ >+ let closed = new Promise(resolve => { >+ let seen = 0; >+ gBrowser.tabs.forEach(tab => { >+ tab.addEventListener("TabClose", function f() { >+ tab.removeEventListener("TabClose", f); >+ ++seen; >+ if (seen == 2) { >+ ok(true, "The tab was closed"); >+ resolve(); >+ } >+ }); >+ }); >+ }); I don't understand this part. You add event listener to 3 tabs, but wait for only 2 to be closed and then resolve. Doesn't this possibly cause a leak or issues in the following tests? Shouldn't you just add event listener to gBrowser and once you've got the expected events remove it. >+ yield Promise.all([ >+ BrowserTestUtils.browserLoaded(tab.linkedBrowser), >+ new Promise(resolve => { >+ let seen = false; >+ gBrowser.tabs.forEach(tab => { >+ tab.addEventListener("TabClose", function f() { >+ tab.removeEventListener("TabClose", f); >+ if (!seen) { >+ seen = true; >+ ok(true, "The tab was closed"); >+ resolve(); >+ } >+ }); >+ }); >+ }), >+ ]); This might have similar issue. Hard to say
Attachment #8816272 - Flags: review?(bugs) → review-
Attachment #8816271 - Flags: review?(ehsan) → review+
Comment on attachment 8816269 [details] [diff] [review] Part 1(v1): Add support for prerendering PartialSHistories to GroupedSHistory Could you Samael do the first review and I could do then a second one.
Attachment #8816269 - Flags: review?(sawang)
Attachment #8816269 - Flags: review?(bugs)
Attachment #8816269 - Flags: feedback?(sawang)
Comment on attachment 8816270 [details] [diff] [review] Part 2(v1): Implement <link rel=prerender> behind a pref Same with this
Attachment #8816270 - Flags: review?(sawang)
Attachment #8816270 - Flags: review?(bugs)
Attachment #8816270 - Flags: feedback?(sawang)
(In reply to Olli Pettay [:smaug] from comment #20) > I don't understand this part. You add event listener to 3 tabs, but wait for > only 2 to be closed and then resolve. > Doesn't this possibly cause a leak or issues in the following tests? > Shouldn't you just add event listener to gBrowser and once you've got the > expected events remove it. When a mochitest browser test is running, the test is started with a single tabbrowser open with a single about:blank tab. The tests I am running do not modify or close that single about:blank tab, so I only needed to add listeners for the visible one and the hidden prerendering tab. I don't remember why I decided to attach the listener to each of the tabs, but I seem to remember the "TabClose" event not making it all of the way up to the gBrowser... I can try again.
Comment on attachment 8816269 [details] [diff] [review] Part 1(v1): Add support for prerendering PartialSHistories to GroupedSHistory Review of attachment 8816269 [details] [diff] [review]: ----------------------------------------------------------------- My original idea was to build prerendering as a separate feature on top of the groupedshistory, but the proposed design here, which makes prerendering entries as potential future managed by groupedshistory also looks good to me, with only few comments below. You're much more familiar on the cycle collection stuff then me so I'll let smaug to review that part. ::: docshell/shistory/nsIPartialSHistory.idl @@ +30,5 @@ > > + // The groupedSHistory which this partialSHistory is a part of, or null. > + readonly attribute nsIGroupedSHistory groupedSHistory; > + > + // True if this frameloader is active I think the comment doesn't reflect the constants here. ::: dom/base/GroupedSHistory.cpp @@ +274,5 @@ > + > +NS_IMETHODIMP > +GroupedSHistory::GetActiveFrameLoader(nsIFrameLoader** aFrameLoader) > +{ > + return mPartialHistories[mIndexOfActivePartialHistory]->GetOwnerFrameLoader(aFrameLoader); Could we add a check to ensure mIndexOfActivePartialHistory >= 0 here just in case? @@ +294,5 @@ > + NS_ENSURE_TRUE(fl, NS_ERROR_FAILURE); > + > + nsCOMPtr<nsIFrameLoader> activeFl; > + GetActiveFrameLoader(getter_AddRefs(activeFl)); > + NS_ENSURE_TRUE(fl, NS_ERROR_FAILURE); nit: activeFl @@ +299,5 @@ > + > + nsresult rv = fl->MakePrerenderedLoaderActive(); > + NS_ENSURE_SUCCESS(rv, rv); > + > + return activeFl->AppendPartialSessionHistoryAndSwap(fl, aPromise); I'm not sure what would happen if the just activated frameloader operating on session history before being attached to the grouped history. We may need to think about it.
Attachment #8816269 - Flags: review?(sawang)
Attachment #8816269 - Flags: review?(bugs)
Attachment #8816269 - Flags: review+
Comment on attachment 8816270 [details] [diff] [review] Part 2(v1): Implement <link rel=prerender> behind a pref Review of attachment 8816270 [details] [diff] [review]: ----------------------------------------------------------------- Could you address the comments before passing to smaug? ::: browser/base/content/tab-content.js @@ +645,5 @@ > + }, > + > + startPrerenderingDocument(aHref, aReferrer) { > + // XXX: Make this constant a pref > + if (this.pending.length >= 2) { At some point we'll want to discard old requests rather then block new ones, but it's OK for now. @@ +680,5 @@ > + return; // Don't run the failure handler yet > + } > + } > + > + setTimeout(() => aFailure.run(), 0); Suggest to add a null check here. @@ +716,5 @@ > return true; > + }, > + > + startPrerenderingDocument: function(aHref, aReferrer) { > + PrerenderContentHandler.startPrerenderingDocument(aHref, aReferrer); Could we check if PrerenderContentHandler is actually initialized here (or check if it's in content process), just to prevent abuse? @@ +720,5 @@ > + PrerenderContentHandler.startPrerenderingDocument(aHref, aReferrer); > + }, > + > + switchToPrerenderedDocument: function(aHref, aReferrer, aSuccess, aFailure) { > + PrerenderContentHandler.switchToPrerenderedDocument(aHref, aReferrer, aSuccess, aFailure); Same here. ::: browser/base/content/tabbrowser.xml @@ +2813,5 @@ > ]]> > </body> > </method> > > + <method name="adoptPrerenderedBrowser"> We no longer need this method with your design. @@ +4847,5 @@ > + relatedToCurrent: true, > + isPrerendered: true, > + }); > + let partialSHistory = newTab.linkedBrowser.frameLoader.partialSessionHistory; > + groupedSHistory.addPrerenderingPartialSHistory(partialSHistory, data.id); Could we add a check for data.href and data.id? @@ +4854,5 @@ > + > + case "Prerender:Cancel": { > + let groupedSHistory = browser.frameLoader.groupedSessionHistory; > + if (groupedSHistory) { > + groupedSHistory.cancelPrerendering(data.id); Same here. @@ +4863,5 @@ > + case "Prerender:Swap": { > + let frameloader = browser.frameLoader; > + let groupedSHistory = browser.frameLoader.groupedSessionHistory; > + if (groupedSHistory) { > + groupedSHistory.activatePrerendering(data.id).then( And here. ::: dom/base/nsDocument.cpp @@ +2941,5 @@ > + NS_ENSURE_TRUE(webNav, false); > + > + bool canGoForward = false; > + nsresult rv = webNav->GetCanGoForward(&canGoForward); > + if (NS_FAILED(rv) || canGoForward) { It's also checked in tabbrowser.xml so maybe not necessary to check here. We might actually want to support this case in the future. Chrome does. But GroupedSHistory doesn't support this use case yet. ::: toolkit/content/widgets/browser.xml @@ +306,5 @@ > // Only useful for remote browsers. > </body> > </method> > > + <method name="adoptPrerenderedBrowser"> We no longer need this method with your design. @@ +325,1 @@ > <method name="makePrerenderedBrowserActive"> Same as this method, but the 2 test cases will need to update: dom/xul/test/file_bug1069772.xul dom/xul/test/file_bug1271240.xul
Attachment #8816270 - Flags: review?(sawang) → review-
Comment on attachment 8816270 [details] [diff] [review] Part 2(v1): Implement <link rel=prerender> behind a pref Review of attachment 8816270 [details] [diff] [review]: ----------------------------------------------------------------- I somehow missed this piece in previous comment... ::: browser/base/content/tab-content.js @@ +623,5 @@ > + if (this.pending[i].failure) { > + this.pending[i].failure.run(); > + } > + // Remove the item from the array > + this.pending.splice(i, 1); Would it be better to just break the loop after splice? I firstly thought the loop was incorrect as the original i+1 item would be skipped due to index change, but later realized there should be only one match anyway (right?).
MozReview-Commit-ID: A5bwSy8NkH3
Attachment #8817442 - Flags: review?(bugs)
Attachment #8816269 - Attachment is obsolete: true
Attachment #8816270 - Attachment is obsolete: true
Attachment #8816269 - Flags: review?(bugs)
MozReview-Commit-ID: ARET98o1FTU
Attachment #8817443 - Flags: review?(bugs)
Comment on attachment 8817442 [details] [diff] [review] Part 1(v2): Add support for prerendering PartialSHistories to GroupedSHistory >+ // All nsIPartialSHistories which are being prerendered. >+ struct PrerenderingHistory { Nit, { goes to its own line > // Destroy the other frame loader owners now that we are being destroyed. >- if (mGroupedSessionHistory) { >- mGroupedSessionHistory->CloseInactiveFrameLoaderOwners(); >+ if (mPartialSessionHistory && >+ mPartialSessionHistory->GetActiveState() == nsIPartialSHistory::STATE_ACTIVE) { >+ nsCOMPtr<nsIGroupedSHistory> groupedSHistory; >+ GetGroupedSessionHistory(getter_AddRefs(groupedSHistory)); >+ if (groupedSHistory) { >+ NS_DispatchToCurrentThread(NS_NewRunnableFunction([groupedSHistory] () { >+ groupedSHistory->CloseInactiveFrameLoaderOwners(); >+ })); >+ } Why a runnable is needed here? This needs a comment. (Also wondering why you chose using RunnableFunction and not RunnableMethod, but shouldn't matter. Hard to say which one is easier to read)
Attachment #8817442 - Flags: review?(bugs) → review+
Comment on attachment 8817443 [details] [diff] [review] Part 2(v2): Implement <link rel=prerender> behind a pref >+let PrerenderContentHandler = { >+ init() { >+ this.pending = []; >+ this.idMonotonic = 0; >+ this.initialized = true; Shouldn't the member variables in JS objects have _ prefix. Some browser/toolkit peer might have an opinion. >+ case "Prerender:Request": { >+ let sendCancelPrerendering = () => { >+ browser.frameloader.messageManager. >+ sendAsyncMessage("Prerender:Canceled", { id: data.id }); >+ }; >+ >+ let tab = this.getTabForBrowser(browser); >+ if (!tab) { >+ dump("[prerender] no tab?\n"); >+ sendCancelPrerendering(); >+ break; >+ } >+ >+ if (tab.hidden) { >+ dump("[prerender] skip prerender on hidden tab\n"); >+ sendCancelPrerendering(); >+ break; >+ } >+ >+ if (browser.canGoForward) { I don't understand this check. Doesn't this break prerendering in cases you go back to previous page and add new prerender links dynamically? (which is something I assume search engines do) >+ dump("[prerender] skip prerender on history navigation\n"); >+ sendCancelPrerendering(); >+ break; >+ } >+ >+ if (!data.href) { >+ dump("[prerender] bad request\n"); >+ sendCancelPrerendering(); >+ break; >+ } >+ >+ let groupedSHistory = browser.frameLoader.ensureGroupedSHistory(); >+ >+ let newTab = this.loadOneTab(data.href, { >+ referrerURI: (data.referrer ? makeURI(data.referrer) : null), >+ referrerPolicy: Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT, >+ postData: null, >+ allowThirdPartyFixup: true, >+ relatedToCurrent: true, >+ isPrerendered: true, >+ }); >+ let partialSHistory = newTab.linkedBrowser.frameLoader.partialSessionHistory; >+ groupedSHistory.addPrerenderingPartialSHistory(partialSHistory, data.id); >+ break; >+ } please remove dump()s above >+++ b/docshell/base/nsDocShell.cpp >@@ -13887,16 +13887,26 @@ nsDocShell::OnLinkClick(nsIContent* aContent, > if (NS_FAILED(rv)) { > target = aTargetSpec; > } > > nsCOMPtr<nsIRunnable> ev = > new OnLinkClickEvent(this, aContent, aURI, target.get(), aFileName, > aPostDataStream, aHeadersDataStream, noOpenerImplied, > aIsTrusted); >+ >+ // We don't want to try to prerender on a targeted link. >+ // XXX: In the future we may want to consider doing this more intellegently. intelligently >+ if (browserChrome3 && target.IsEmpty()) { >+ rv = browserChrome3->SwitchToPrerenderedDocument(aURI, mCurrentURI, nullptr, ev); >+ if (NS_SUCCEEDED(rv)) { >+ return NS_OK; >+ } So it is rather nasty to hide the asynchronousness to SwitchToPrerenderedDocument. Could you rather have a runnable which calls SwitchToPrerenderedDocument and it does its stuff synchronously and if that fails, OnLinkClickEvent's Run is called synchronously. >+++ b/dom/base/Link.cpp >@@ -123,16 +123,24 @@ Link::TryDNSPrefetchPreconnectOrPrefetch() > nsCOMPtr<nsIURI> uri(GetURI()); > if (uri && mElement->OwnerDoc()) { > mElement->OwnerDoc()->MaybePreconnect(uri, > mElement->AttrValueToCORSMode(mElement->GetParsedAttr(nsGkAtoms::crossorigin))); > return; > } > } > >+ if (linkTypes & nsStyleLinkElement::ePRERENDER) { >+ nsCOMPtr<nsIURI> uri(GetURI()); >+ if (uri && mElement->OwnerDoc()) { >+ mElement->OwnerDoc()->PrerenderHref(uri); >+ return; >+ } >+ } Since you now make this method to handle yet another case, please rename the method. If no better new name comes to mind, just append OrPrerender >+ nsCOMPtr<nsIURI> referrer = GetDocumentURI(); >+ bool urisMatch = false; >+ aHref->Equals(referrer, &urisMatch); >+ if (urisMatch) { >+ // NOTE: Cannot preload the current document >+ return false; >+ } Better comment please. Why current document can't be preloaded? Is that because we aren't in general supposed to load a new page but just do fragment navigation. But then, don't you want EqualsExceptRef, not Equals check >+ bool canGoForward = false; >+ nsresult rv = webNav->GetCanGoForward(&canGoForward); >+ if (NS_FAILED(rv) || canGoForward) { >+ // NOTE: Preloading doesn't occur within history entries. >+ return false; >+ } This is still mystery to me, same as in tabbrowser code. Why this check. >+ >+ if (docShell->GetProcessLockReason() != nsIDocShell::PROCESS_LOCK_NONE) { >+ return false; >+ } This needs a comment. All this "ProcessLock" is oddly named so one needs to always check what it is about. >+++ b/dom/webidl/Document.webidl >@@ -217,16 +217,19 @@ partial interface Document { > > /** > * Current referrer policy - one of the REFERRER_POLICY_* constants > * from nsIHttpChannel. > */ > [ChromeOnly] > readonly attribute unsigned long referrerPolicy; > >+ [ChromeOnly] >+ boolean prerenderHref(URI aHref); Why this change? Why we need anything in .webidl? The method is being called in C++ only, at least in this patch. Does some test need this? >+ /** >+ * Tell the browser to start prerendering the given document. This prerendering >+ * _must_ be for the toplevel document. >+ * >+ * @param aHref The URI to begin prerendering >+ * @param aReferrer The URI of the document requesting the prerender. >+ */ >+ void startPrerenderingDocument(in nsIURI aHref, in nsIURI aReferrer); >+ >+ /** >+ * Switch to a prerendered document, if it is avaliable. available I could take another look after the patch has been updated.
Attachment #8817443 - Flags: review?(bugs) → review-
Attachment #8816269 - Attachment description: Part 1: Add support for prerendering PartialSHistories to GroupedSHistory → Part 1(v1): Add support for prerendering PartialSHistories to GroupedSHistory
Attachment #8816270 - Attachment description: Part 2: Implement <link rel=prerender> behind a pref → Part 2(v1): Implement <link rel=prerender> behind a pref
Attachment #8816271 - Attachment description: Part 3: Update close_dependent_tabs test for new prerendering behavior → Part 3(v1): Update close_dependent_tabs test for new prerendering behavior
Attachment #8816272 - Attachment description: Part 4: Add a test for basic prerendering functionality → Part 4(v1): Add a test for basic prerendering functionality
Attachment #8817442 - Attachment description: Part 1: Add support for prerendering PartialSHistories to GroupedSHistory → Part 1(v2): Add support for prerendering PartialSHistories to GroupedSHistory
Attachment #8817442 - Attachment is obsolete: true
Attachment #8817443 - Attachment description: Part 2: Implement <link rel=prerender> behind a pref → Part 2(v2): Implement <link rel=prerender> behind a pref
Attachment #8817443 - Attachment is obsolete: true
Attachment #8816272 - Attachment is obsolete: true
Attachment #8816271 - Attachment is obsolete: true
Attachment #8818825 - Attachment description: Part 2: Implement <link rel=prerender> behind a pref, → Part 2(v2-rebased): Implement <link rel=prerender> behind a pref,
Attachment #8818825 - Attachment is obsolete: true
Attachment #8818826 - Attachment description: Part 3: Update close_dependent_tabs test for new prerendering behavior, → Part 3(v1-rebased): Update close_dependent_tabs test for new prerendering behavior,
Attachment #8818826 - Attachment is obsolete: true
Attachment #8818827 - Attachment description: Part 4: Add a test for basic prerendering functionality, → Part 4(v1-rebased): Add a test for basic prerendering functionality,
Attachment #8818827 - Attachment is obsolete: true
Attachment #8815187 - Attachment is obsolete: true
Attachment #8818854 - Attachment description: Part 2: Implement <link rel=prerender> behind a pref, → Part 2(v3): Implement <link rel=prerender> behind a pref,
Attachment #8818854 - Attachment is obsolete: true
Attachment #8819208 - Attachment description: Part 2: Implement <link rel=prerender> behind a pref, r=smaug → Part 2(v4): Implement <link rel=prerender> behind a pref, r=smaug
Attachment #8819208 - Attachment is obsolete: true
Comment on attachment 8819211 [details] [diff] [review] Part 2: Implement <link rel=prerender> behind a pref, r=smaug (In reply to Olli Pettay [:smaug] from comment #30) > Comment on attachment 8817443 [details] [diff] [review] > Part 2(v2): Implement <link rel=prerender> behind a pref Michael and I are desperate to land this so I would try to push the progress as much as I can. I rebuilt all related bugs on my local branch, which means the patches are rebased. You can interdiff with attachment 8818825 [details] [diff] [review] instead. It's exactly the same patch in previous review, but rebased. That should generate a more readable interdiff for review. > >+ bool canGoForward = false; > >+ nsresult rv = webNav->GetCanGoForward(&canGoForward); > >+ if (NS_FAILED(rv) || canGoForward) { > >+ // NOTE: Preloading doesn't occur within history entries. > >+ return false; > >+ } > This is still mystery to me, same as in tabbrowser code. Why this check. GroupedSHistory doesn't support it yet. In this case we'd want previous partial history remove some entries when appending another partial history, and the offset of the appending partial history should based on the entry index. I filed bug 1323650 for that, and added comment to point to that bug. > >+ if (browserChrome3 && target.IsEmpty()) { > >+ rv = browserChrome3->SwitchToPrerenderedDocument(aURI, mCurrentURI, nullptr, ev); > >+ if (NS_SUCCEEDED(rv)) { > >+ return NS_OK; > >+ } > So it is rather nasty to hide the asynchronousness to > SwitchToPrerenderedDocument. > Could you rather have a runnable which calls SwitchToPrerenderedDocument and > it does its stuff synchronously and if that fails, > OnLinkClickEvent's Run is called synchronously. I think Micheal and I were both considering the asynchronous nature of SwitchToPrerenderedDocument - the failure callback can be pended and invoked until parent process notifies that the swapping fails. So we think making the immediate failure also being asynchronous makes sense. However, one issue which stops us from merging attachment 8815187 [details] [diff] [review] (moving the call from OnLinkClick to InternalLoad) is that many test cases assume InternalLoad triggered by OnLinkClick is synchronous, and causes a lot of failures [1]. I'm changing it to ShouldSwitchToPrerenderedDocument which returns a boolean so that callers can continue synchronously if there wasn't a prerendered document for the link at all. I think with that I can move the call to InternalLoad. I'm not 100% sure it's the right place to call ShouldSwitchToPrerenderedDocument, but the idea is that the switch should happen after PermitUnload and user accepted to leave the page, and it should be before DocShell trying to save the presentation and stop current document. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=db358e438affb8354dc7ab750f50def45fc932f4
Attachment #8819211 - Flags: review?(bugs)
Comment on attachment 8819210 [details] [diff] [review] Part 4: Add a test for basic prerendering functionality, r=smaug >+function onBeforeUnloadDialogShown(node) { >+ dialogShown = true; >+ let dismissButton = node.ui.button0; >+ dismissButton.click(); >+} What is this new code in this version of the patch. I don't see anyone ever calling onBeforeUnloadDialogShown. Am I missing something here? The useless function removed or explained, r+
Attachment #8819210 - Flags: review?(bugs) → review+
Comment on attachment 8819211 [details] [diff] [review] Part 2: Implement <link rel=prerender> behind a pref, r=smaug >+ shouldSwitchToPrerenderedDocument(aHref, aReferrer, aSuccess, aFailure) { >+ // Check if we think there is a prerendering document pending for the given >+ // href and referrer. If we think there is one, we will send a message to >+ // the parent process asking it to do a swap, and hook up the success and >+ // failure listeners. >+ for (let i = 0; i < this._pending.length; ++i) { >+ let p = this._pending[i]; >+ if (p.href.equals(aHref) && p.referrer.equals(aReferrer)) Something to test and possibly file a followup bug. How do other browser deal with url which just different from the #fragment part here. Please file a followup bug about this issue > GroupedSHistory::GroupedHistoryEnabled() { >- return Preferences::GetBool("browser.groupedhistory.enabled", false); >+ static bool sPrerenderEnabled = false; >+ static bool sPrerenderPrefCached = false; >+ if (!sPrerenderPrefCached) { >+ sPrerenderPrefCached = true; >+ Preferences::AddBoolVarCache(&sPrerenderEnabled, >+ "browser.groupedhistory.enabled", >+ false); >+ } >+ >+ return sPrerenderEnabled; > } Hmm, the variable names are wrong here. The method is about browser.groupedhistory.enabled, not prerendering. There is separate pref for dom.linkPrerender.enabled >+ // Adopting an out-of-process prerendered document is conceptually similar to >+ // switching dochshell's process, since it's the same browsing context from >+ // other browsing contexts' perspective. If we're locked in current process, >+ // we can not prerender out-of-process. >+ if (docShell->GetIsProcessLocked()) { It is still rather unclear what "locked" means in this context. Could you please file a followup to rename "locked" to something understandable.
Attachment #8819211 - Flags: review?(bugs) → review+
Attachment #8819209 - Attachment is obsolete: true
Attachment #8819210 - Attachment is obsolete: true
Attachment #8819211 - Attachment is obsolete: true
Attachment #8818824 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #42) > Comment on attachment 8819211 [details] [diff] [review] > >+ shouldSwitchToPrerenderedDocument(aHref, aReferrer, aSuccess, aFailure) { > >+ // Check if we think there is a prerendering document pending for the given > >+ // href and referrer. If we think there is one, we will send a message to > >+ // the parent process asking it to do a swap, and hook up the success and > >+ // failure listeners. > >+ for (let i = 0; i < this._pending.length; ++i) { > >+ let p = this._pending[i]; > >+ if (p.href.equals(aHref) && p.referrer.equals(aReferrer)) > Something to test and possibly file a followup bug. How do other browser > deal with url which just different from the #fragment part here. > Please file a followup bug about this issue Filed bug 1324358 accordingly. > >+ // Adopting an out-of-process prerendered document is conceptually similar to > >+ // switching dochshell's process, since it's the same browsing context from > >+ // other browsing contexts' perspective. If we're locked in current process, > >+ // we can not prerender out-of-process. > >+ if (docShell->GetIsProcessLocked()) { > It is still rather unclear what "locked" means in this context. > Could you please file a followup to rename "locked" to something > understandable. And bug 1324359.
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6c012704b9 Part 1: Add support for prerendering PartialSHistories to GroupedSHistory, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/059753ec9117 Part 2: Implement <link rel=prerender> behind a pref, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1e59969729 Part 3: Update close_dependent_tabs test for new prerendering behavior, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/5414d6a71785 Part 4: Add a test for basic prerendering functionality, r=smaug
Keywords: checkin-needed
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b341032ba0 Backed out changeset 5414d6a71785 https://hg.mozilla.org/integration/mozilla-inbound/rev/c499b603d9bf Backed out changeset cb1e59969729 https://hg.mozilla.org/integration/mozilla-inbound/rev/be8630945ce9 Backed out changeset 059753ec9117 for test failures in own test
sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=40776997&repo=mozilla-inbound
Flags: needinfo?(michael)
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/636a1e09a0e4 Backed out changeset ab6c012704b9 for failing on own test
Looks the test case should wait for TabClose before leaving. I'll make an update and try.
Flags: needinfo?(michael)
Attachment #8819760 - Attachment is obsolete: true
Attachment #8820223 - Attachment description: Part 3: Update close_dependent_tabs / grouped_shistory_crossproc test for new prerendering behavior, → Part 3: Update close_dependent_tabs / grouped_shistory_crossproc test for new prerendering behavior
Attachment #8820223 - Flags: review+
Ran 20 times mochitest-bc without seeing browser_grouped_shistory_crossproc.js failure with updated patch. Others look like existing intermittent bugs. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8248b50d59be60d292f55e36993fb56c753f0b75&group_state=expanded&selectedJob=33085877
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/680af1b6873e Part 1: Add support for prerendering PartialSHistories to GroupedSHistory, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/7362b109b032 Part 2: Implement <link rel=prerender> behind a pref, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/7a472f22dc64 Part 3: Update close_dependent_tabs / grouped_shistory_crossproc test for new prerendering behavior, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/93999353954c Part 4: Add a test for basic prerendering functionality, r=smaug
Keywords: checkin-needed
Hooray! \o/
I've updated the browser support info for <link rel="prerender"> : https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types#Browser_compatibility And added an entry to our experimental features page: https://developer.mozilla.org/en-US/Firefox/Experimental_features#HTML Let me know if anything else is needed; thanks!
Keywords: dev-doc-needed → dev-doc-complete
Ack, I just saw bug 1383876. I've removed the experimental features note, and updated the footnote in here accordingly: https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types#Browser_compatibility
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: