Move window.close implementation to DOMWindow

Move window.close implementation to DOMWindow

This CL moves the implementation of window.close to DOMWindow, such that it is shared between LocalDOMWindow and RemoteDOMWindow. Follow up CLs will split the difference between closing windows and popups in the WebWidgetClient interface to ensure we have separate codepaths for those two types.

BUG=498900
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197688

[nasko, 2015-06-10 17:45:35]

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

[nasko, 2015-06-10 17:45:36]

Hey Daniel,
This is a strawman CL to implement RemoteDOMWindow::close. Let me know if it is
in the right direction.
Thanks in advance!
Nasko

[dcheng, 2015-06-10 20:10:41]

https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
Source/core/frame/RemoteFrame.cpp:157: remoteFrameClient()->close();
LocalDOMWindow::close() has a bunch of other stuff. It seems like at least some
of these other things are relevant for RemoteDOMWindow too, right?

For example, respecting the "allow scripts to close windows" setting, setting
m_windowIsClosing, etc.

Also, can we just use ChromeClient::closeWindowSoon()?

[nasko, 2015-06-11 00:04:39]

https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
Source/core/frame/RemoteFrame.cpp:157: remoteFrameClient()->close();
On 2015/06/10 20:10:41, dcheng wrote:
> LocalDOMWindow::close() has a bunch of other stuff. It seems like at least
some
> of these other things are relevant for RemoteDOMWindow too, right?
> 
> For example, respecting the "allow scripts to close windows" setting, setting
> m_windowIsClosing, etc.

I've added those to RemoteDOMWindow.
 
> Also, can we just use ChromeClient::closeWindowSoon()?

I'd rather not, as it goes over to the widget and for remote frames we don't
have a widget on the content/ side, just a proxy.

https://codereview.chromium.org/1176843006/diff/60001/Source/core/frame/Remot...
File Source/core/frame/RemoteDOMWindow.cpp (right):

https://codereview.chromium.org/1176843006/diff/60001/Source/core/frame/Remot...
Source/core/frame/RemoteDOMWindow.cpp:256: return;
Most of the above checks are somewhat redundant between Local and Remote
DOMWindow. Should we pull at least the first few checks up to DOMWindow?

[dcheng, 2015-06-11 02:00:06]

https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
Source/core/frame/RemoteFrame.cpp:157: remoteFrameClient()->close();
On 2015/06/11 at 00:04:39, nasko wrote:
> On 2015/06/10 20:10:41, dcheng wrote:
> > LocalDOMWindow::close() has a bunch of other stuff. It seems like at least
some
> > of these other things are relevant for RemoteDOMWindow too, right?
> > 
> > For example, respecting the "allow scripts to close windows" setting,
setting
> > m_windowIsClosing, etc.
> 
> I've added those to RemoteDOMWindow.
>  
> > Also, can we just use ChromeClient::closeWindowSoon()?
> 
> I'd rather not, as it goes over to the widget and for remote frames we don't
have a widget on the content/ side, just a proxy.

Well... but we can change that to do what we need... the important part is
ChromeClient lets us get to the WebViewClient.

Logically, there's no real difference between closing a LocalDOMWindow and a
RemoteDOMWindow. Closing either one ultimately still requires the browser to do
things for us.

And if we do use ChromeClient::closeWindowSoon() here, I think we might be able
to just move the entire function up into DOMWindow, eliminating the duplicated
checks, etc.

https://codereview.chromium.org/1176843006/diff/60001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1176843006/diff/60001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:7336: runPendingTasks();
Is this necessary? If so, it'd be nice to document why.

[nasko, 2015-06-11 04:42:01]

On 2015/06/11 02:00:06, dcheng wrote:
>
https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
> File Source/core/frame/RemoteFrame.cpp (right):
> 
>
https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
> Source/core/frame/RemoteFrame.cpp:157: remoteFrameClient()->close();
> On 2015/06/11 at 00:04:39, nasko wrote:
> > On 2015/06/10 20:10:41, dcheng wrote:
> > > LocalDOMWindow::close() has a bunch of other stuff. It seems like at least
> some
> > > of these other things are relevant for RemoteDOMWindow too, right?
> > > 
> > > For example, respecting the "allow scripts to close windows" setting,
> setting
> > > m_windowIsClosing, etc.
> > 
> > I've added those to RemoteDOMWindow.
> >  
> > > Also, can we just use ChromeClient::closeWindowSoon()?
> > 
> > I'd rather not, as it goes over to the widget and for remote frames we don't
> have a widget on the content/ side, just a proxy.
> 
> Well... but we can change that to do what we need... the important part is
> ChromeClient lets us get to the WebViewClient.

