Support window.opener after a process swap.

Support window.opener after a process swap.

Changes:
1. Swap out RVHs instead of closing them, so we can return to the same Frame object if we later come back.
2. Filter out messages from swapped out RVHs, in case any are in-flight.
3. Remove the workaround for navigating away from an app.
4. Update tests to reflect the new sequence of events.

BUG=65953
TEST=AppApiTest.AppProcess
TEST=ResourceDispatcherTest.CrossSiteImmediateLoadOnunloadCookie

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

[Charlie Reis, 2011-04-13 22:57:21]

Here's the current draft, while I try to fix some of the other test failures and
issues.

The main changes are to RenderViewHost, RenderViewHostManager, and RenderView. 
Most of the rest is just plumbing to use SwapOut instead of ClosePage.

Feel free to ping me if you have questions about it.  Thanks!

http://codereview.chromium.org/6319001/diff/12001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6319001/diff/12001/content/renderer/render_vie...
content/renderer/render_view.cc:4481:
webview()->mainFrame()->loadRequest(request);
This is the main sticking point right now.  We need a way of clearing the
contents of the frame as if a new navigation had taken place, running unload but
not beforeunload.  I'm simulating that by loading about:blank and intercepting
the beforeunload, but it's far from perfect, and trips up if you try to go back
from an actual about:blank page.

I'm guessing we'll have to add something to WebKit.

[jam, 2011-04-14 02:07:26]

I don't quite understand how this is being done :)  I read the motivation in the
bug, but I don't follow why we keep the RV around, since we navigate it to a
blank url?  if that same RV is then navigated to the original url, it will have
the window.opener still set i assume.  I assume you already thought of this, so
for my own understanding, can you explain why can't we create a new RV and just
set its window.opener to the previous RV?

[Charlie Reis, 2011-04-14 03:02:14]

On 2011/04/14 02:07:26, John Abd-El-Malek wrote:
> I don't quite understand how this is being done :)  I read the motivation in
the
> bug, but I don't follow why we keep the RV around, since we navigate it to a
> blank url?  if that same RV is then navigated to the original url, it will
have
> the window.opener still set i assume.  I assume you already thought of this,
so
> for my own understanding, can you explain why can't we create a new RV and
just
> set its window.opener to the previous RV?

Sure-- if we created a new RenderView and Frame object, existing JavaScript
references to the previous Frame would be broken.  We need to keep the original
Frame object around so that the opener window can still talk to the windows it
opened.  (There's no way to restore those JavaScript window references, as we
might do for window.opener.)

[Charlie Reis, 2011-04-14 17:55:48]

Just uploaded a new draft (patch set 4) that removes the temporary workaround
logic introduced in http://crbug.com/59285.  This lets us use
AppApiTest.AppProcess as a browser test for this change, since it verifies that
window.opener is still valid after a process swap.

I also swapped Hide and Show in RVHM::CommitPending, since it prevents us from
seeing a quick glimpse of the sad tab during cross-process swaps.

The change to RenderView takes a different approach to navigating to
about:blank, but it doesn't work in a different way.  Still investigating.

[jam, 2011-04-14 19:16:21]

Thanks for the doc, that explained it more.  However I have to say that I
feel unqualified to review this, other than at a high level (which I don't
see any issues), since I'm not familiar with these specific codepaths..

On Thu, Apr 14, 2011 at 10:55 AM, <creis@chromium.org> wrote:

> Just uploaded a new draft (patch set 4) that removes the temporary
> workaround
> logic introduced in http://crbug.com/59285.  This lets us use
> AppApiTest.AppProcess as a browser test for this change, since it verifies
> that
> window.opener is still valid after a process swap.
>
> I also swapped Hide and Show in RVHM::CommitPending, since it prevents us
> from
> seeing a quick glimpse of the sad tab during cross-process swaps.
>
> The change to RenderView takes a different approach to navigating to
> about:blank, but it doesn't work in a different way.  Still investigating.
>
>
> http://codereview.chromium.org/6319001/
>

[Charlie Reis, 2011-04-14 19:38:48]

Thanks for taking a look, John.  Matt, would you be able to review this, or
should I wait until Darin or Brett have time?  It's still blocked on some tricky
behavior in render_view.cc, but most of it is ready for feedback.

