Changes to not use the prerendered contents when window.opener needs to be set.

Changes to not use the prerendered contents when window.opener needs to be set.

BUG=79922
TEST=browser_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85394

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85689

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86242

[Shishir, 2011-05-03 21:37:57]

Please look at this CL when you get time.

[sreeram, 2011-05-04 17:09:02]

Thanks for the work on this.

However, I am a little concerned about the IPCs. It seems we are willy-nilly
adding all these corner case pieces of information into the messages (cf: the
HTML5 audio/video message). I think it would perhaps be better for the renderer
itself to just cancel the rendering when it encounters such corner cases, and
send the generic ViewHostMsg_Close, with an enum indicating why.

[cbentzel, 2011-05-04 17:10:15]

+brettw for the additional has_opener_set bool in the provisional load IPC
messages.

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:53: // page should pre
prerendered correctly. The page still may not be used.
Nit: prerender correctly rather than pre prerendered

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:936:
OpenDestUrlInNewWindowViaJs();
Should these also do a normal NavigateToURL and make sure they are swapped in
correctly then?

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:317: void
PrerenderContents::set_final_status(FinalStatus final_status) {
This is going to need to be modified to allow transitions out of
FINAL_STATUS_WINDOW_OPENER. For example, there may be a window.open() for the
prerendered link followed by a normal <a href=""> transition where the
PrerenderContents is used.

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:426: LOG(ERROR) << "Not using
prerendered page.";
Remove LOG(ERROR)

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:529: LOG(ERROR) << "Not using
prerendered page.";
Remove LOG(ERROR)

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:75: struct
PrerenderedPageProperties {
This seems like overkill right now.

I'd just have an enum to pass in specifying whether opener is expected.

[brettw, 2011-05-04 17:20:45]

I agree in general with sreeram's sentiment. But I'm also worried about
cluttering up the render_view code with prerendering rules. What do you think
the solution would look like if we went with his suggestion?

[sreeram, 2011-05-04 17:31:44]

Shishir just explained to me in person that my suggestion won't work
here. The renderer that's sending the viewhostmsg (with the new
has_opener bool) is not the one that prerendered the page. So
shishir's approach is the only workable solution here.

On Wed, May 4, 2011 at 10:20,  <brettw@chromium.org> wrote:
> I agree in general with sreeram's sentiment. But I'm also worried about
> cluttering up the render_view code with prerendering rules. What do you
> think
> the solution would look like if we went with his suggestion?
>
> http://codereview.chromium.org/6915019/
>

[Shishir, 2011-05-04 18:39:40]

Since we want to reuse the prerendered page and not throw it away, I will hold
this CL till domnic's cl is in since that will likely add code paths I will also
need to add.

On 2011/05/04 17:31:44, sreeram wrote:
> Shishir just explained to me in person that my suggestion won't work
> here. The renderer that's sending the viewhostmsg (with the new
> has_opener bool) is not the one that prerendered the page. So
> shishir's approach is the only workable solution here.
> 
> On Wed, May 4, 2011 at 10:20,  <mailto:brettw@chromium.org> wrote:
> > I agree in general with sreeram's sentiment. But I'm also worried about
> > cluttering up the render_view code with prerendering rules. What do you
> > think
> > the solution would look like if we went with his suggestion?
> >
> > http://codereview.chromium.org/6915019/
> >

[cbentzel, 2011-05-04 18:56:19]

It may be worth waiting for Dominic's CL, but I don't think it will have a
major impact on how this is structured. You are basically avoiding swapping
in the prerendered page if window.opener is required - this is similar to if
there were mismatched URLs.

On Wed, May 4, 2011 at 2:39 PM, <shishir@chromium.org> wrote:

> Since we want to reuse the prerendered page and not throw it away, I will
> hold
> this CL till domnic's cl is in since that will likely add code paths I will
> also
> need to add.
>
>
> On 2011/05/04 17:31:44, sreeram wrote:
>
>> Shishir just explained to me in person that my suggestion won't work
>> here. The renderer that's sending the viewhostmsg (with the new
>> has_opener bool) is not the one that prerendered the page. So
>> shishir's approach is the only workable solution here.
>>
>
>  On Wed, May 4, 2011 at 10:20,  <mailto:brettw@chromium.org> wrote:
>> > I agree in general with sreeram's sentiment. But I'm also worried about
>> > cluttering up the render_view code with prerendering rules. What do you
>> > think
>> > the solution would look like if we went with his suggestion?
>> >
>> > http://codereview.chromium.org/6915019/
>> >
>>
>
>
>
> http://codereview.chromium.org/6915019/
>