Not exactly. ChromeClientImpl::closeWindowSoon calls
m_webView->client()->closeWidgetSoon(), which is a WebWidgetClient, not a
WebViewClient. We don't have a widget for the remote frame, so this isn't
exactly consistent either.

> Logically, there's no real difference between closing a LocalDOMWindow and a
> RemoteDOMWindow. Closing either one ultimately still requires the browser to
do
> things for us.
> 
> And if we do use ChromeClient::closeWindowSoon() here, I think we might be
able
> to just move the entire function up into DOMWindow, eliminating the duplicated
> checks, etc.
> 
>
https://codereview.chromium.org/1176843006/diff/60001/Source/web/tests/WebFra...
> File Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/1176843006/diff/60001/Source/web/tests/WebFra...
> Source/web/tests/WebFrameTest.cpp:7336: runPendingTasks();
> Is this necessary? If so, it'd be nice to document why.

[dcheng, 2015-06-11 04:47:55]

Yes, but we can change ChromeClient.

On Wed, Jun 10, 2015, 21:42  <nasko@chromium.org> wrote:

> On 2015/06/11 02:00:06, dcheng wrote:
>
>
>
https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
> > File Source/core/frame/RemoteFrame.cpp (right):
>
>
>
>
https://codereview.chromium.org/1176843006/diff/1/Source/core/frame/RemoteFra...
> > Source/core/frame/RemoteFrame.cpp:157: remoteFrameClient()->close();
> > On 2015/06/11 at 00:04:39, nasko wrote:
> > > On 2015/06/10 20:10:41, dcheng wrote:
> > > > LocalDOMWindow::close() has a bunch of other stuff. It seems like at
> > least
> > some
> > > > of these other things are relevant for RemoteDOMWindow too, right?
> > > >
> > > > For example, respecting the "allow scripts to close windows" setting,
> > setting
> > > > m_windowIsClosing, etc.
> > >
> > > I've added those to RemoteDOMWindow.
> > >
> > > > Also, can we just use ChromeClient::closeWindowSoon()?
> > >
> > > I'd rather not, as it goes over to the widget and for remote frames we
> > don't
> > have a widget on the content/ side, just a proxy.
>
> > Well... but we can change that to do what we need... the important part
> is
> > ChromeClient lets us get to the WebViewClient.
>
> Not exactly. ChromeClientImpl::closeWindowSoon calls
> m_webView->client()->closeWidgetSoon(), which is a WebWidgetClient, not a
> WebViewClient. We don't have a widget for the remote frame, so this isn't
> exactly consistent either.
>
> > Logically, there's no real difference between closing a LocalDOMWindow
> > and a
> > RemoteDOMWindow. Closing either one ultimately still requires the browser
> > to
> do
> > things for us.
>
> > And if we do use ChromeClient::closeWindowSoon() here, I think we might
> be
> able
> > to just move the entire function up into DOMWindow, eliminating the
> > duplicated
> > checks, etc.
>
>
>
>
https://codereview.chromium.org/1176843006/diff/60001/Source/web/tests/WebFra...
> > File Source/web/tests/WebFrameTest.cpp (right):
>
>
>
>
https://codereview.chromium.org/1176843006/diff/60001/Source/web/tests/WebFra...
> > Source/web/tests/WebFrameTest.cpp:7336: runPendingTasks();
> > Is this necessary? If so, it'd be nice to document why.
>
>
> https://codereview.chromium.org/1176843006/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium Site Isolation Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to site-isolation-reviews+unsubscribe@chromium.org.
> For more options, visit https://groups.google.com/a/chromium.org/d/optout.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[nasko, 2015-06-15 17:45:19]

Hey Daniel,

I've taken the road we've discussed of moving ::close into DOMWindow and out of
the Local/Remote objects. The change to fix up closeWindowSoon/closeWidgetSoon
will go in a separate CL to make these cleaner on their own.

Can you take another look at this one?

[dcheng, 2015-06-16 20:08:24]

(Sorry I had drafts and forgot to send them out!)

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:310: // FIXME: The session history length should
be implemented for RemoteFrame and
TODO(japhet) for this one? (if you're OK with this Nate: I have the vague
impression that we kind of need this for other stuff in one of the other layers)

Also, add a link to the crbug.

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:314: hasNavigated =
toLocalFrame(frame())->loader().client()->backForwardLength() > 1;
Maybe let's move this to FrameClient and leave the TODO to implement in
RemoteFrameClientImpl?

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:323: // FIXME: The shouldClose functionality
should be implemented for RemoteFrame
TODO(username) is the style now. Ditto on linking to crbug.

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:325: if (frame()->isLocalFrame() &&
!toLocalFrame(frame())->loader().shouldClose())
Same idea: maybe make this a virtual on Frame and leave the TODO to implement in
RemoteFrame?