[Matt Perry, 2011-04-14 23:48:08]

Mostly LGTM. My one concern is whether a swapped-out RenderView can still send
messages to the RVH (and its TabContents). Can you add a CHECK somewhere (in the
RVH message handler maybe) that we're not getting messages from swapped-out RVs?

[Charlie Reis, 2011-04-18 18:01:02]

On 2011/04/14 23:48:08, Matt Perry wrote:
> Mostly LGTM. My one concern is whether a swapped-out RenderView can still send
> messages to the RVH (and its TabContents). Can you add a CHECK somewhere (in
the
> RVH message handler maybe) that we're not getting messages from swapped-out
RVs?

I'm making progress on getting the RenderView behavior working, but I've
discovered that we do need to worry about messages being sent to the browser
from a swapped out renderer.

Some messages need to get through, like InputEvent_ACK.  Otherwise the browser
thinks it's waiting for an ACK and never sends future InputEvent messages.

Some we want to block, like DidStartLoading, DidStopLoading, FrameNavigate, etc,
which happen when we do the dummy about:blank navigation.  We're kind of hoping
that no future messages are sent from the renderer as well, but blocking all
messages runs the risk of breaking things like InputEvent_ACK.

I see three options:
1) Block particular messages that happen during navigation and allow everything
else.  (Might be possible, but we still face the risk of sending unexpected
messages that confuse the browser.)

2) Block all messages after the SwapOut_ACK, and get the browser to clean up any
outstanding state when it receives the SwapOut_ACK.  (Might be tough, given all
the places that might expect ACKs.)

3) Block everything except ACK messages.  This should keep the browser's state
in good shape, and the browser will stop sending messages to be ACK'd once the
RVh is swapped out.

I'm inclined to go with option 3 unless there's a reason not to.

[Matt Perry, 2011-04-18 18:06:47]

On 2011/04/18 18:01:02, creis wrote:
> On 2011/04/14 23:48:08, Matt Perry wrote:
> > Mostly LGTM. My one concern is whether a swapped-out RenderView can still
send
> > messages to the RVH (and its TabContents). Can you add a CHECK somewhere (in
> the
> > RVH message handler maybe) that we're not getting messages from swapped-out
> RVs?
> 
> I'm making progress on getting the RenderView behavior working, but I've
> discovered that we do need to worry about messages being sent to the browser
> from a swapped out renderer.
> 
> Some messages need to get through, like InputEvent_ACK.  Otherwise the browser
> thinks it's waiting for an ACK and never sends future InputEvent messages.
> 
> Some we want to block, like DidStartLoading, DidStopLoading, FrameNavigate,
etc,
> which happen when we do the dummy about:blank navigation.  We're kind of
hoping
> that no future messages are sent from the renderer as well, but blocking all
> messages runs the risk of breaking things like InputEvent_ACK.
> 
> I see three options:
> 1) Block particular messages that happen during navigation and allow
everything
> else.  (Might be possible, but we still face the risk of sending unexpected
> messages that confuse the browser.)
> 
> 2) Block all messages after the SwapOut_ACK, and get the browser to clean up
any
> outstanding state when it receives the SwapOut_ACK.  (Might be tough, given
all
> the places that might expect ACKs.)
> 
> 3) Block everything except ACK messages.  This should keep the browser's state
> in good shape, and the browser will stop sending messages to be ACK'd once the
> RVh is swapped out.
> 
> I'm inclined to go with option 3 unless there's a reason not to.

Interesting. 2 sounds like the ideal option, though I can imagine it being
difficult. Why is it important to receive ACKs in the swap-out case when it was
not important back when we just closed the RenderView? Can we somehow reset a
RVH/RV's state on swap-in to what it would be if it were newly created?

That said, I'd be OK with 3, but I'd prefer we explore the other options first.

[Charlie Reis, 2011-04-18 18:29:12]

On 2011/04/18 18:06:47, Matt Perry wrote:
> Interesting. 2 sounds like the ideal option, though I can imagine it being
> difficult. Why is it important to receive ACKs in the swap-out case when it
was
> not important back when we just closed the RenderView? Can we somehow reset a
> RVH/RV's state on swap-in to what it would be if it were newly created?
> 
> That said, I'd be OK with 3, but I'd prefer we explore the other options
first.

