Allow out-of-process iframes to update the mouse cursor

Allow out-of-process iframes to update the mouse cursor

Plumb both Blink and content to allow mouse cursor updates to occur
when the mouse position passes over elements within an out-of-process
iframe that should change its appearance, such as a text field.

BUG=529377
Committed: https://crrev.com/77db507348c8b41b84cc3c481711a11b41f1a793
Cr-Commit-Position: refs/heads/master@{#354321}

[kenrb, 2015-10-02 21:50:08]

kenrb@chromium.org changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org

[kenrb, 2015-10-02 21:50:54]

dcheng: PTAL third_party/WebKit?
nasko: PTAL content?

[dcheng, 2015-10-05 06:48:24]

Why plumb this through WebFrameWidget if we're just going to talk to the root
view anyway?

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3520: explicit
CursorMessageFilter()
No explicit.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3540: int
last_set_cursor_routing_id() { return last_set_cursor_routing_id_; }
Nit: const.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3546:
base::TimeDelta::FromMilliseconds(2000));
Is there any significance to 2 seconds?

https://codereview.chromium.org/1382593004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1382593004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:839: if (render_widget_.get())
return render_widget_ ? render_widget.get() : render_view_.get();

[nasko, 2015-10-05 18:18:46]

Poked only through content/