https://codereview.chromium.org/1176843006/diff/100001/Source/web/ChromeClien...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1176843006/diff/100001/Source/web/ChromeClien...
Source/web/ChromeClientImpl.cpp:404: m_webView->mainFrame()->stopLoading();
Oof. We'll need to fix this too.

[nasko, 2015-06-16 22:58:54]

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:310: // FIXME: The session history length should
be implemented for RemoteFrame and
On 2015/06/16 20:08:23, dcheng wrote:
> TODO(japhet) for this one? (if you're OK with this Nate: I have the vague
> impression that we kind of need this for other stuff in one of the other
layers)
> 
> Also, add a link to the crbug.

Done.

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:314: hasNavigated =
toLocalFrame(frame())->loader().client()->backForwardLength() > 1;
On 2015/06/16 20:08:23, dcheng wrote:
> Maybe let's move this to FrameClient and leave the TODO to implement in
> RemoteFrameClientImpl?

Done.

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:323: // FIXME: The shouldClose functionality
should be implemented for RemoteFrame
On 2015/06/16 20:08:23, dcheng wrote:
> TODO(username) is the style now. Ditto on linking to crbug.

Done.

https://codereview.chromium.org/1176843006/diff/100001/Source/core/frame/DOMW...
Source/core/frame/DOMWindow.cpp:325: if (frame()->isLocalFrame() &&
!toLocalFrame(frame())->loader().shouldClose())
On 2015/06/16 20:08:23, dcheng wrote:
> Same idea: maybe make this a virtual on Frame and leave the TODO to implement
in
> RemoteFrame?

Done.

https://codereview.chromium.org/1176843006/diff/100001/Source/web/ChromeClien...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1176843006/diff/100001/Source/web/ChromeClien...
Source/web/ChromeClientImpl.cpp:404: m_webView->mainFrame()->stopLoading();
On 2015/06/16 20:08:23, dcheng wrote:
> Oof. We'll need to fix this too.

I didn't get to this one today :(. I will fix it once I'm back to a working
state.

[nasko, 2015-06-16 23:21:03]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-16 23:21:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176843006/140001

[commit-bot: I haz the power, 2015-06-17 00:47:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-17 00:47:41]

Dry run: This issue passed the CQ dry run.

[dcheng, 2015-06-17 23:39:50]

https://codereview.chromium.org/1176843006/diff/140001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1176843006/diff/140001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:355: return m_loader.shouldClose();
Let's add a TODO, since we need to fix this to work for OOPI too: right now, we
only dispatch beforeunload to in-process frames.

https://codereview.chromium.org/1176843006/diff/140001/Source/web/ChromeClien...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1176843006/diff/140001/Source/web/ChromeClien...
Source/web/ChromeClientImpl.cpp:403: if
(m_webView->mainFrame()->isWebLocalFrame())
TODO this?

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7340: TEST_F(WebFrameSwapTest,
WindowOpenRemoteClose)
I don't think there's any need to inherit from the test fixture, since we don't
really need the test fixture.

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7352: LocalFrame* localFrame =
toLocalFrame(toCoreFrame(mainFrame()));
Let's just use FrameTestHelpers::WebViewHelper directly here, since we don't
need all of the remote frame stuff.

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7365: reset();
Make sure you call view->close() and whatever else necessary so we don't leak
objects.

[nasko, 2015-06-19 13:23:23]

https://codereview.chromium.org/1176843006/diff/140001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1176843006/diff/140001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:355: return m_loader.shouldClose();
On 2015/06/17 23:39:50, dcheng wrote:
> Let's add a TODO, since we need to fix this to work for OOPI too: right now,
we
> only dispatch beforeunload to in-process frames.

Done.

https://codereview.chromium.org/1176843006/diff/140001/Source/web/ChromeClien...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1176843006/diff/140001/Source/web/ChromeClien...
Source/web/ChromeClientImpl.cpp:403: if
(m_webView->mainFrame()->isWebLocalFrame())
On 2015/06/17 23:39:50, dcheng wrote:
> TODO this?

Actually, the method is available on WebRemoteFrameImpl, but had a not reached
statement. I put a comment there and made this unconditional.

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7340: TEST_F(WebFrameSwapTest,
WindowOpenRemoteClose)
On 2015/06/17 23:39:50, dcheng wrote:
> I don't think there's any need to inherit from the test fixture, since we
don't
> really need the test fixture.

Do you mean inherit just from WebFrameTest instead of WebFrameSwap test? If so,
done.

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7352: LocalFrame* localFrame =
toLocalFrame(toCoreFrame(mainFrame()));
On 2015/06/17 23:39:50, dcheng wrote:
> Let's just use FrameTestHelpers::WebViewHelper directly here, since we don't
> need all of the remote frame stuff.