I suppose I could try to abstract out some of the code from
RenderWidgetHost::RendererExited to reset any of the *_ack_pending_ state.  I
just find it odd to pretend like an ACK arrived if it never did.  If the ACK
carries some kind of meaningful response (like the yes/no answer in
ShouldClose_ACK), we might end up making up the wrong answer and then leaving
the browser and renderer in inconsistent states.

The reason it's important now and wasn't in the closing case is that we actually
do intend to return to this RenderView and resume using it.  It seems like we
should make sure the state is valid for that case.

Of course, blindly allowing all ACKs through isn't ideal, but it does have some
reasoning behind it.  I'm looking into having Send vs SendIfNotSwappedOut
methods so that we can treat each case appropriately, with "only allow ACKs"
being the general guideline.

[Charlie Reis, 2011-04-28 01:49:50]

After looking at the message filtering more, it became clear that we'd need to
filter things out on the browser side, because there could be messages in flight
from a renderer the browser has just decided to swap out.  From there, it just
became easiest to filter solely in the browser.

It took some work, but (almost) all the tests are passing now.  There's one UI
test failing on Mac, but that one passes for me on my local Mac, so I haven't
figured out what's wrong yet.

Other than that, I think this is about ready to go.  To make it easier to
review, I'll summarize the changes again:

1. Swap out RVHs instead of closing them, so that we can come back to them
later.
 - RVH, RVHM, ResourceDispatcherHost, SiteInstance, ViewMessages, RenderView,
RenderWidget
 - Just plumbing: Browser, VisitedLinkEventsTest, BrowserRPH, MockRPH, RPH,
RVHTest, RenderWidgetHelper

2. Filter out messages from swapped out renderers.  Includes sending some
messages to RVH instead of directly to TabContents, and making it possible to
suppress JavaScript dialogs.
 - RVH, RVHDelegate, TabContents
 - Just plumbing: ExtensionHost, PrerenderContents, BackgroundContents

3. Remove the workaround for navigating away from an app, and update test.
 - ChromeContentRendererClient, AppProcessApiTest

4. Update tests to reflect new cross-site navigation events.
 - WebContentsUnitTest, ResourceDispatcherHostUITest, TestRVH, RVHMTest,
TestTabContents, WebUITest

[Matt Perry, 2011-04-28 22:48:20]

http://codereview.chromium.org/6319001/diff/34002/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6319001/diff/34002/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:876: if (view &&
!is_swapped_out_)
rather than adding these checks in every message handler (which will be hard to
maintain as new messages are introduced), what about this:
- add a CanProcessMessageWhileSwappedOut method that has a whitelist of all the
allowed messages
- in RenderViewHost::OnMessageReceived, check if the RVH is swapped out. if so,
pass the message to the above method, and stop processing if it says no.
- revert the changes to the RVHD and tab_contents for processing these messages
specially.

The ordering of the check is important. I think you'll want to do the
swapped_out check before calling the RVH's delegates or observers.

[Charlie Reis, 2011-04-29 17:53:53]

http://codereview.chromium.org/6319001/diff/34002/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6319001/diff/34002/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:876: if (view &&
!is_swapped_out_)
On 2011/04/28 22:48:20, Matt Perry wrote:
> rather than adding these checks in every message handler (which will be hard
to
> maintain as new messages are introduced), what about this:
> - add a CanProcessMessageWhileSwappedOut method that has a whitelist of all
the
> allowed messages
> - in RenderViewHost::OnMessageReceived, check if the RVH is swapped out. if
so,
> pass the message to the above method, and stop processing if it says no.
> - revert the changes to the RVHD and tab_contents for processing these
messages
> specially.
> 
> The ordering of the check is important. I think you'll want to do the
> swapped_out check before calling the RVH's delegates or observers.

That's a great idea-- thanks.  Now there's only a couple places (where we want
the ACKs to be sent) that we have to filter like this.

New patch uploaded.