https://codereview.chromium.org/1382593004/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1382593004/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:230: if
(frame_proxy_in_parent_renderer_->frame_tree_node()) {
RenderFrameProxyHost should always have a FrameTreeNode. Why do we need this
check?
I can see if we are checking whether frame_proxy_in_parent_renderer_ is null or
not.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:238: return nullptr;
Inverting the if to return immediately a nullptr and the method returning the
pointer is a bit more readable.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3544:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
Why do we need to post a task at all? If we never get a ViewHostMsg_SetCursor,
then the test should fail, right?

[kenrb, 2015-10-06 17:39:19]

Comments addressed. Another look?

On 2015/10/05 06:48:24, dcheng wrote:
> Why plumb this through WebFrameWidget if we're just going to talk to the root
> view anyway?
> 

I'm not certain that I understand the question, but it needs to get to the
RenderWidgetHostView for the top-level frame. There isn't a direct path to this
from an OOPIF renderer -- basically it has to go up through the OOPIF's
RenderWidget -> RWH -> RWHV, and I have added a path through CPFC that gets to
the top-level RWHV. I added a method to access that RWHV because there will be
other cases where it is needed, such as tooltip text, IME, and maybe the status
bar. Basically any time the OOPIF needs to change something related to browser
UI.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1382593004/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:230: if
(frame_proxy_in_parent_renderer_->frame_tree_node()) {
On 2015/10/05 18:18:46, nasko (slow to review) wrote:
> RenderFrameProxyHost should always have a FrameTreeNode. Why do we need this
> check?
> I can see if we are checking whether frame_proxy_in_parent_renderer_ is null
or
> not.

I don't think we need to check that either. I have removed the null check
entirely.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3520: explicit
CursorMessageFilter()
On 2015/10/05 06:48:24, dcheng wrote:
> No explicit.

Done.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3540: int
last_set_cursor_routing_id() { return last_set_cursor_routing_id_; }
On 2015/10/05 06:48:24, dcheng wrote:
> Nit: const.

Done.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3544:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/10/05 18:18:46, nasko (slow to review) wrote:
> Why do we need to post a task at all? If we never get a ViewHostMsg_SetCursor,
> then the test should fail, right?

When I tried locally without this, the test ran for a really long time waiting
for the response.

https://codereview.chromium.org/1382593004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3546:
base::TimeDelta::FromMilliseconds(2000));
On 2015/10/05 06:48:24, dcheng wrote:
> Is there any significance to 2 seconds?

No, it's arbitrary. Just needed a value large enough that we can be quite
certain if the cursor message hasn't arrived by then, it isn't going to.

https://codereview.chromium.org/1382593004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1382593004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:839: if (render_widget_.get())
On 2015/10/05 06:48:24, dcheng wrote:
> return render_widget_ ? render_widget.get() : render_view_.get();

Done.

[dcheng, 2015-10-06 17:57:00]

On 2015/10/06 at 17:39:19, kenrb wrote:
> Comments addressed. Another look?
> 
> On 2015/10/05 06:48:24, dcheng wrote:
> > Why plumb this through WebFrameWidget if we're just going to talk to the
root
> > view anyway?
> > 
> 
> I'm not certain that I understand the question, but it needs to get to the
RenderWidgetHostView for the top-level frame. There isn't a direct path to this
from an OOPIF renderer -- basically it has to go up through the OOPIF's
RenderWidget -> RWH -> RWHV, and I have added a path through CPFC that gets to
the top-level RWHV. I added a method to access that RWHV because there will be
other cases where it is needed, such as tooltip text, IME, and maybe the status
bar. Basically any time the OOPIF needs to change something related to browser
UI.

Basically, I'm wondering why we need to route it through a local root at all. It
seems like it should just go through WebViewClient, since this is a canonical
example of something that only makes sense at the top-level.

[kenrb, 2015-10-06 19:01:19]

On 2015/10/06 17:57:00, dcheng wrote:
> On 2015/10/06 at 17:39:19, kenrb wrote:
> > Comments addressed. Another look?
> > 
> > On 2015/10/05 06:48:24, dcheng wrote:
> > > Why plumb this through WebFrameWidget if we're just going to talk to the
> root
> > > view anyway?
> > > 
> > 
> > I'm not certain that I understand the question, but it needs to get to the
> RenderWidgetHostView for the top-level frame. There isn't a direct path to
this
> from an OOPIF renderer -- basically it has to go up through the OOPIF's
> RenderWidget -> RWH -> RWHV, and I have added a path through CPFC that gets to
> the top-level RWHV. I added a method to access that RWHV because there will be
> other cases where it is needed, such as tooltip text, IME, and maybe the
status
> bar. Basically any time the OOPIF needs to change something related to browser
> UI.
> 
> Basically, I'm wondering why we need to route it through a local root at all.
It
> seems like it should just go through WebViewClient, since this is a canonical
> example of something that only makes sense at the top-level.

From the perspective of the renderer, this needs to find its way to a
RenderWidgetHost, and in particular to a RenderWidgetHostView, so as to be
handled by the platform. The top-level frame in an OOPIF renderer shouldn't (and
for RWHV, doesn't) have these things.

The fact that it needs to be handled at the top-level is somewhat abstract from
that point of view. RenderWidgetHostViewChildFrame knows that its platform is a
parent frame, and accordingly it interacts with a higher-level RWHV via
CrossProcessFrameConnector. But for the renderer, we are just concerned with
getting a cursor update to the RWHV that manages display and input for the frame
that wants to change the cursor.

[dcheng, 2015-10-07 04:35:37]

On 2015/10/06 at 19:01:19, kenrb wrote:
> On 2015/10/06 17:57:00, dcheng wrote:
> > On 2015/10/06 at 17:39:19, kenrb wrote:
> > > Comments addressed. Another look?
> > > 
> > > On 2015/10/05 06:48:24, dcheng wrote:
> > > > Why plumb this through WebFrameWidget if we're just going to talk to the
> > root
> > > > view anyway?
> > > > 
> > > 
> > > I'm not certain that I understand the question, but it needs to get to the
> > RenderWidgetHostView for the top-level frame. There isn't a direct path to
this
> > from an OOPIF renderer -- basically it has to go up through the OOPIF's
> > RenderWidget -> RWH -> RWHV, and I have added a path through CPFC that gets
to
> > the top-level RWHV. I added a method to access that RWHV because there will
be
> > other cases where it is needed, such as tooltip text, IME, and maybe the
status
> > bar. Basically any time the OOPIF needs to change something related to
browser
> > UI.
> > 
> > Basically, I'm wondering why we need to route it through a local root at
all. It
> > seems like it should just go through WebViewClient, since this is a
canonical
> > example of something that only makes sense at the top-level.
> 
> From the perspective of the renderer, this needs to find its way to a
RenderWidgetHost, and in particular to a RenderWidgetHostView, so as to be
handled by the platform. The top-level frame in an OOPIF renderer shouldn't (and
for RWHV, doesn't) have these things.

I agree. But neither does the local root RWHV, so we end up poking a hole in
CPFC to talk to the root view.

> 
> The fact that it needs to be handled at the top-level is somewhat abstract
from that point of view. RenderWidgetHostViewChildFrame knows that its platform
is a parent frame, and accordingly it interacts with a higher-level RWHV via
CrossProcessFrameConnector. But for the renderer, we are just concerned with
getting a cursor update to the RWHV that manages display and input for the frame
that wants to change the cursor.

Yes, but in this case, that's always the root RWHV. Routing it via the local
root gives the impression that local root is somehow involved, but it is pretty
much irrelevant. Why not just go from WebView/WebViewClient ->
RenderView/RenderPage -> browser-side equivalent -> root RWHV?

[kenrb, 2015-10-07 12:31:05]

On 2015/10/07 04:35:37, dcheng wrote:
> On 2015/10/06 at 19:01:19, kenrb wrote:
> > From the perspective of the renderer, this needs to find its way to a
> RenderWidgetHost, and in particular to a RenderWidgetHostView, so as to be
> handled by the platform. The top-level frame in an OOPIF renderer shouldn't
(and
> for RWHV, doesn't) have these things.
> 
> I agree. But neither does the local root RWHV, so we end up poking a hole in
> CPFC to talk to the root view.
> 

This is consistent with our model, though. RWHV(Mac|Aura|Android) represents a
view of the render state to the corresponding platform. RWHVChildFrame does the
same, but in this case the platform is the embedding frame, and CPFC becomes the
platform's representation. So here, RWHVChildFrame is notifying its 'platform'
of the SetCursor message, and CPFC nicely encapsulates the fact that it is in
fact proxying to another RWHV that is interacts with the real platform.

> Yes, but in this case, that's always the root RWHV. Routing it via the local
> root gives the impression that local root is somehow involved, but it is
pretty
> much irrelevant. Why not just go from WebView/WebViewClient ->
> RenderView/RenderPage -> browser-side equivalent -> root RWHV?

When we need to send an update to the RWHV, it should pass through the
RenderWidgetHost. I think we are trying to get away from routing messages to
RenderViewHost that are actually intended for RenderWidgetHost.

[kenrb, 2015-10-09 14:26:24]

Daniel: Friendly ping. Let me know if you want have a chat about this.

[dcheng, 2015-10-09 15:16:44]

On 2015/10/07 at 12:31:05, kenrb wrote:
> On 2015/10/07 04:35:37, dcheng wrote:
> > On 2015/10/06 at 19:01:19, kenrb wrote:
> > > From the perspective of the renderer, this needs to find its way to a
> > RenderWidgetHost, and in particular to a RenderWidgetHostView, so as to be
> > handled by the platform. The top-level frame in an OOPIF renderer shouldn't
(and
> > for RWHV, doesn't) have these things.
> > 
> > I agree. But neither does the local root RWHV, so we end up poking a hole in
> > CPFC to talk to the root view.
> > 
> 
> This is consistent with our model, though. RWHV(Mac|Aura|Android) represents a
view of the render state to the corresponding platform. RWHVChildFrame does the
same, but in this case the platform is the embedding frame, and CPFC becomes the
platform's representation. So here, RWHVChildFrame is notifying its 'platform'
of the SetCursor message, and CPFC nicely encapsulates the fact that it is in
fact proxying to another RWHV that is interacts with the real platform.
> 
> > Yes, but in this case, that's always the root RWHV. Routing it via the local
> > root gives the impression that local root is somehow involved, but it is
pretty
> > much irrelevant. Why not just go from WebView/WebViewClient ->
> > RenderView/RenderPage -> browser-side equivalent -> root RWHV?
> 
> When we need to send an update to the RWHV, it should pass through the
RenderWidgetHost. I think we are trying to get away from routing messages to
RenderViewHost that are actually intended for RenderWidgetHost.

It's far more straightforward to just go straight to the page-level object,
rather than using RWHVCF as a proxy to the actual platform. Yes, we are moving
some things out of RVH, but not everything makes sense to move from RVH to RWH.
This is one of those things.

[kenrb, 2015-10-09 15:49:14]

On 2015/10/09 15:16:44, dcheng wrote:
> It's far more straightforward to just go straight to the page-level object,
> rather than using RWHVCF as a proxy to the actual platform. Yes, we are moving
> some things out of RVH, but not everything makes sense to move from RVH to
RWH.
> This is one of those things.

I still don't quite understand this. RWHV's job is to interface with the
platform on behalf of the renderer, and RWHVCF calling a method on CPFC to set
the cursor is it doing exactly that.

Consider what that change would do to the flow for the top-level frame.
Currently it works like this:
ChromeClient -> RenderWidget -(IPC)-> RenderWidgetHost -> RWHV -> (OS API)

Under your proposal it would work like this:
ChromeClient -> RenderWidget -> RenderView -(IPC)-> RenderViewHost ->
RenderWidgetHost -> RWHV -> (OS API)

It seems to me that the current situation is more correct, and OOPIFs and
top-level frames should use the same path for setting the cursor.

[dcheng, 2015-10-13 22:14:37]

https://codereview.chromium.org/1382593004/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3589: //
rwh_child->ForwardMouseEvent(mouse_event);
?

https://codereview.chromium.org/1382593004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1382593004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:836: return render_widget_.get() ?
render_widget_.get() : render_view_.get();
return render_widget_ ? render_widget_.get() : render_view_.get();

Also, remind me why we need this change?

[kenrb, 2015-10-14 16:29:53]

Thanks for looking. I cleaned up the points you mentioned, and also disabled the
test on Android, for obvious reasons.

https://codereview.chromium.org/1382593004/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3589: //
rwh_child->ForwardMouseEvent(mouse_event);
On 2015/10/13 22:14:37, dcheng wrote:
> ?

Sorry, debugging code that I missed.

https://codereview.chromium.org/1382593004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1382593004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:836: return render_widget_.get() ?
render_widget_.get() : render_view_.get();
On 2015/10/13 22:14:37, dcheng wrote:
> return render_widget_ ? render_widget_.get() : render_view_.get();
> 
> Also, remind me why we need this change?

Looks like we don't, I think I accidentally pulled this over from my IME CL.
Thanks for catching that.

[dcheng, 2015-10-14 17:13:53]

https://codereview.chromium.org/1382593004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1382593004/diff/80001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:30: class WebCursor;
Nit: alphabetize

https://codereview.chromium.org/1382593004/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3546:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
I think we should just remove this line, to avoid potential flakiness on a slow
bot.

https://codereview.chromium.org/1382593004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1382593004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:143: virtual void
setCursor(const Cursor&, LocalFrame*) = 0;
Explicitly name this parameter, since it can't just be an arbitrary frame. This
also applies to the various overloads.

[kenrb, 2015-10-14 17:42:30]

https://codereview.chromium.org/1382593004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1382593004/diff/80001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:30: class WebCursor;
On 2015/10/14 17:13:52, dcheng wrote:
> Nit: alphabetize

Done.

https://codereview.chromium.org/1382593004/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3546:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/10/14 17:13:52, dcheng wrote:
> I think we should just remove this line, to avoid potential flakiness on a
slow
> bot.

I added this because it isn't very helpful when a test just fails to terminate
-- eventually the test runner kills the process, but then you have to read
through the test to puzzle out what happened. There is a de facto timeout
already implied by how long before the process kill happens, which locally seems
to be something like 30 seconds. I have boosted this to 10 seconds which should
be plenty to prevent flakiness but still output a failure condition when the
message doesn't come. If you feel strongly about this then I can remove it.

https://codereview.chromium.org/1382593004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1382593004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:143: virtual void
setCursor(const Cursor&, LocalFrame*) = 0;
On 2015/10/14 17:13:52, dcheng wrote:
> Explicitly name this parameter, since it can't just be an arbitrary frame.
This
> also applies to the various overloads.

Done.

[dcheng, 2015-10-14 21:23:37]

lgtm but I really think we should remove the custom test timeout logic.

https://codereview.chromium.org/1382593004/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3546:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/10/14 at 17:42:30, kenrb wrote:
> On 2015/10/14 17:13:52, dcheng wrote:
> > I think we should just remove this line, to avoid potential flakiness on a
slow
> > bot.
> 
> I added this because it isn't very helpful when a test just fails to terminate
-- eventually the test runner kills the process, but then you have to read
through the test to puzzle out what happened. There is a de facto timeout
already implied by how long before the process kill happens, which locally seems
to be something like 30 seconds. I have boosted this to 10 seconds which should
be plenty to prevent flakiness but still output a failure condition when the
message doesn't come. If you feel strongly about this then I can remove it.

That's really a failure of the test harness though: for example, we could
conceivably change it to dump thread stacks when we kill it for a timeout. IMO,
we shouldn't add custom timeout logic like this on a per-test basis.

[nasko, 2015-10-15 17:06:55]

https://codereview.chromium.org/1382593004/diff/100001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1382593004/diff/100001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:235: ->GetView());
What happens with <webview> where the root RWHV is not at the inner WebContents,
but the outer? I think in that case we have a RWHVChildFrame, which will do
another hop through its own frame connector, right?

https://codereview.chromium.org/1382593004/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3519: // There are no cursors on
Android.
nit: I'd move the comment before the !defined.

https://codereview.chromium.org/1382593004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1382593004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:684: // FIXME: For top-level
frames we still use the WebView as a WebWidget. This
Blink now uses // TODO(username): comments.

[dcheng, 2015-10-15 17:07:07]

thanks, lgtm

https://codereview.chromium.org/1382593004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1382593004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:684: // FIXME: For top-level
frames we still use the WebView as a WebWidget. This
Nasko points out this should be TODO.

[kenrb, 2015-10-15 17:12:55]

https://codereview.chromium.org/1382593004/diff/100001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1382593004/diff/100001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:235: ->GetView());
On 2015/10/15 17:06:54, nasko (slow to review) wrote:
> What happens with <webview> where the root RWHV is not at the inner
WebContents,
> but the outer? I think in that case we have a RWHVChildFrame, which will do
> another hop through its own frame connector, right?

Yes.

https://codereview.chromium.org/1382593004/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1382593004/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3519: // There are no cursors on
Android.
On 2015/10/15 17:06:54, nasko (slow to review) wrote:
> nit: I'd move the comment before the !defined.

Done.

https://codereview.chromium.org/1382593004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1382593004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:684: // FIXME: For top-level
frames we still use the WebView as a WebWidget. This
On 2015/10/15 17:06:54, nasko (slow to review) wrote:
> Blink now uses // TODO(username): comments.

Done.

[nasko, 2015-10-15 17:41:05]

LGTM

[kenrb, 2015-10-15 17:42:19]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2015-10-15 17:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1382593004/#ps140001
(title: "nits addressed")

[commit-bot: I haz the power, 2015-10-15 17:42:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382593004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382593004/140001

[commit-bot: I haz the power, 2015-10-15 19:21:32]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-15 19:22:28]

Patchset 8 (id:??) landed as
https://crrev.com/77db507348c8b41b84cc3c481711a11b41f1a793
Cr-Commit-Position: refs/heads/master@{#354321}