I'm not sure I follow here. I've changed this test from WebFrameSwapTest to
WebFrameTest, which meant I need to have a WebViewHelper for the initial view. I
still need to have another WebView for the opened window, which has a
RemoteFrame as its main frame. Is there a way to use WebViewHelper to create a
WebView with RemoteFrame as the main one? I didn't see it, as its initialize()
method uses WebLocaFrame in the call to setMainFrame.

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7365: reset();
On 2015/06/17 23:39:50, dcheng wrote:
> Make sure you call view->close() and whatever else necessary so we don't leak
> objects.

I thought view->close() is equivalent to the above call of
remoteFrame->domWindow()->close(). Fixed.

[dcheng, 2015-06-20 00:41:32]

lgtm with a nit

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1176843006/diff/140001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7352: LocalFrame* localFrame =
toLocalFrame(toCoreFrame(mainFrame()));
On 2015/06/19 at 13:23:22, nasko (paris) wrote:
> On 2015/06/17 23:39:50, dcheng wrote:
> > Let's just use FrameTestHelpers::WebViewHelper directly here, since we don't
> > need all of the remote frame stuff.
> 
> I'm not sure I follow here. I've changed this test from WebFrameSwapTest to
WebFrameTest, which meant I need to have a WebViewHelper for the initial view. I
still need to have another WebView for the opened window, which has a
RemoteFrame as its main frame. Is there a way to use WebViewHelper to create a
WebView with RemoteFrame as the main one? I didn't see it, as its initialize()
method uses WebLocaFrame in the call to setMainFrame.

What you have now is fine.

https://codereview.chromium.org/1176843006/diff/160001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1176843006/diff/160001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7368: mainWebView.reset();
Nit: probably not needed, since WebViewHelper should clean up after itself
(you're not doing anything particularly weird that would require this)

[nasko, 2015-06-22 12:47:45]

Responded to your comment. There might be a further issue to investigate, but
I'm not sure if it should be done as part of this CL. If you think this is good
to go, feel free to check the CQ box.

https://codereview.chromium.org/1176843006/diff/160001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1176843006/diff/160001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7368: mainWebView.reset();
On 2015/06/20 00:41:32, dcheng wrote:
> Nit: probably not needed, since WebViewHelper should clean up after itself
> (you're not doing anything particularly weird that would require this)

There is indeed a use-after-free happening in OpenedFrameTracker destructor if I
omit this. The issue is that once view->close() is called, the opened WebView is
no longer valid, yet the mainWebView has it in its list of opened windows.

I'm guessing that in regular chrome the DOMWindow::close and WebView::close
happen differently, as I wasn't able to reproduce that in the browser. Based on
reading of the code, I don't see what is different and how this works, so I'm
sure I'm missing something. Any ideas?

[nasko, 2015-06-22 12:47:57]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-22 12:48:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176843006/160001

[commit-bot: I haz the power, 2015-06-22 14:38:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-22 14:38:51]

Dry run: This issue passed the CQ dry run.

[nasko, 2015-06-23 21:39:03]

Daniel, do you think this is ready to go?

[nasko, 2015-06-23 21:39:22]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[nasko, 2015-06-23 21:39:23]

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/1176843006/#ps180001
(title: "Remove resetting of the main view.")

[commit-bot: I haz the power, 2015-06-23 21:40:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176843006/180001

[dcheng, 2015-06-23 21:40:56]

Yeah, I think there might be room for future cleanup in DOMWindow::close()...
but I'm happy to leave that for the future.

https://codereview.chromium.org/1176843006/diff/160001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1176843006/diff/160001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7368: mainWebView.reset();
On 2015/06/22 at 12:47:44, nasko (paris) wrote:
> On 2015/06/20 00:41:32, dcheng wrote:
> > Nit: probably not needed, since WebViewHelper should clean up after itself
> > (you're not doing anything particularly weird that would require this)
> 
> There is indeed a use-after-free happening in OpenedFrameTracker destructor if
I omit this. The issue is that once view->close() is called, the opened WebView
is no longer valid, yet the mainWebView has it in its list of opened windows.
> 
> I'm guessing that in regular chrome the DOMWindow::close and WebView::close
happen differently, as I wasn't able to reproduce that in the browser. Based on
reading of the code, I don't see what is different and how this works, so I'm
sure I'm missing something. Any ideas?

That's unexpected. Can you see if setOpener() is getting called when you call
view->close() below?

[dcheng, 2015-06-23 21:41:24]

(Ignore the draft, I forgot to remove it)

[commit-bot: I haz the power, 2015-06-23 23:17:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-23 23:17:34]

Dry run: This issue passed the CQ dry run.

[dcheng, 2015-06-24 00:35:46]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-06-24 00:36:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176843006/180001

[commit-bot: I haz the power, 2015-06-24 00:41:25]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197688