http://codereview.chromium.org/6319001/diff/34002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/6319001/diff/34002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.cc:303: void
RenderWidgetHost::WasSwappedOut() {
I also moved this to RenderViewHost, since we don't have any other swapped out
logic in this class.  (It's handled by RenderWidget, but that doesn't matter
here.)

[Matt Perry, 2011-04-29 19:25:18]

LGTM

http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.h:192: // Called to notify the
RenderWidget that it has been visibly swapped out,
RenderView

http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.h:194: void WasSwappedOut();
I'm a little confused on the difference between SwapOut and WasSwappedOut. When
would one be called rather than the other? Could you clarify this in the
comments?

http://codereview.chromium.org/6319001/diff/33046/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6319001/diff/33046/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:800:
swapped_out_hosts_.erase(iter);
I think you can just do: swapped_out_hosts_.erase(site_instance_id);

[Charlie Reis, 2011-05-02 21:06:38]

Thanks Matt.  Just trying to solve one last issue that's causing the test
failure on the Mac bot.

(It's possible for the browser to go back and then quickly forward, which sends
a Navigate message to a renderer that has decided to exit.  We end up stuck
waiting for a navigation that will never finish.  I'm looking at how the exit
behavior works to figure out a solution.)

Charlie


On 2011/04/29 19:25:18, Matt Perry wrote:
> LGTM
> 
>
http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
> File content/browser/renderer_host/render_view_host.h (right):
> 
>
http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
> content/browser/renderer_host/render_view_host.h:192: // Called to notify the
> RenderWidget that it has been visibly swapped out,
> RenderView
> 
>
http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
> content/browser/renderer_host/render_view_host.h:194: void WasSwappedOut();
> I'm a little confused on the difference between SwapOut and WasSwappedOut.
When
> would one be called rather than the other? Could you clarify this in the
> comments?
> 
>
http://codereview.chromium.org/6319001/diff/33046/content/browser/tab_content...
> File content/browser/tab_contents/render_view_host_manager.cc (right):
> 
>
http://codereview.chromium.org/6319001/diff/33046/content/browser/tab_content...
> content/browser/tab_contents/render_view_host_manager.cc:800:
> swapped_out_hosts_.erase(iter);
> I think you can just do: swapped_out_hosts_.erase(site_instance_id);

[Charlie Reis, 2011-05-05 15:17:58]

Ok, we pass all the tests now!

The main changes from patch set 11:

1. We now prevent a renderer process from exiting if it has been scheduled for a
new navigation, by sending a ShutdownRequest to BrowserRenderProcessHost.  I've
pulled this part out into a separate CL for John to review (though it's
necessary here): http://codereview.chromium.org/6927014/.

2. We now filter messages when sending them from the renderer as well, if it is
swapped out.  This is important in case the browser has swapped it out and then
quickly back in again-- the browser won't be filtering out messages from the
swap out operation.  (This includes changes to ChromeRenderViewObserver, which
is the one that knows about DomOperationResponse messages.)

Matt, can take another look over it?

Thanks!
Charlie

http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.h:192: // Called to notify the
RenderWidget that it has been visibly swapped out,
On 2011/04/29 19:25:18, Matt Perry wrote:
> RenderView

Done.

http://codereview.chromium.org/6319001/diff/33046/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.h:194: void WasSwappedOut();
On 2011/04/29 19:25:18, Matt Perry wrote:
> I'm a little confused on the difference between SwapOut and WasSwappedOut.
When
> would one be called rather than the other? Could you clarify this in the
> comments?

Done.

http://codereview.chromium.org/6319001/diff/33046/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6319001/diff/33046/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:800:
swapped_out_hosts_.erase(iter);
On 2011/04/29 19:25:18, Matt Perry wrote:
> I think you can just do: swapped_out_hosts_.erase(site_instance_id);

Done.

[Matt Perry, 2011-05-05 17:06:50]

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
content/renderer/render_widget.cc:205: bool
RenderWidget::CanSendWhileSwappedOut(IPC::Message* message) {
It's a shame that these message lists are duplicated in RenderViewHost. Here's
an idea to fix it: it looks like the decision to send is entirely based on the
message type. That means you can move this list to some shared function in
content/common/, and call it from both RenderWidget::Send and
RenderViewHost::OnMessageReceived.

[Charlie Reis, 2011-05-10 20:15:09]

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
content/renderer/render_widget.cc:205: bool
RenderWidget::CanSendWhileSwappedOut(IPC::Message* message) {
On 2011/05/05 17:06:50, Matt Perry wrote:
> It's a shame that these message lists are duplicated in RenderViewHost. Here's
> an idea to fix it: it looks like the decision to send is entirely based on the
> message type. That means you can move this list to some shared function in
> content/common/, and call it from both RenderWidget::Send and
> RenderViewHost::OnMessageReceived.

The lists aren't identical, but it does seem like the list of acceptable
messages to send should be a subset of those acceptable to receive.  I suppose I
could add two new functions in content/common, perhaps in a
swapped_out_messages.{h,cc} file.

That will have to call out to ChromeContentClient to know about Chrome-specific
messages, the way that this patch calls out to ChromeRenderViewObserver from
RenderView.  Sounds feasible, though-- I'll try to put it together.

[Matt Perry, 2011-05-10 20:26:32]

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
content/renderer/render_widget.cc:205: bool
RenderWidget::CanSendWhileSwappedOut(IPC::Message* message) {
On 2011/05/10 20:15:09, creis wrote:
> On 2011/05/05 17:06:50, Matt Perry wrote:
> > It's a shame that these message lists are duplicated in RenderViewHost.
Here's
> > an idea to fix it: it looks like the decision to send is entirely based on
the
> > message type. That means you can move this list to some shared function in
> > content/common/, and call it from both RenderWidget::Send and
> > RenderViewHost::OnMessageReceived.
> 
> The lists aren't identical, but it does seem like the list of acceptable
> messages to send should be a subset of those acceptable to receive.  I suppose
I
> could add two new functions in content/common, perhaps in a
> swapped_out_messages.{h,cc} file.
> 
> That will have to call out to ChromeContentClient to know about
Chrome-specific
> messages, the way that this patch calls out to ChromeRenderViewObserver from
> RenderView.  Sounds feasible, though-- I'll try to put it together.

Oh interesting. There are some messages that you want to be able to receive but
not send? and/or vice versa?

[Charlie Reis, 2011-05-10 20:35:23]

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
content/renderer/render_widget.cc:205: bool
RenderWidget::CanSendWhileSwappedOut(IPC::Message* message) {
On 2011/05/10 20:26:32, Matt Perry wrote:
> On 2011/05/10 20:15:09, creis wrote:
> > On 2011/05/05 17:06:50, Matt Perry wrote:
> > > It's a shame that these message lists are duplicated in RenderViewHost.
> Here's
> > > an idea to fix it: it looks like the decision to send is entirely based on
> the
> > > message type. That means you can move this list to some shared function in
> > > content/common/, and call it from both RenderWidget::Send and
> > > RenderViewHost::OnMessageReceived.
> > 
> > The lists aren't identical, but it does seem like the list of acceptable
> > messages to send should be a subset of those acceptable to receive.  I
suppose
> I
> > could add two new functions in content/common, perhaps in a
> > swapped_out_messages.{h,cc} file.
> > 
> > That will have to call out to ChromeContentClient to know about
> Chrome-specific
> > messages, the way that this patch calls out to ChromeRenderViewObserver from
> > RenderView.  Sounds feasible, though-- I'll try to put it together.
> 
> Oh interesting. There are some messages that you want to be able to receive
but
> not send? and/or vice versa?

Yes-- there are several messages that the renderer might send before it's
swapped out, expecting ACKs.  The browser should send the ACK even if it knows
the renderer is swapped out, so that the renderer isn't confused if we swap it
back in.  For example, ShowView.  JavaScript dialogs also need to be silently
suppressed with an ACK.

The renderer shouldn't be allowed to send these while swapped out-- we just have
to handle the case when the messages are in flight.

That's why they're implemented on both sides right now, though I agree there's a
lot of overlap.

[Charlie Reis, 2011-05-10 23:17:56]

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/6319001/diff/39023/content/renderer/render_wid...
content/renderer/render_widget.cc:205: bool
RenderWidget::CanSendWhileSwappedOut(IPC::Message* message) {
On 2011/05/10 20:35:23, creis wrote:
> On 2011/05/10 20:26:32, Matt Perry wrote:
> > Oh interesting. There are some messages that you want to be able to receive
> but
> > not send? and/or vice versa?
> 
> Yes-- there are several messages that the renderer might send before it's
> swapped out, expecting ACKs.  The browser should send the ACK even if it knows
> the renderer is swapped out, so that the renderer isn't confused if we swap it
> back in.  For example, ShowView.  JavaScript dialogs also need to be silently
> suppressed with an ACK.
> 
> The renderer shouldn't be allowed to send these while swapped out-- we just
have
> to handle the case when the messages are in flight.
> 
> That's why they're implemented on both sides right now, though I agree there's
a
> lot of overlap.

Fixed in the new patch set.  I abstract it out to SwappedOutMessageFilters in
content/chrome, and delegated to ChromeContentClient for Chrome-specific
messages.

[Matt Perry, 2011-05-10 23:23:43]

LGTM

http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
File content/common/swapped_out_message_filter.cc (right):

http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
content/common/swapped_out_message_filter.cc:40: if
(CanSendWhileSwappedOut(&msg))
much clearer! thanks!

[Charlie Reis, 2011-05-11 01:03:54]

Thanks Matt.  John, can you take a look for sanity (now that Matt has approved),
as one of the owners of the content files?

I'm also trying to figure out why the latest patch won't compile on Windows. 
Looks like including render_messages.h in chrome_content_client.cc is causing
the try bot to fail on some Skia code in the NaCl target, despite it compiling
for me locally on Windows.  I don't see a way around including
render_messages.h, though.  Any ideas?

[Charlie Reis, 2011-05-11 20:13:29]

On 2011/05/11 01:03:54, creis wrote:
> Thanks Matt.  John, can you take a look for sanity (now that Matt has
approved),
> as one of the owners of the content files?
> 
> I'm also trying to figure out why the latest patch won't compile on Windows. 
> Looks like including render_messages.h in chrome_content_client.cc is causing
> the try bot to fail on some Skia code in the NaCl target, despite it compiling
> for me locally on Windows.  I don't see a way around including
> render_messages.h, though.  Any ideas?

I made a change to render_messages.h to get it to compile.  Looks like we're all
clear now.

(FYI, the current Windows try bot failure in test_shell_tests is an issue with
the bot, affecting others as well.)

John, think you'll be able to take a look for owners approval?

Thanks!
Charlie

[jam, 2011-05-11 20:19:31]

sorry i forgot to hit publish on this earlier.  i think the nacl build error
should be solved in content_chrome_client for consistency

http://codereview.chromium.org/6319001/diff/49001/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/6319001/diff/49001/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:15: #include
"chrome/common/render_messages.h"
the reason you're getting the compile link failure is because on windows, we
build a dll that has a little bit of chrome (see the chrome_dll_nacl_win64
target) in 64 bits for the nacl sandbox to work.  it doesn't link in with
webkit/skia etc, since those aren't built in 64 bit (and we wouldn't want them
to, since it would bloat the binary unnecessarily).

one (admittedly hacky) way to get around this would be to surround the body of
these functions with "#if !defined(NACL_WIN64)", similar to other places in this
file

http://codereview.chromium.org/6319001/diff/49001/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6319001/diff/49001/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:196: if
(pending_render_view_host_->GetPendingRequestId() == -1)
nit: need brace brackets here

http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
File content/common/swapped_out_message_filter.h (right):

http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
content/common/swapped_out_message_filter.h:5: #ifndef
CONTENT_COMMON_SWAPPED_OUT_MESSAGE_FILTER_H_
why is this file called _message_filter?  we usually use that naming for classes
that implement IPC::ChannelProxy::MessageFilter.  in this case,
swapped_out_messages.h (or something similar) seems more appropriate

http://codereview.chromium.org/6319001/diff/49001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6319001/diff/49001/content/renderer/render_vie...
content/renderer/render_view.cc:690: if (is_swapped_out_) {
nit: most of this file doesn't use brace brackets for one line statements, so
should stick with that

[jam, 2011-05-11 20:20:32]

On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
> sorry i forgot to hit publish on this earlier.  i think the nacl build error
> should be solved in content_chrome_client for consistency

actually nm, I said that before I saw your fix :)  ok fine as is.

[Charlie Reis, 2011-05-11 20:49:15]

http://codereview.chromium.org/6319001/diff/49001/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/6319001/diff/49001/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:15: #include
"chrome/common/render_messages.h"
On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
> the reason you're getting the compile link failure is because on windows, we
> build a dll that has a little bit of chrome (see the chrome_dll_nacl_win64
> target) in 64 bits for the nacl sandbox to work.  it doesn't link in with
> webkit/skia etc, since those aren't built in 64 bit (and we wouldn't want them
> to, since it would bloat the binary unnecessarily).
> 
> one (admittedly hacky) way to get around this would be to surround the body of
> these functions with "#if !defined(NACL_WIN64)", similar to other places in
this
> file

Thanks.   (Fixed in render_messages.h, as you saw.)

http://codereview.chromium.org/6319001/diff/49001/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6319001/diff/49001/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:196: if
(pending_render_view_host_->GetPendingRequestId() == -1)
On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
> nit: need brace brackets here

Done.

http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
File content/common/swapped_out_message_filter.h (right):

http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
content/common/swapped_out_message_filter.h:5: #ifndef
CONTENT_COMMON_SWAPPED_OUT_MESSAGE_FILTER_H_
On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
> why is this file called _message_filter?  we usually use that naming for
classes
> that implement IPC::ChannelProxy::MessageFilter.  in this case,
> swapped_out_messages.h (or something similar) seems more appropriate

Ah, wasn't aware of the convention.  Fixed.

http://codereview.chromium.org/6319001/diff/49001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6319001/diff/49001/content/renderer/render_vie...
content/renderer/render_view.cc:690: if (is_swapped_out_) {
On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
> nit: most of this file doesn't use brace brackets for one line statements, so
> should stick with that

Done.

[jam, 2011-05-11 21:43:52]

rubber stamp lgtm (rubberstamp since I didn't review in detail)

On Wed, May 11, 2011 at 1:49 PM, <creis@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6319001/diff/49001/chrome/common/chrome_conten...
> File chrome/common/chrome_content_client.cc (right):
>
>
>
http://codereview.chromium.org/6319001/diff/49001/chrome/common/chrome_conten...
> chrome/common/chrome_content_client.cc:15: #include
> "chrome/common/render_messages.h"
> On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
>
>> the reason you're getting the compile link failure is because on
>>
> windows, we
>
>> build a dll that has a little bit of chrome (see the
>>
> chrome_dll_nacl_win64
>
>> target) in 64 bits for the nacl sandbox to work.  it doesn't link in
>>
> with
>
>> webkit/skia etc, since those aren't built in 64 bit (and we wouldn't
>>
> want them
>
>> to, since it would bloat the binary unnecessarily).
>>
>
>  one (admittedly hacky) way to get around this would be to surround the
>>
> body of
>
>> these functions with "#if !defined(NACL_WIN64)", similar to other
>>
> places in this
>
>> file
>>
>
> Thanks.   (Fixed in render_messages.h, as you saw.)
>
>
>
>
http://codereview.chromium.org/6319001/diff/49001/content/browser/tab_content...
> File content/browser/tab_contents/render_view_host_manager.cc (right):
>
>
>
http://codereview.chromium.org/6319001/diff/49001/content/browser/tab_content...
> content/browser/tab_contents/render_view_host_manager.cc:196: if
> (pending_render_view_host_->GetPendingRequestId() == -1)
> On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
>
>> nit: need brace brackets here
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
> File content/common/swapped_out_message_filter.h (right):
>
>
>
http://codereview.chromium.org/6319001/diff/49001/content/common/swapped_out_...
> content/common/swapped_out_message_filter.h:5: #ifndef
> CONTENT_COMMON_SWAPPED_OUT_MESSAGE_FILTER_H_
> On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
>
>> why is this file called _message_filter?  we usually use that naming
>>
> for classes
>
>> that implement IPC::ChannelProxy::MessageFilter.  in this case,
>> swapped_out_messages.h (or something similar) seems more appropriate
>>
>
> Ah, wasn't aware of the convention.  Fixed.
>
>
>
>
http://codereview.chromium.org/6319001/diff/49001/content/renderer/render_vie...
> File content/renderer/render_view.cc (right):
>
>
>
http://codereview.chromium.org/6319001/diff/49001/content/renderer/render_vie...
> content/renderer/render_view.cc:690: if (is_swapped_out_) {
> On 2011/05/11 20:19:31, John Abd-El-Malek wrote:
>
>> nit: most of this file doesn't use brace brackets for one line
>>
> statements, so
>
>> should stick with that
>>
>
> Done.
>
>
> http://codereview.chromium.org/6319001/
>