[Shishir, 2011-05-05 23:09:54]

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:53: // page should pre
prerendered correctly. The page still may not be used.
On 2011/05/04 17:10:15, cbentzel wrote:
> Nit: prerender correctly rather than pre prerendered

Done.

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:936:
OpenDestUrlInNewWindowViaJs();
On 2011/05/04 17:10:15, cbentzel wrote:
> Should these also do a normal NavigateToURL and make sure they are swapped in
> correctly then? 

As discussed we dont use the prerendered page.

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:317: void
PrerenderContents::set_final_status(FinalStatus final_status) {
On 2011/05/04 17:10:15, cbentzel wrote:
> This is going to need to be modified to allow transitions out of
> FINAL_STATUS_WINDOW_OPENER. For example, there may be a window.open() for the
> prerendered link followed by a normal <a href=""> transition where the
> PrerenderContents is used. 

As discussed we will throw away the prerendered page.

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:426: LOG(ERROR) << "Not using
prerendered page.";
On 2011/05/04 17:10:15, cbentzel wrote:
> Remove LOG(ERROR)

Done.

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:529: LOG(ERROR) << "Not using
prerendered page.";
On 2011/05/04 17:10:15, cbentzel wrote:
> Remove LOG(ERROR)

Done.

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6915019/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:75: struct
PrerenderedPageProperties {
On 2011/05/04 17:10:15, cbentzel wrote:
> This seems like overkill right now.
> 
> I'd just have an enum to pass in specifying whether opener is expected.

Then I think we might as well have a bool for now and add the struct if more
reasons show up. Changing.

[cbentzel, 2011-05-06 02:30:01]

http://codereview.chromium.org/6915019/diff/2003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6915019/diff/2003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:429:
prerender_contents->set_final_status(FINAL_STATUS_WINDOW_OPENER);
Should this be Destroy rather than set_final_status?

With just set_final_status, you may have the behavior of window.open() followed
by a href= navigation work, which we agreed was not desirable.

[Shishir, 2011-05-09 23:21:09]

Added a new patch integrating the Destroy() changes and fixing a minor issue in
PrerenderManager::MoveEntryToPendingDelete. Please take another look.

[sreeram, 2011-05-10 15:58:19]

http://codereview.chromium.org/6915019/diff/10001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:54: bool
ShouldRenderPrerenderedPageCorrectly(FinalStatus status) {
Naming suggestion: Perhaps call this DidPrerenderSucceed() or
IsFinalStatusSuccess() or some variant thereof. The current name seems way too
long and brings in unnecessary concepts ("render").

http://codereview.chromium.org/6915019/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:997:
OpenDestUrlInNewWindowViaJs();
Seems to me that you should do this:
    queue.push_back(FINAL_STATUS_USED);
    queue.push_back(FINAL_STATUS_WINDOW_OPENER);
    PrerenderTestURL(..., queue, 1);
    OpenDestUrlInNewWindowViaJs();

With your current code, if for some strange reason the prerender results in a
WINDOW_OPENER status (or some other status that causes it stop loading) before
you call OpenDestUrlInNewWindow, the test won't catch it.

[sreeram, 2011-05-10 17:01:16]

http://codereview.chromium.org/6915019/diff/10001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:997:
OpenDestUrlInNewWindowViaJs();
On 2011/05/10 15:58:20, sreeram wrote:
> Seems to me that you should do this:
>     queue.push_back(FINAL_STATUS_USED);
>     queue.push_back(FINAL_STATUS_WINDOW_OPENER);
>     PrerenderTestURL(..., queue, 1);
>     OpenDestUrlInNewWindowViaJs();
> 
> With your current code, if for some strange reason the prerender results in a
> WINDOW_OPENER status (or some other status that causes it stop loading) before
> you call OpenDestUrlInNewWindow, the test won't catch it.

If the queue thing doesn't work (because the final status is only tested later,
in ~TestPrerenderContents()), then find some other way to assert that between
the calls to PrerenderTestURL() and OpenDestUrlInNewWindow(), the final status
is USED, and not WINDOW_OPENER.

Also, do the same in the ViaClick() test below.

[Shishir, 2011-05-11 22:20:06]

http://codereview.chromium.org/6915019/diff/10001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:997:
OpenDestUrlInNewWindowViaJs();
Added a check in PrerenderTestURLImpl

On 2011/05/10 17:01:16, sreeram wrote:
> On 2011/05/10 15:58:20, sreeram wrote:
> > Seems to me that you should do this:
> >     queue.push_back(FINAL_STATUS_USED);
> >     queue.push_back(FINAL_STATUS_WINDOW_OPENER);
> >     PrerenderTestURL(..., queue, 1);
> >     OpenDestUrlInNewWindowViaJs();
> > 
> > With your current code, if for some strange reason the prerender results in
a
> > WINDOW_OPENER status (or some other status that causes it stop loading)
before
> > you call OpenDestUrlInNewWindow, the test won't catch it.
> 
> If the queue thing doesn't work (because the final status is only tested
later,
> in ~TestPrerenderContents()), then find some other way to assert that between
> the calls to PrerenderTestURL() and OpenDestUrlInNewWindow(), the final status
> is USED, and not WINDOW_OPENER.
> 
> Also, do the same in the ViaClick() test below.

[Shishir, 2011-05-12 00:29:48]

Hi Brett, Chris,

Could you take another look at the CL?

Thanks
Shishir

[sreeram, 2011-05-12 02:36:41]

LGTM

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:326:
EXPECT_EQ(prerender_contents->final_status(), FINAL_STATUS_MAX);
Swap the arguments. In EXPECT_EQ, the first argument should be the expected
value.

[cbentzel, 2011-05-12 11:17:08]

My only major concern is whether we need to specify opener on the redirect IPC.

I'm also interested in Brett's opinion on adding the opener bool in the
provisional load statement.

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:54: bool
ShouldRenderPrerenderedPageCorrectly(FinalStatus status) {
Is this needed anymore? The prerendered page is destroyed with
FINAL_STATUS_WINDOW_OPENER now, so I think this could revert back to the old
behavior.

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:226: void
OpenDestUrlInNewWindowViaJs() const {
In these two cases, you don't need to do the interface/Impl split like we did
for PrerenderTestURL or NavigateToURLImpl.

Thanks for trying to be consistent, though.

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:351:
L"window.domAutomationController.send(ClickOpenPrerenderedLinkInNewWindow())",
Nit: 80 char-line. However, this may be more readable than the
split-the-literal-across-lines-approach.

I think you could just shorten the name of the JS function which is called,
especially since it's only in the context of some specific tests. Maybe
"ClickOpenLink" and "JsOpenLink"

http://codereview.chromium.org/6915019/diff/16001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/6915019/diff/16001/content/common/view_message...
content/common/view_messages.h:1749: bool /* true if the frame has an opener set
*/,
Why do we need to specify an opener here? This wouldn't change from the initial
provisional load, right?

[Shishir, 2011-05-12 22:05:18]

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:54: bool
ShouldRenderPrerenderedPageCorrectly(FinalStatus status) {
This is still needed as in the opener case, the prereder still has to happen
correctly. The prerender is only discarded on click.

On 2011/05/12 11:17:08, cbentzel wrote:
> Is this needed anymore? The prerendered page is destroyed with
> FINAL_STATUS_WINDOW_OPENER now, so I think this could revert back to the old
> behavior.

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:226: void
OpenDestUrlInNewWindowViaJs() const {
On 2011/05/12 11:17:08, cbentzel wrote:
> In these two cases, you don't need to do the interface/Impl split like we did
> for PrerenderTestURL or NavigateToURLImpl.
> 
> Thanks for trying to be consistent, though.

Done.

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:326:
EXPECT_EQ(prerender_contents->final_status(), FINAL_STATUS_MAX);
On 2011/05/12 02:36:41, sreeram wrote:
> Swap the arguments. In EXPECT_EQ, the first argument should be the expected
> value.

Done.

http://codereview.chromium.org/6915019/diff/16001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:351:
L"window.domAutomationController.send(ClickOpenPrerenderedLinkInNewWindow())",
On 2011/05/12 11:17:08, cbentzel wrote:
> Nit: 80 char-line. However, this may be more readable than the
> split-the-literal-across-lines-approach.
> 
> I think you could just shorten the name of the JS function which is called,
> especially since it's only in the context of some specific tests. Maybe
> "ClickOpenLink" and "JsOpenLink"

Done.

http://codereview.chromium.org/6915019/diff/16001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/6915019/diff/16001/content/common/view_message...
content/common/view_messages.h:1749: bool /* true if the frame has an opener set
*/,
On 2011/05/12 11:17:08, cbentzel wrote:
> Why do we need to specify an opener here? This wouldn't change from the
initial
> provisional load, right? 

Yes that is correct. But suppose that the initial load did not hit a prerendered
page, but the redirect url did. Essentially anytime we might end up using a
prerendered page, we need to inspect the opener property.

[cbentzel, 2011-05-13 00:28:12]

LGTM

You'll still need Brett's approval for the IPC changes [or someone else with
OWNERS permission].

[brettw, 2011-05-13 05:08:04]

LGTM

[commit-bot: I haz the power, 2011-05-13 17:35:09]

Can't apply patch for file content/browser/tab_contents/tab_contents_observer.h.
patching file content/browser/tab_contents/tab_contents_observer.h
Hunk #1 FAILED at 51.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/tab_contents/tab_contents_observer.h.rej

[commit-bot: I haz the power, 2011-05-13 18:57:43]

Presubmit check for 6915019-16015 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/test/data/prerender/prerender_loader.html, line 24, 82 chars

[commit-bot: I haz the power, 2011-05-13 19:02:21]

Presubmit check for 6915019-16015 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/test/data/prerender/prerender_loader.html, line 24, 82 chars

[commit-bot: I haz the power, 2011-05-14 22:23:20]

Change committed as 85394

[Shishir, 2011-05-16 19:01:24]

Hi Dominic,

Could you upload this CL to the try bots?

Thanks
Shishir

[dominich, 2011-05-16 22:21:12]

http://codereview.chromium.org/6915019/diff/29001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6915019/diff/29001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:56: // Returns true iff the
final status is one in which the prerendered
nit: 'if'

[Shishir, 2011-05-16 22:53:21]

Hi Timo,

Can you commit this since chris is out?

Thanks
Shishir

[tburkard, 2011-05-17 17:45:35]

LGTM

On 2011/05/16 22:53:21, Shishir wrote:
> Hi Timo,
> 
> Can you commit this since chris is out?
> 
> Thanks
> Shishir

[commit-bot: I haz the power, 2011-05-17 20:30:23]

Can't apply patch for file chrome/browser/prerender/prerender_final_status.h.
patching file chrome/browser/prerender/prerender_final_status.h
Hunk #1 FAILED at 34.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/prerender/prerender_final_status.h.rej

[commit-bot: I haz the power, 2011-05-17 22:32:33]

Change committed as 85689

[jam, 2011-05-17 23:53:32]

why aren't you running this change through the trybot first?  this broke the
tree and had to be reverted.

please run all changes through the trybots.

[shishir1, 2011-05-17 23:56:16]

Dominich had run the tests through the trybots yesterday and I didnt get any
failure mails. Not sure what changed here.

-Shishir

On Tue, May 17, 2011 at 4:53 PM, <jam@chromium.org> wrote:

> why aren't you running this change through the trybot first?  this broke
> the
> tree and had to be reverted.
>
> please run all changes through the trybots.
>
>
> http://codereview.chromium.org/6915019/
>

[tburkard, 2011-05-18 20:13:21]

LGTM

On Tue, May 17, 2011 at 4:55 PM, Shishir Agrawal <shishir@google.com> wrote:

> Dominich had run the tests through the trybots yesterday and I didnt get
> any failure mails. Not sure what changed here.
>
> -Shishir
>
>
> On Tue, May 17, 2011 at 4:53 PM, <jam@chromium.org> wrote:
>
>> why aren't you running this change through the trybot first?  this broke
>> the
>> tree and had to be reverted.
>>
>> please run all changes through the trybots.
>>
>>
>> http://codereview.chromium.org/6915019/
>>
>
>

[commit-bot: I haz the power, 2011-05-22 19:40:29]

Change committed as 86242