Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7034043:
Prerender/Instant interopability. (Closed)
|
Created:
9 years, 7 months ago by dominich Modified:
9 years, 7 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrerender/Instant interopability.
BUG=82590
TEST=Use instant to navigate to a page that is currently being prerendered. Observe it does not crash.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86651
Patch Set 1 #
Total comments: 6
Patch Set 2 : set up the new preview contents correctly. #Patch Set 3 : Adding ShowPreview #Patch Set 4 : Cleanup and consolidate code #
Total comments: 10
Patch Set 5 : Move the delegate implementation and do more old tc cleanup. #Patch Set 6 : rebase #
Total comments: 10
Patch Set 7 : responding to comments #Patch Set 8 : Immediate swap called if we're ready. #Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : rebase #
Messages
Total messages: 30 (0 generated)
LGTM
LGTM But please check with @sky about possible effects (does the InstantLoader need to reset any state since its preview_contents has been changed? does it need to notify any delegates? etc).
On 2011/05/19 18:05:08, sreeram wrote: > LGTM > > But please check with @sky about possible effects (does the InstantLoader need > to reset any state since its preview_contents has been changed? does it need to > notify any delegates? etc). Added sky@ to reviewers. I looked in to what Instant does with the preview contents that it creates, and it shouldn't be necessary as we're swapping in a fully formed contents.
http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... chrome/browser/instant/instant_loader.cc:150: TabContentsWrapper* tab_contents_wrapper = preview_contents_.release(); ignore_result http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... chrome/browser/instant/instant_loader.cc:152: preview_contents_.reset(new_tc); I'm not sure under when this might be invoked. Couple of things that likely need to be done: . See the code in CreatePreviewContents. I think we need to do all that on the tabcontents. Be sure to remove TabContentsDelegateImpl as it is still installed on the old delegate. . I assume if this happens the page is ready to be shown. In which case invoke ShowPreview. . CreatePreviewContents makes sure to adjust the page ids. If swap is called what happens to page id of the new tabcontents? Are we sure the id is >= id of old? http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... chrome/browser/instant/instant_loader.h:81: TabContentsWrapper* new_tc); OVERRIDE
This fixes the crash, however there is an outstanding issue: The prerendered page is not displayed until the user commits. I'll try calling ShowPreview and see if that fixes it. (Ie, there may be another patch set to come). http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... chrome/browser/instant/instant_loader.cc:150: TabContentsWrapper* tab_contents_wrapper = preview_contents_.release(); On 2011/05/19 19:53:55, sky wrote: > ignore_result Done. http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... chrome/browser/instant/instant_loader.cc:152: preview_contents_.reset(new_tc); On 2011/05/19 19:53:55, sky wrote: > I'm not sure under when this might be invoked. It's invoked if the InstantLoader is the delegate for a TabContentsWrapper that is being swapped for a Prerender TabContents. This is called as a side-effect of the ProvisionalChangeToMainFrameUrl in the TabContentsObserver so shouldn't disturb the rest of the Instant flow. > Couple of things that likely need > to be done: > > . See the code in CreatePreviewContents. I think we need to do all that on the > tabcontents. Be sure to remove TabContentsDelegateImpl as it is still installed > on the old delegate. Done > . I assume if this happens the page is ready to be shown. In which case invoke > ShowPreview. This shouldn't be necessary as we're about to go through the Instant commit flow as normal. > . CreatePreviewContents makes sure to adjust the page ids. If swap is called > what happens to page id of the new tabcontents? Are we sure the id is >= id of > old? While the Prerender system has already increased the page id based on the source of the prerender, it can't hurt to make sure we're increasing it here too. http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... chrome/browser/instant/instant_loader.h:81: TabContentsWrapper* new_tc); On 2011/05/19 19:53:55, sky wrote: > OVERRIDE Done.
Adding ShowPreview() was the key. Thanks sky. This should be complete now.
On Thu, May 19, 2011 at 4:57 PM, <dominich@chromium.org> wrote: > This fixes the crash, however there is an outstanding issue: The prerendered > page is not displayed until the user commits. > > I'll try calling ShowPreview and see if that fixes it. (Ie, there may be > another > patch set to come). > > > http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... > File chrome/browser/instant/instant_loader.cc (right): > > http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... > chrome/browser/instant/instant_loader.cc:150: TabContentsWrapper* > tab_contents_wrapper = preview_contents_.release(); > On 2011/05/19 19:53:55, sky wrote: >> >> ignore_result > > Done. > > http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... > chrome/browser/instant/instant_loader.cc:152: > preview_contents_.reset(new_tc); > On 2011/05/19 19:53:55, sky wrote: >> >> I'm not sure under when this might be invoked. > > It's invoked if the InstantLoader is the delegate for a > TabContentsWrapper that is being swapped for a Prerender TabContents. > > This is called as a side-effect of the ProvisionalChangeToMainFrameUrl > in the TabContentsObserver so shouldn't disturb the rest of the Instant > flow. Can you give me a more concrete example. >> Couple of things that likely need >> to be done: > >> . See the code in CreatePreviewContents. I think we need to do all > > that on the >> >> tabcontents. Be sure to remove TabContentsDelegateImpl as it is still > > installed >> >> on the old delegate. > > Done > >> . I assume if this happens the page is ready to be shown. In which > > case invoke >> >> ShowPreview. > > This shouldn't be necessary as we're about to go through the Instant > commit flow as normal. > >> . CreatePreviewContents makes sure to adjust the page ids. If swap is > > called >> >> what happens to page id of the new tabcontents? Are we sure the id is >> = id of >> old? > > While the Prerender system has already increased the page id based on > the source of the prerender, it can't hurt to make sure we're increasing > it here too. Does that mean it will have honored the max page id of the TabContents created in InstantLoader::CreatePreviewContents? -Scott
On 2011/05/20 14:18:13, sky wrote: > On Thu, May 19, 2011 at 4:57 PM, <mailto:dominich@chromium.org> wrote: > > This fixes the crash, however there is an outstanding issue: The prerendered > > page is not displayed until the user commits. > > > > I'll try calling ShowPreview and see if that fixes it. (Ie, there may be > > another > > patch set to come). > > > > > > > http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... > > File chrome/browser/instant/instant_loader.cc (right): > > > > > http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... > > chrome/browser/instant/instant_loader.cc:150: TabContentsWrapper* > > tab_contents_wrapper = preview_contents_.release(); > > On 2011/05/19 19:53:55, sky wrote: > >> > >> ignore_result > > > > Done. > > > > > http://codereview.chromium.org/7034043/diff/1/chrome/browser/instant/instant_... > > chrome/browser/instant/instant_loader.cc:152: > > preview_contents_.reset(new_tc); > > On 2011/05/19 19:53:55, sky wrote: > >> > >> I'm not sure under when this might be invoked. > > > > It's invoked if the InstantLoader is the delegate for a > > TabContentsWrapper that is being swapped for a Prerender TabContents. > > > > This is called as a side-effect of the ProvisionalChangeToMainFrameUrl > > in the TabContentsObserver so shouldn't disturb the rest of the Instant > > flow. > > Can you give me a more concrete example. I'm not really sure what you're asking, but here's the code flow: This is called by the Prerender system when a page that we've prerendered is requested. In this case, instead of the browser handling the swap, which would normally happen, InstantLoader is the delegate for the tab so gets call to swap in the tab instead. This request is being made only at the point that Instant is expecting to display the page. To be absolutely sure, I'm now mimicking CreatePreviewContents (in fact calling a common function) to make sure this new preview_contents_ has everything set as Instant expects. > > >> Couple of things that likely need > >> to be done: > > > >> . See the code in CreatePreviewContents. I think we need to do all > > > > that on the > >> > >> tabcontents. Be sure to remove TabContentsDelegateImpl as it is still > > > > installed > >> > >> on the old delegate. > > > > Done > > > >> . I assume if this happens the page is ready to be shown. In which > > > > case invoke > >> > >> ShowPreview. > > > > This shouldn't be necessary as we're about to go through the Instant > > commit flow as normal. > > > >> . CreatePreviewContents makes sure to adjust the page ids. If swap is > > > > called > >> > >> what happens to page id of the new tabcontents? Are we sure the id is > >> = id of > >> old? > > > > While the Prerender system has already increased the page id based on > > the source of the prerender, it can't hurt to make sure we're increasing > > it here too. > > Does that mean it will have honored the max page id of the TabContents > created in InstantLoader::CreatePreviewContents? > It would (especially as Prerender adds 10 to the current max page id) but just to be sure it now increases the page id at the point that it swaps in the new preview_contents_ to one greater than the old preview_contnts_.
http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... File chrome/browser/instant/instant_loader.cc (left): http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.cc:998: preview_contents_.reset(new TabContentsWrapper(new_contents)); You need to still install the delegate on the new tabcontents. http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.cc:156: old_tc->download_tab_helper()->set_delegate(NULL); unset it on the tabcontents, not the download helper. http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.cc:1024: preview_contents_->set_delegate(this); You need to unset this some where. http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.h:40: public TabContentsWrapperDelegate { Make TabContentsWrapperDelegate implement this. It's the place meant to encapsulate all the delegate logic. http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.h:159: // Called to set up the preview_contents_ when it is created or replaced. preview_contents_ -> |tab_contents| But looking at the implementation, the implementation should either use preview_contents_ every where and not take tab_contents as a parameter, or using the parameter and not tab_contents.
http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.cc:156: old_tc->download_tab_helper()->set_delegate(NULL); On 2011/05/20 15:57:36, sky wrote: > unset it on the tabcontents, not the download helper. Both need to be done. Done. http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.cc:1024: preview_contents_->set_delegate(this); On 2011/05/20 15:57:36, sky wrote: > You need to unset this some where. Done. http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.h:40: public TabContentsWrapperDelegate { On 2011/05/20 15:57:36, sky wrote: > Make TabContentsWrapperDelegate implement this. It's the place meant to > encapsulate all the delegate logic. Done. http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.h:159: // Called to set up the preview_contents_ when it is created or replaced. On 2011/05/20 15:57:36, sky wrote: > preview_contents_ -> |tab_contents| > But looking at the implementation, the implementation should either use > preview_contents_ every where and not take tab_contents as a parameter, or using > the parameter and not tab_contents. The implementation should be using preview_contents_ everywhere. tab_contents is either the TabContentsWrapper passed in from CreatePreviewContents, or the old preview_contents_ being replaced.
http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/7034043/diff/8003/chrome/browser/instant/insta... chrome/browser/instant/instant_loader.h:159: // Called to set up the preview_contents_ when it is created or replaced. On 2011/05/20 16:39:53, dominic wrote: > On 2011/05/20 15:57:36, sky wrote: > > preview_contents_ -> |tab_contents| > > But looking at the implementation, the implementation should either use > > preview_contents_ every where and not take tab_contents as a parameter, or > using > > the parameter and not tab_contents. > > The implementation should be using preview_contents_ everywhere. tab_contents is > either the TabContentsWrapper passed in from CreatePreviewContents, or the old > preview_contents_ being replaced. My mistake. I was assuming tab_contents is the new tab contents, when it isn't. Please update the description to clarify that. http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.cc:1021: old_tc->download_tab_helper()->set_delegate(NULL); Why do you need to reset the download_tab_helper delegate? http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); If the tabcontents was already showing then the ShowPreview does nothing and the display won't update (meaning it'll still be displaying the old tabcontents you are about to delete). http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.h:15: #include "chrome/browser/ui/tab_contents/tab_contents_wrapper_delegate.h" You don't need this include anymore. http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.h:155: // into our preview_contents_. Use || around fields, eg |preview_contents_|. And on 159.
http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); On 2011/05/20 18:54:19, sky wrote: > If the tabcontents was already showing then the ShowPreview does nothing and the > display won't update (meaning it'll still be displaying the old tabcontents you > are about to delete). To deal with this you'll likely want to add a new InstantLoaderDelegate method and have InstantController then notify its delegate if the loader is active.
http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.cc:1021: old_tc->download_tab_helper()->set_delegate(NULL); On 2011/05/20 18:54:19, sky wrote: > Why do you need to reset the download_tab_helper delegate? It's set to the preview_tab_contents_delegate_ and as we're no longer using this TabContentsWrapper for preview I thought we'd want to reset all delegates. http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); On 2011/05/20 18:54:19, sky wrote: > If the tabcontents was already showing then the ShowPreview does nothing and the > display won't update (meaning it'll still be displaying the old tabcontents you > are about to delete). When would this happen though? If we're swapping in from the Prerender system it must be a new, unseen, TabContents. http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.h:15: #include "chrome/browser/ui/tab_contents/tab_contents_wrapper_delegate.h" On 2011/05/20 18:54:19, sky wrote: > You don't need this include anymore. great spot. thank you.
http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.cc:1021: old_tc->download_tab_helper()->set_delegate(NULL); On 2011/05/20 23:22:34, dominic wrote: > On 2011/05/20 18:54:19, sky wrote: > > Why do you need to reset the download_tab_helper delegate? > > It's set to the preview_tab_contents_delegate_ and as we're no longer using this > TabContentsWrapper for preview I thought we'd want to reset all delegates. I see it. Sorry. http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); On 2011/05/20 23:22:34, dominic wrote: > On 2011/05/20 18:54:19, sky wrote: > > If the tabcontents was already showing then the ShowPreview does nothing and > the > > display won't update (meaning it'll still be displaying the old tabcontents > you > > are about to delete). > > When would this happen though? If we're swapping in from the Prerender system it > must be a new, unseen, TabContents. Is that always the case? Lets say foo.com is prerendered. If I type 'food.com' in the url bar so that so that instant loads 'food.com', and then before pressing enter type in 'foo.com' won't this get swapped to the prerendered page?
On 2011/05/20 23:38:33, sky wrote: > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > File chrome/browser/instant/instant_loader.cc (right): > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > chrome/browser/instant/instant_loader.cc:1021: > old_tc->download_tab_helper()->set_delegate(NULL); > On 2011/05/20 23:22:34, dominic wrote: > > On 2011/05/20 18:54:19, sky wrote: > > > Why do you need to reset the download_tab_helper delegate? > > > > It's set to the preview_tab_contents_delegate_ and as we're no longer using > this > > TabContentsWrapper for preview I thought we'd want to reset all delegates. > > I see it. Sorry. > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); > On 2011/05/20 23:22:34, dominic wrote: > > On 2011/05/20 18:54:19, sky wrote: > > > If the tabcontents was already showing then the ShowPreview does nothing and > > the > > > display won't update (meaning it'll still be displaying the old tabcontents > > you > > > are about to delete). > > > > When would this happen though? If we're swapping in from the Prerender system > it > > must be a new, unseen, TabContents. > > Is that always the case? Lets say http://foo.com is prerendered. If I type 'food.com' > in the url bar so that so that instant loads 'food.com', and then before > pressing enter type in 'foo.com' won't this get swapped to the prerendered page? As you're typing, if we autocomplete to foo.com once you get to 'foo' then we'll swap in the prerendered page. If you then complete it as food.com and then change it to foo.com, we'll not use the prerendered page a second time. If we don't autocomplete in the first place, then we will use foo.com.
On Mon, May 23, 2011 at 11:23 AM, <dominich@chromium.org> wrote: > On 2011/05/20 23:38:33, sky wrote: > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> File chrome/browser/instant/instant_loader.cc (right): > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> chrome/browser/instant/instant_loader.cc:1021: >> old_tc->download_tab_helper()->set_delegate(NULL); >> On 2011/05/20 23:22:34, dominic wrote: >> > On 2011/05/20 18:54:19, sky wrote: >> > > Why do you need to reset the download_tab_helper delegate? >> > >> > It's set to the preview_tab_contents_delegate_ and as we're no longer >> > using >> this >> > TabContentsWrapper for preview I thought we'd want to reset all >> > delegates. > >> I see it. Sorry. > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); >> On 2011/05/20 23:22:34, dominic wrote: >> > On 2011/05/20 18:54:19, sky wrote: >> > > If the tabcontents was already showing then the ShowPreview does >> > > nothing > > and >> >> > the >> > > display won't update (meaning it'll still be displaying the old > > tabcontents >> >> > you >> > > are about to delete). >> > >> > When would this happen though? If we're swapping in from the Prerender > > system >> >> it >> > must be a new, unseen, TabContents. > >> Is that always the case? Lets say http://foo.com is prerendered. If I type > > 'food.com' >> >> in the url bar so that so that instant loads 'food.com', and then before >> pressing enter type in 'foo.com' won't this get swapped to the prerendered > > page? > > As you're typing, if we autocomplete to foo.com once you get to 'foo' then > we'll > swap in the prerendered page. If you then complete it as food.com and then > change it to foo.com, we'll not use the prerendered page a second time. The case I cared about is: . have prerendered page for foo.com. . type in omnibox so that instant starts. . type in omnibox so that end up food.com. The page finishes loading and is shown. . type in foo.com. Is this a valid path that could lead to attempting swap TabContents in an InstantLoader that is already showing? -Scott
On 2011/05/23 20:54:14, sky wrote: > On Mon, May 23, 2011 at 11:23 AM, <mailto:dominich@chromium.org> wrote: > > On 2011/05/20 23:38:33, sky wrote: > > > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > >> > >> File chrome/browser/instant/instant_loader.cc (right): > > > > > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > >> > >> chrome/browser/instant/instant_loader.cc:1021: > >> old_tc->download_tab_helper()->set_delegate(NULL); > >> On 2011/05/20 23:22:34, dominic wrote: > >> > On 2011/05/20 18:54:19, sky wrote: > >> > > Why do you need to reset the download_tab_helper delegate? > >> > > >> > It's set to the preview_tab_contents_delegate_ and as we're no longer > >> > using > >> this > >> > TabContentsWrapper for preview I thought we'd want to reset all > >> > delegates. > > > >> I see it. Sorry. > > > > > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > >> > >> chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); > >> On 2011/05/20 23:22:34, dominic wrote: > >> > On 2011/05/20 18:54:19, sky wrote: > >> > > If the tabcontents was already showing then the ShowPreview does > >> > > nothing > > > > and > >> > >> > the > >> > > display won't update (meaning it'll still be displaying the old > > > > tabcontents > >> > >> > you > >> > > are about to delete). > >> > > >> > When would this happen though? If we're swapping in from the Prerender > > > > system > >> > >> it > >> > must be a new, unseen, TabContents. > > > >> Is that always the case? Lets say http://foo.com is prerendered. If I type > > > > 'food.com' > >> > >> in the url bar so that so that instant loads 'food.com', and then before > >> pressing enter type in 'foo.com' won't this get swapped to the prerendered > > > > page? > > > > As you're typing, if we autocomplete to http://foo.com once you get to 'foo' then > > we'll > > swap in the prerendered page. If you then complete it as http://food.com and then > > change it to http://foo.com, we'll not use the prerendered page a second time. > > The case I cared about is: > . have prerendered page for http://foo.com. > . type in omnibox so that instant starts. > . type in omnibox so that end up http://food.com. The page finishes loading > and is shown. > . type in http://foo.com. > Is this a valid path that could lead to attempting swap TabContents in > an InstantLoader that is already showing? > Yes. Are you suggesting that this will not actually change what is seen by the user?
On Mon, May 23, 2011 at 1:55 PM, <dominich@chromium.org> wrote: > On 2011/05/23 20:54:14, sky wrote: >> >> On Mon, May 23, 2011 at 11:23 AM, <mailto:dominich@chromium.org> wrote: >> > On 2011/05/20 23:38:33, sky wrote: >> > >> > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> >> >> >> File chrome/browser/instant/instant_loader.cc (right): >> > >> > >> > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> >> >> >> chrome/browser/instant/instant_loader.cc:1021: >> >> old_tc->download_tab_helper()->set_delegate(NULL); >> >> On 2011/05/20 23:22:34, dominic wrote: >> >> > On 2011/05/20 18:54:19, sky wrote: >> >> > > Why do you need to reset the download_tab_helper delegate? >> >> > >> >> > It's set to the preview_tab_contents_delegate_ and as we're no longer >> >> > using >> >> this >> >> > TabContentsWrapper for preview I thought we'd want to reset all >> >> > delegates. >> > >> >> I see it. Sorry. >> > >> > >> > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> >> >> >> chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); >> >> On 2011/05/20 23:22:34, dominic wrote: >> >> > On 2011/05/20 18:54:19, sky wrote: >> >> > > If the tabcontents was already showing then the ShowPreview does >> >> > > nothing >> > >> > and >> >> >> >> > the >> >> > > display won't update (meaning it'll still be displaying the old >> > >> > tabcontents >> >> >> >> > you >> >> > > are about to delete). >> >> > >> >> > When would this happen though? If we're swapping in from the >> >> > Prerender >> > >> > system >> >> >> >> it >> >> > must be a new, unseen, TabContents. >> > >> >> Is that always the case? Lets say http://foo.com is prerendered. If I >> >> type >> > >> > 'food.com' >> >> >> >> in the url bar so that so that instant loads 'food.com', and then >> >> before >> >> pressing enter type in 'foo.com' won't this get swapped to the >> >> prerendered >> > >> > page? >> > >> > As you're typing, if we autocomplete to http://foo.com once you get to >> > 'foo' > > then >> >> > we'll >> > swap in the prerendered page. If you then complete it as http://food.com >> > and > > then >> >> > change it to http://foo.com, we'll not use the prerendered page a second > > time. > >> The case I cared about is: >> . have prerendered page for http://foo.com. >> . type in omnibox so that instant starts. >> . type in omnibox so that end up http://food.com. The page finishes >> loading >> and is shown. >> . type in http://foo.com. >> Is this a valid path that could lead to attempting swap TabContents in >> an InstantLoader that is already showing? > > > Yes. Are you suggesting that this will not actually change what is seen by > the > user? Yes. In this case ShowPreview does nothing (because the TabContents has already been shown). I believe you'll need a new InstantLoaderDelegate method to cover this case. -Scott
On 2011/05/23 21:04:16, sky wrote: > On Mon, May 23, 2011 at 1:55 PM, <mailto:dominich@chromium.org> wrote: > > On 2011/05/23 20:54:14, sky wrote: > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > >> > >> >> > >> >> chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); > >> >> On 2011/05/20 23:22:34, dominic wrote: > >> >> > On 2011/05/20 18:54:19, sky wrote: > >> >> > > If the tabcontents was already showing then the ShowPreview does > >> >> > > nothing > >> > > >> > and > >> >> > >> >> > the > >> >> > > display won't update (meaning it'll still be displaying the old > >> > > >> > tabcontents > >> >> > >> >> > you > >> >> > > are about to delete). > >> >> > > >> >> > When would this happen though? If we're swapping in from the > >> >> > Prerender > >> > > >> > system > >> >> > >> >> it > >> >> > must be a new, unseen, TabContents. > >> > > >> >> Is that always the case? Lets say http://foo.com is prerendered. If I > >> >> type > >> > > >> > 'food.com' > >> >> > >> >> in the url bar so that so that instant loads 'food.com', and then > >> >> before > >> >> pressing enter type in 'foo.com' won't this get swapped to the > >> >> prerendered > >> > > >> > page? > >> > > >> > As you're typing, if we autocomplete to http://foo.com once you get to > >> > 'foo' > > > > then > >> > >> > we'll > >> > swap in the prerendered page. If you then complete it as http://food.com > >> > and > > > > then > >> > >> > change it to http://foo.com, we'll not use the prerendered page a second > > > > time. > > > >> The case I cared about is: > >> . have prerendered page for http://foo.com. > >> . type in omnibox so that instant starts. > >> . type in omnibox so that end up http://food.com. The page finishes > >> loading > >> and is shown. > >> . type in http://foo.com. > >> Is this a valid path that could lead to attempting swap TabContents in > >> an InstantLoader that is already showing? > > > > > > Yes. Are you suggesting that this will not actually change what is seen by > > the > > user? > > Yes. In this case ShowPreview does nothing (because the TabContents > has already been shown). I believe you'll need a new > InstantLoaderDelegate method to cover this case. > My local test: - Prerender prerender_success.html - type 'prerender_success2.html' in omnibox and verify instant tab shows SUCCESS TWO - type 'prerender_success.html' in omnibox and verify instant tab changes to SUCCESS - switch back and forth between the two pages verifying that the instant tab changes correctly I believe this is the same as the above case and it works with my changes. Can you suggest what I might have missed?
On Mon, May 23, 2011 at 2:33 PM, <dominich@chromium.org> wrote: > On 2011/05/23 21:04:16, sky wrote: >> >> On Mon, May 23, 2011 at 1:55 PM, <mailto:dominich@chromium.org> wrote: >> > On 2011/05/23 20:54:14, sky wrote: >> > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> >> >> >> >> >> >> >> chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); >> >> >> On 2011/05/20 23:22:34, dominic wrote: >> >> >> > On 2011/05/20 18:54:19, sky wrote: >> >> >> > > If the tabcontents was already showing then the ShowPreview does >> >> >> > > nothing >> >> > >> >> > and >> >> >> >> >> >> > the >> >> >> > > display won't update (meaning it'll still be displaying the old >> >> > >> >> > tabcontents >> >> >> >> >> >> > you >> >> >> > > are about to delete). >> >> >> > >> >> >> > When would this happen though? If we're swapping in from the >> >> >> > Prerender >> >> > >> >> > system >> >> >> >> >> >> it >> >> >> > must be a new, unseen, TabContents. >> >> > >> >> >> Is that always the case? Lets say http://foo.com is prerendered. If >> >> >> I >> >> >> type >> >> > >> >> > 'food.com' >> >> >> >> >> >> in the url bar so that so that instant loads 'food.com', and then >> >> >> before >> >> >> pressing enter type in 'foo.com' won't this get swapped to the >> >> >> prerendered >> >> > >> >> > page? >> >> > >> >> > As you're typing, if we autocomplete to http://foo.com once you get >> >> > to >> >> > 'foo' >> > >> > then >> >> >> >> > we'll >> >> > swap in the prerendered page. If you then complete it as >> >> > http://food.com >> >> > and >> > >> > then >> >> >> >> > change it to http://foo.com, we'll not use the prerendered page a >> >> > second >> > >> > time. >> > >> >> The case I cared about is: >> >> . have prerendered page for http://foo.com. >> >> . type in omnibox so that instant starts. >> >> . type in omnibox so that end up http://food.com. The page finishes >> >> loading >> >> and is shown. >> >> . type in http://foo.com. >> >> Is this a valid path that could lead to attempting swap TabContents in >> >> an InstantLoader that is already showing? >> > >> > >> > Yes. Are you suggesting that this will not actually change what is seen >> > by >> > the >> > user? > >> Yes. In this case ShowPreview does nothing (because the TabContents >> has already been shown). I believe you'll need a new >> InstantLoaderDelegate method to cover this case. > > > My local test: > > - Prerender prerender_success.html > - type 'prerender_success2.html' in omnibox and verify instant tab shows > SUCCESS > TWO > - type 'prerender_success.html' in omnibox and verify instant tab changes to > SUCCESS > - switch back and forth between the two pages verifying that the instant tab > changes correctly > > I believe this is the same as the above case and it works with my changes. > Can > you suggest what I might have missed? If going from prerender_success2.html to prerender_success.html you're just deleting the 2, that should test what I'm worried about. Can you step through the debugger in this case. When you delete the '2' what happens when ShowPreview is invoked from ReplacePreviewContents? -Scott
On 2011/05/23 22:15:34, sky wrote: > On Mon, May 23, 2011 at 2:33 PM, <mailto:dominich@chromium.org> wrote: > > On 2011/05/23 21:04:16, sky wrote: > >> > >> On Mon, May 23, 2011 at 1:55 PM, <mailto:dominich@chromium.org> wrote: > >> > On 2011/05/23 20:54:14, sky wrote: > >> > > > > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... > >> > >> >> > >> >> >> > >> >> >> chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); > >> >> >> On 2011/05/20 23:22:34, dominic wrote: > >> >> >> > On 2011/05/20 18:54:19, sky wrote: > >> >> >> > > If the tabcontents was already showing then the ShowPreview does > >> >> >> > > nothing > >> >> > > >> >> > and > >> >> >> > >> >> >> > the > >> >> >> > > display won't update (meaning it'll still be displaying the old > >> >> > > >> >> > tabcontents > >> >> >> > >> >> >> > you > >> >> >> > > are about to delete). > >> >> >> > > >> >> >> > When would this happen though? If we're swapping in from the > >> >> >> > Prerender > >> >> > > >> >> > system > >> >> >> > >> >> >> it > >> >> >> > must be a new, unseen, TabContents. > >> >> > > >> >> >> Is that always the case? Lets say http://foo.com is prerendered. If > >> >> >> I > >> >> >> type > >> >> > > >> >> > 'food.com' > >> >> >> > >> >> >> in the url bar so that so that instant loads 'food.com', and then > >> >> >> before > >> >> >> pressing enter type in 'foo.com' won't this get swapped to the > >> >> >> prerendered > >> >> > > >> >> > page? > >> >> > > >> >> > As you're typing, if we autocomplete to http://foo.com once you get > >> >> > to > >> >> > 'foo' > >> > > >> > then > >> >> > >> >> > we'll > >> >> > swap in the prerendered page. If you then complete it as > >> >> > http://food.com > >> >> > and > >> > > >> > then > >> >> > >> >> > change it to http://foo.com, we'll not use the prerendered page a > >> >> > second > >> > > >> > time. > >> > > >> >> The case I cared about is: > >> >> . have prerendered page for http://foo.com. > >> >> . type in omnibox so that instant starts. > >> >> . type in omnibox so that end up http://food.com. The page finishes > >> >> loading > >> >> and is shown. > >> >> . type in http://foo.com. > >> >> Is this a valid path that could lead to attempting swap TabContents in > >> >> an InstantLoader that is already showing? > >> > > >> > > >> > Yes. Are you suggesting that this will not actually change what is seen > >> > by > >> > the > >> > user? > > > >> Yes. In this case ShowPreview does nothing (because the TabContents > >> has already been shown). I believe you'll need a new > >> InstantLoaderDelegate method to cover this case. > > > > > > My local test: > > > > - Prerender prerender_success.html > > - type 'prerender_success2.html' in omnibox and verify instant tab shows > > SUCCESS > > TWO > > - type 'prerender_success.html' in omnibox and verify instant tab changes to > > SUCCESS > > - switch back and forth between the two pages verifying that the instant tab > > changes correctly > > > > I believe this is the same as the above case and it works with my changes. > > Can > > you suggest what I might have missed? > > If going from prerender_success2.html to prerender_success.html you're > just deleting the 2, that should test what I'm worried about. Can you > step through the debugger in this case. When you delete the '2' what > happens when ShowPreview is invoked from ReplacePreviewContents? > I reproduced the issue - I paste in _success2.html entirely to avoid prerendering _success.html on the way. After that I don't see the tab contents as ready_ is true. Setting ready_ to false is not enough to have the prerender tab show up in this case. I don't know the Instant code well enough to know what I need to do so guidance would be useful. I presume that the delegate doesn't have a displayable loader but this should only be the case if the status is not 200 or the current loader in the manager is NULL. You mention above a different delegate function but I'm not sure what I need that to do.
You'll want something like the attached. Only invoke InstantLoaderSwappedTabContents if ready_, otherwise do the Show. -Scott On Mon, May 23, 2011 at 4:02 PM, <dominich@chromium.org> wrote: > On 2011/05/23 22:15:34, sky wrote: >> >> On Mon, May 23, 2011 at 2:33 PM, <mailto:dominich@chromium.org> wrote: >> > On 2011/05/23 21:04:16, sky wrote: >> >> >> >> On Mon, May 23, 2011 at 1:55 PM, <mailto:dominich@chromium.org> > > wrote: >> >> >> > On 2011/05/23 20:54:14, sky wrote: >> >> > >> > >> > > > http://codereview.chromium.org/7034043/diff/13002/chrome/browser/instant/inst... >> >> >> >> >> >> >> >> >> >> >> >> >> >> chrome/browser/instant/instant_loader.cc:1035: ShowPreview(); >> >> >> >> On 2011/05/20 23:22:34, dominic wrote: >> >> >> >> > On 2011/05/20 18:54:19, sky wrote: >> >> >> >> > > If the tabcontents was already showing then the ShowPreview >> >> >> >> > > does >> >> >> >> > > nothing >> >> >> > >> >> >> > and >> >> >> >> >> >> >> >> > the >> >> >> >> > > display won't update (meaning it'll still be displaying the >> >> >> >> > > old >> >> >> > >> >> >> > tabcontents >> >> >> >> >> >> >> >> > you >> >> >> >> > > are about to delete). >> >> >> >> > >> >> >> >> > When would this happen though? If we're swapping in from the >> >> >> >> > Prerender >> >> >> > >> >> >> > system >> >> >> >> >> >> >> >> it >> >> >> >> > must be a new, unseen, TabContents. >> >> >> > >> >> >> >> Is that always the case? Lets say http://foo.com is prerendered. >> >> >> >> If >> >> >> >> I >> >> >> >> type >> >> >> > >> >> >> > 'food.com' >> >> >> >> >> >> >> >> in the url bar so that so that instant loads 'food.com', and then >> >> >> >> before >> >> >> >> pressing enter type in 'foo.com' won't this get swapped to the >> >> >> >> prerendered >> >> >> > >> >> >> > page? >> >> >> > >> >> >> > As you're typing, if we autocomplete to http://foo.com once you >> >> >> > get >> >> >> > to >> >> >> > 'foo' >> >> > >> >> > then >> >> >> >> >> >> > we'll >> >> >> > swap in the prerendered page. If you then complete it as >> >> >> > http://food.com >> >> >> > and >> >> > >> >> > then >> >> >> >> >> >> > change it to http://foo.com, we'll not use the prerendered page a >> >> >> > second >> >> > >> >> > time. >> >> > >> >> >> The case I cared about is: >> >> >> . have prerendered page for http://foo.com. >> >> >> . type in omnibox so that instant starts. >> >> >> . type in omnibox so that end up http://food.com. The page finishes >> >> >> loading >> >> >> and is shown. >> >> >> . type in http://foo.com. >> >> >> Is this a valid path that could lead to attempting swap TabContents >> >> >> in >> >> >> an InstantLoader that is already showing? >> >> > >> >> > >> >> > Yes. Are you suggesting that this will not actually change what is >> >> > seen >> >> > by >> >> > the >> >> > user? >> > >> >> Yes. In this case ShowPreview does nothing (because the TabContents >> >> has already been shown). I believe you'll need a new >> >> InstantLoaderDelegate method to cover this case. >> > >> > >> > My local test: >> > >> > - Prerender prerender_success.html >> > - type 'prerender_success2.html' in omnibox and verify instant tab shows >> > SUCCESS >> > TWO >> > - type 'prerender_success.html' in omnibox and verify instant tab >> > changes to >> > SUCCESS >> > - switch back and forth between the two pages verifying that the instant >> > tab >> > changes correctly >> > >> > I believe this is the same as the above case and it works with my >> > changes. >> > Can >> > you suggest what I might have missed? > >> If going from prerender_success2.html to prerender_success.html you're >> just deleting the 2, that should test what I'm worried about. Can you >> step through the debugger in this case. When you delete the '2' what >> happens when ShowPreview is invoked from ReplacePreviewContents? > > > I reproduced the issue - I paste in _success2.html entirely to avoid > prerendering _success.html on the way. After that I don't see the tab > contents > as ready_ is true. Setting ready_ to false is not enough to have the > prerender > tab show up in this case. > > I don't know the Instant code well enough to know what I need to do so > guidance > would be useful. I presume that the delegate doesn't have a displayable > loader > but this should only be the case if the status is not 200 or the current > loader > in the manager is NULL. > > You mention above a different delegate function but I'm not sure what I need > that to do. > > > http://codereview.chromium.org/7034043/ >
LGTM, but how about a test for some coverage?
On 2011/05/24 16:04:41, sky wrote: > LGTM, but how about a test for some coverage? Added a bug to create a test as it is quite involved and I'd prefer to do it in a separate CL.
Try job failure for 7034043-21007 on linux: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
Can't apply patch for file chrome/browser/instant/instant_loader.cc. patching file chrome/browser/instant/instant_loader.cc Hunk #5 FAILED at 1010. Hunk #6 FAILED at 1023. Hunk #7 succeeded at 1044 (offset -7 lines). 2 out of 7 hunks FAILED -- saving rejects to file chrome/browser/instant/instant_loader.cc.rej
Change committed as 86651 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
