BrowserPlugin/WebView - Move plugin lifetime to DOM

BrowserPlugin/WebView - Move plugin lifetime to DOM

This CL adds a mechanism whereby a plugin can have its lifetime managed within an HTMLPlugInElement, including early plugin creation (i.e. before it has been attached to the DOM and assigned a renderer), and persistent lifetime so that it is not killed if its renderer is deleted (e.g. display = 'none' is set on the element). The CL is motivated by the desire to make WebView a sutom element, though the approach used should extend to other plugins on an opt-in basis.

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

[wjmaclean, 2013-10-07 19:16:14]

Eric - I've cleaned this up, can you take a look? The companion (Chromium-side)
CL is at https://codereview.chromium.org/23591016/, and includes some initial
tests for this.

[eseidel, 2013-10-17 14:40:44]

I think the idea of moving more plugin logic out of the renderers and into the
DOM is a good one.

I'm a bit wary about making this a mime-type based option.

It's also not clear to me from this CL what problem we're solving here.  Why
would a plugin want to be created when display-none?

We're considering moving plugins to function more like frames as part of the out
of process iframes (OOPI) work, and this feels like the opposite direction.

This is for when one wants to treat a plugin like a worker?  Or when one wants
to have a plugin which only plays audio?  (probably both!)

[wjmaclean, 2013-10-23 14:53:27]

On 2013/10/17 14:40:44, eseidel wrote:
> I think the idea of moving more plugin logic out of the renderers and into the
> DOM is a good one.
> 
> I'm a bit wary about making this a mime-type based option.

The reason I chose this is that a WebView element can be created in JavaScript,
and configured (size, src, JavaScript injected, etc.), before it is attached to
the DOM. The setting of the mime-type is the first clear indication that we'll
(eventually) need to create a BrowserPlugin, and so this seemed like a natural
place to do this. This differs from the OOPIF case in that an OOPIF will never
have a mime-type, or at least not a browser plugin mime-type (correct me if I'm
wrong).

> It's also not clear to me from this CL what problem we're solving here.  Why
> would a plugin want to be created when display-none?

Apparently many developers what to be able to hide/re-display WebView tags using
display none. One example is creating a multi-tab browser where each tab is a
WebView, but only one tab can be visible at a time. At present, the only option
for a developer is to created an attached WebView, but translate it offscreen
until it is needed. But this is a work-around, and developers do not feel
display:none "works as intended" with WebView at present.

Another example is creating a WebView and using it to load a page while still
hidden, then turning it on to display to the user.

> We're considering moving plugins to function more like frames as part of the
out
> of process iframes (OOPI) work, and this feels like the opposite direction.

Will OOPI use BrowserPlugin?

> This is for when one wants to treat a plugin like a worker?  Or when one wants
> to have a plugin which only plays audio?  (probably both!)

I don't think the motivation was ever really an audio-only plugin ... it's
primarily driven by WebView development. I'm not sure if that qualifies as a
"worker" or not, though I'm guessing not.

[eseidel, 2013-10-28 16:59:29]

So this is a Widget which can exist before, and persist after a RenderWidget,
correct?  How is the plugin notified of the construction/destruction of the
view?

https://codereview.chromium.org/23618022/diff/6001/Source/core/html/HTMLPlugI...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/23618022/diff/6001/Source/core/html/HTMLPlugI...
Source/core/html/HTMLPlugInElement.h:75: PassRefPtr<Widget> plugin();
Unless this is passing ownership, I doubt you want PassRefPtr?  I assume this is
just a getter, and you can use Widget* ?

[eseidel, 2013-10-28 17:11:00]

What happens when you reparent one of these into a differnet DOM tree?

[abarth-chromium, 2013-10-28 17:50:34]

sutom -> custom

[eseidel, 2013-10-28 17:54:59]

Discussed offline:  <browser>, <webview> and <adview> are apps-only features.

We want to load all plugins like this.  We have to maintain backwards
compatibility with old plugins to have them die when the renderers die.

I don't think we want to support two code-paths for loading plugins however.  We
should first switch all plugins to this model and then add support for plugins
to not have the display: none behavior.

That's a much harder problem to solve, but I believe the right way for the
codebase long-term.

I'm happy to review incremental changes to introduce this new dom-based loading
system and move all plugins onto it.

[Fady Samuel, 2013-10-28 17:59:23]

On 2013/10/28 16:59:29, eseidel wrote:
> So this is a Widget which can exist before, and persist after a RenderWidget,
> correct?  How is the plugin notified of the construction/destruction of the
> view?
> 
>
https://codereview.chromium.org/23618022/diff/6001/Source/core/html/HTMLPlugI...
> File Source/core/html/HTMLPlugInElement.h (right):
> 
>
https://codereview.chromium.org/23618022/diff/6001/Source/core/html/HTMLPlugI...
> Source/core/html/HTMLPlugInElement.h:75: PassRefPtr<Widget> plugin();
> Unless this is passing ownership, I doubt you want PassRefPtr?  I assume this
is
> just a getter, and you can use Widget* ?

I agree, it would be useful to inform the plugin on the construction/destruction
of its view. For example, we might want to call WebContents::WasHidden when the
view is gone in the browser process and restore the WebContents visibility state
when a new view is constructed.

[eseidel, 2013-10-31 17:59:24]

We talked a bit offline.  My two issues with this patch:
1.  There are now two ways to hold onto a RefPtr<Widget> one from the DOM and
one from the RenderWidget.  That's bad (confusing).  We'd like to have one, and
the DOM way is the way of the future.  I think we could split this patch and
just move ownership of the RefPtr<Widget> to the DOM *for all plugins* and add
the appropriate notifications of renderer creation/destruction (calls form
attach/detach or from RenderWidget()/~RenderWidget() unless esprehn has better
suggestions) to make that new location still keep the old symantics.

2.  There is currently no way to test this in Blink.  I suspect once we did #1
above, we could add an Internals flag to allow testing this new lifetime in a
LayoutTest with the TestPlugin.

If those two were done, I think this opt-in for <browser> is great.  NaCl would
love to have this I'm sure.  I think all PPAPI plugins going forward should be
DOM-lifetime.

[wjmaclean, 2013-11-11 22:15:50]

Eric, can you please take a look? Is this sort of what you had in mind?

A couple of comments:

1) I chose not to combine createPluginWithoutRenderer() and loadPlugin() in this
patch, though it certainly can be done if you like. But the two functions are
different enough I thought it would be clearer to keep them separate.

2) In addition to plugins, there are other widget types that get stored on
RenderWidget (HTMLAppletElement, and various Frame views). *Temporarily*, I've
introduced two parallel code paths in RenderWidget, one for widgets whose
pointers are stored in RenderWidget, and one for widgets whose pointers live in
the DOM. Presumably we'll collapse this to a single path when Frame/Applet
widgets also store their own pointers. When that happens, I'm pretty sure we can
extract RenderWidget from the render hierarchy, though we will likely want to
keep the common machinery for attaching/detaching widget pointers in one place
(perhaps something like "DOMWidget", at least until Widgets disappear
entirely?).

Finally, layout tests on ToT seem really broken today, so I don't yet know how
this CL fares, though it seems to run fine on Linux-Chrome as far as I've been
able to test it.

[wjmaclean, 2013-11-12 14:57:35]

On 2013/11/11 22:15:50, wjmaclean wrote:
> Eric, can you please take a look? Is this sort of what you had in mind?
> 
> A couple of comments:
> 
> 1) I chose not to combine createPluginWithoutRenderer() and loadPlugin() in
this
> patch, though it certainly can be done if you like. But the two functions are
> different enough I thought it would be clearer to keep them separate.
> 
> 2) In addition to plugins, there are other widget types that get stored on
> RenderWidget (HTMLAppletElement, and various Frame views). *Temporarily*, I've
> introduced two parallel code paths in RenderWidget, one for widgets whose
> pointers are stored in RenderWidget, and one for widgets whose pointers live
in
> the DOM. Presumably we'll collapse this to a single path when Frame/Applet
> widgets also store their own pointers. When that happens, I'm pretty sure we
can
> extract RenderWidget from the render hierarchy, though we will likely want to
> keep the common machinery for attaching/detaching widget pointers in one place
> (perhaps something like "DOMWidget", at least until Widgets disappear
> entirely?).
> 
> Finally, layout tests on ToT seem really broken today, so I don't yet know how
> this CL fares, though it seems to run fine on Linux-Chrome as far as I've been
> able to test it.

I re-built with use_aura=0 and ran the layout test, and everything appeared to
run fine (modulo usual flake, though nothing that appeared related to plugins).
In particular, the *plugin* layout tests all ran as expected.

[eseidel, 2013-11-18 22:09:53]

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:133: if (m_widget.get())
Can we just remove RenderWidget's m_widget pointer?  You can't have anonymous
renderers with plugins to my knowledge, so can't we just remove this check?

[eseidel, 2013-11-18 22:26:44]

I think this is the right direction.  But I think we should complete the "move
everything to the DOM way" before we bother adding the browser bits.  We can add
a way to test this with the test_plugin.  I'm happy you get that running if you
need.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLObj...
File Source/core/html/HTMLObjectElement.cpp (left):

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLObj...
Source/core/html/HTMLObjectElement.cpp:100: setNeedsWidgetUpdate(true);
I take it we're trying to avoid synchronously creating the plugin at this
callstack?  Presumably because plugin creation can do all sorts of things and
may cause us to reenter?

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:147: if (m_plugin && (mimeType ==
m_pluginMimeType))
I'm confused by this check.  It feels like it should be outside of this
function.  This is guarding against calling this twice with the same mimetype,
but I can imagine cases where you'd want this to succeed with the same mimetype
(replacing a plugin with a different one, no?), so it seems this check belongs
at a higher level, or we should aSSERT(!m_plugin) instead of checking.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:158: paramNames.append(String("type"));
Shouldn't need an explicit String() wrapper here?

Why do we need to create fake paramNames, the DOM should have them.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:489: bool loadManually =
document().isPluginDocument() && !document().containsPlugins() &&
toPluginDocument(document()).shouldLoadPluginManually();
There should only be one way to create a plugin.  This code shouldn't need to be
sensitive to there being a second way. :(

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:499: // FIXME: What happens here if our
widget *isn't* a plugin container?
That can't happen.  We're an HTMLPluginElement, we should only have Plugin
subclasses of Widget.  We can ASSERT that here.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:68: PassRefPtr<Widget> plugin();
Normally you don't return PassRefPtr unless the caller is expected to take
ownership (like a create() method).  In this case, you probably just want
Widget* indicating that this object retains ownership and that the Widget*
should last as long as it does.

https://codereview.chromium.org/23618022/diff/351001/Source/core/loader/Empty...
File Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/loader/Empty...
Source/core/loader/EmptyClients.h:237: virtual PassRefPtr<Widget>
createPlugin(const IntSize&, HTMLPlugInElement*, const KURL&, const
Vector<String>&, const Vector<String>&, const String&, bool, bool = false)
OVERRIDE;
Bools as arguments are sad-panda.  Enums are much easier to read.

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:198: // FIXME: Once
Frame/FrameElementBase/FrameOwnerElement and HTMLAppletElement
I see.  The problem is there are multiple DOM classes which know how to have a
Widget?  Seems we should just move them all over before adding the browser bits.

I understand you're here for the browser bits.  I'm just not comfortable with
the complexity this change adds to the core code.  We should fix the core code
to make your usecase the easy case first, imo.

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:243: // type == ConfigureOnAttach.
confused.  Which one of these is the old path vs. the new path?

[wjmaclean, 2013-11-25 17:51:55]

Here's a WIP patch that (I think) addresses your comments, but still needs some
work (or at least, broader testing). Let me know if you think the direction of
this seems right.

I'm not sure about how all the plugin layout tests will react to this ... many
of them seem disabled on Linux.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLObj...
File Source/core/html/HTMLObjectElement.cpp (left):

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLObj...
Source/core/html/HTMLObjectElement.cpp:100: setNeedsWidgetUpdate(true);
On 2013/11/18 22:26:45, eseidel wrote:
> I take it we're trying to avoid synchronously creating the plugin at this
> callstack?  Presumably because plugin creation can do all sorts of things and
> may cause us to reenter?

This needs to be investigated further, but I'm guessing if re-entrancy is
possible, it's due to layout issues. Would that be a problem if we are creating
the plugin without a renderer?

Nonetheless, I've tried the approach of calling setNeedsWidgetUpdate(true) with
or without a renderer and then in HTMLPlugInElement::updateWidgetIfNecessary()
create the plugin without a renderer for supported plugin types. So far so good,
but I'm continuing to test it ...

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:147: if (m_plugin && (mimeType ==
m_pluginMimeType))
On 2013/11/18 22:26:45, eseidel wrote:
> I'm confused by this check.  It feels like it should be outside of this
> function.  This is guarding against calling this twice with the same mimetype,
> but I can imagine cases where you'd want this to succeed with the same
mimetype
> (replacing a plugin with a different one, no?), so it seems this check belongs
> at a higher level, or we should aSSERT(!m_plugin) instead of checking.

Actually, it might be sensible that, if someone sets the mime-type to the same
value it has at present, we should still create a new plugin. I'll revise
accordingly.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:158: paramNames.append(String("type"));
On 2013/11/18 22:26:45, eseidel wrote:
> Shouldn't need an explicit String() wrapper here?
> 
> Why do we need to create fake paramNames, the DOM should have them.

Done.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:489: bool loadManually =
document().isPluginDocument() && !document().containsPlugins() &&
toPluginDocument(document()).shouldLoadPluginManually();
On 2013/11/18 22:26:45, eseidel wrote:
> There should only be one way to create a plugin.  This code shouldn't need to
be
> sensitive to there being a second way. :(

Done.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:499: // FIXME: What happens here if our
widget *isn't* a plugin container?
On 2013/11/18 22:26:45, eseidel wrote:
> That can't happen.  We're an HTMLPluginElement, we should only have Plugin
> subclasses of Widget.  We can ASSERT that here.

Done.

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:68: PassRefPtr<Widget> plugin();
On 2013/11/18 22:26:45, eseidel wrote:
> Normally you don't return PassRefPtr unless the caller is expected to take
> ownership (like a create() method).  In this case, you probably just want
> Widget* indicating that this object retains ownership and that the Widget*
> should last as long as it does.

Yes ... I think I changed this in response to a compiler error I saw, and wasn't
thinking very carefully about what I was doing.

Fixed.

https://codereview.chromium.org/23618022/diff/351001/Source/core/loader/Empty...
File Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/loader/Empty...
Source/core/loader/EmptyClients.h:237: virtual PassRefPtr<Widget>
createPlugin(const IntSize&, HTMLPlugInElement*, const KURL&, const
Vector<String>&, const Vector<String>&, const String&, bool, bool = false)
OVERRIDE;
On 2013/11/18 22:26:45, eseidel wrote:
> Bools as arguments are sad-panda.  Enums are much easier to read.

Sad-panda happy now ... fixed. :-)

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:133: if (m_widget.get())
On 2013/11/18 22:09:54, eseidel wrote:
> Can we just remove RenderWidget's m_widget pointer?  You can't have anonymous
> renderers with plugins to my knowledge, so can't we just remove this check?

I've taken a stab at this .. please take a look and let me know if anything
looks overly naive.

Done.

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:198: // FIXME: Once
Frame/FrameElementBase/FrameOwnerElement and HTMLAppletElement
On 2013/11/18 22:26:45, eseidel wrote:
> I see.  The problem is there are multiple DOM classes which know how to have a
> Widget?  Seems we should just move them all over before adding the browser
bits.
> 
> I understand you're here for the browser bits.  I'm just not comfortable with
> the complexity this change adds to the core code.  We should fix the core code
> to make your usecase the easy case first, imo.

As per comment regarding removing widget pointer.

Done.

https://codereview.chromium.org/23618022/diff/351001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:243: // type == ConfigureOnAttach.
On 2013/11/18 22:26:45, eseidel wrote:
> confused.  Which one of these is the old path vs. the new path?

It's not so much an old vs. new distinction as attach/detach. I've restructured
this code in the new patch in a way that will hopefully clarify this.

[eseidel, 2013-12-02 16:28:34]

This looks great!  All we need is testing and this looks good to land.

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLObj...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLObj...
Source/core/html/HTMLObjectElement.cpp:101: setNeedsWidgetUpdate(true);
But we don't have the data yet, do we?

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:187: if (plugin) {
Just combine these two ifs into one line?

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:491: // FIXME: We assume we don't layout
without a renderer, but check this assumption.
style recalc creates/destroys renderers.  I doubt you need to check (renderer)
here.

https://codereview.chromium.org/23618022/diff/461001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/461001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:218: // If the widget has no parent, it
means it's already detached.
Detach can be called twice?

https://codereview.chromium.org/23618022/diff/461001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/23618022/diff/461001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.h:98: virtual bool pluginShouldPersist()
const;
OVERRIDE?

[wjmaclean, 2013-12-02 21:51:55]

This version of the CL fixes a lot of layout tests ... still working on two
plugin tests that timeout.

I'm starting to work on some tests for the new pathways.

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLObj...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLObj...
Source/core/html/HTMLObjectElement.cpp:101: setNeedsWidgetUpdate(true);
On 2013/12/02 16:28:34, eseidel wrote:
> But we don't have the data yet, do we?

No, but we can check when we run the widget update code.

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:187: if (plugin) {
On 2013/12/02 16:28:34, eseidel wrote:
> Just combine these two ifs into one line?

This disappears in the new patch :-)

https://codereview.chromium.org/23618022/diff/461001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:491: // FIXME: We assume we don't layout
without a renderer, but check this assumption.
On 2013/12/02 16:28:34, eseidel wrote:
> style recalc creates/destroys renderers.  I doubt you need to check (renderer)
> here.

Thanks, that's useful to know. I'll remove the if() and re-test the cl.

https://codereview.chromium.org/23618022/diff/461001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/461001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:218: // If the widget has no parent, it
means it's already detached.
On 2013/12/02 16:28:34, eseidel wrote:
> Detach can be called twice?

That was a bug I was trying to resolve in this CL. In the new CL, this function
is gone.

https://codereview.chromium.org/23618022/diff/461001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/23618022/diff/461001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.h:98: virtual bool pluginShouldPersist()
const;
On 2013/12/02 16:28:34, eseidel wrote:
> OVERRIDE?

Thanks, I missed that!

[wjmaclean, 2013-12-12 20:30:46]

PTAL

Tests added.

This likely needs to be re-based, but otherwise I think we're (one step) closer.

https://codereview.chromium.org/23618022/diff/501001/Source/testing/runner/Te...
File Source/testing/runner/TestPlugin.cpp (right):

https://codereview.chromium.org/23618022/diff/501001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:286: return nameString ==
kPropertyInstanceId || nameString == kPropertyIsPersistent || nameString ==
kPropertyCanCreateWithoutRenderer;
Sorry about the length of this line, but presubmit wouldn't let me line it up on
multiple lines in any manner that seemed reasonable.

[eseidel, 2013-12-12 20:40:12]

I have a meeting to run to, but I'm happy to look again this afternoon.

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/can-...
File LayoutTests/plugins/can-create-without-renderer.html (right):

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/can-...
LayoutTests/plugins/can-create-without-renderer.html:34:
document.body.offsetTop;
Do you still need this?

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/plug...
File LayoutTests/plugins/plugin-persists.html (right):

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/plug...
LayoutTests/plugins/plugin-persists.html:32: document.body.offsetTop;
I might use plugin.offsetTop to make sure we don't optimize skipping layout for
that element.

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:187: if (!renderWidget ||
!renderWidget->isWidget())
What does isWidget() do here?

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:191: if (renderWidget->style()) {
It feels liek this wants to call some method on the renderer.  The rest of this
function looks like code which belongs in the rendering tree, if still called
from this DOM.

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:195: if
(renderWidget->style()->visibility() != VISIBLE) {
This all seems suspect.  The DOM shouldn't be dealing with things like
repainting.

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:137: RefPtr<Widget>
m_persistedPluginWidget;
Why do we have two RefPtr<Widget>s in this patch?  Do these classes no longer
share a parent?

[wjmaclean, 2013-12-12 22:31:14]

Revised ... ptal :-)

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/can-...
File LayoutTests/plugins/can-create-without-renderer.html (right):

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/can-...
LayoutTests/plugins/can-create-without-renderer.html:34:
document.body.offsetTop;
On 2013/12/12 20:40:13, eseidel wrote:
> Do you still need this?

I'll check ... nope, seems ok without (we *shouldn't* need it, now you mention
it)

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/plug...
File LayoutTests/plugins/plugin-persists.html (right):

https://codereview.chromium.org/23618022/diff/501001/LayoutTests/plugins/plug...
LayoutTests/plugins/plugin-persists.html:32: document.body.offsetTop;
On 2013/12/12 20:40:13, eseidel wrote:
> I might use plugin.offsetTop to make sure we don't optimize skipping layout
for
> that element.

Thanks, will give that a try.

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:187: if (!renderWidget ||
!renderWidget->isWidget())
On 2013/12/12 20:40:13, eseidel wrote:
> What does isWidget() do here?

I'm not entirely sure, but even RenderWidget() was making this check, so I was
loathe to remove it. I will try to remove it and see what happens.

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:191: if (renderWidget->style()) {
On 2013/12/12 20:40:13, eseidel wrote:
> It feels liek this wants to call some method on the renderer.  The rest of
this
> function looks like code which belongs in the rendering tree, if still called
> from this DOM.

OK.

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:195: if
(renderWidget->style()->visibility() != VISIBLE) {
On 2013/12/12 20:40:13, eseidel wrote:
> This all seems suspect.  The DOM shouldn't be dealing with things like
> repainting.

Is it ok if dealt with as described above? Or do we need to call something like
needsRepaint() on the renderer?

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/23618022/diff/501001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:137: RefPtr<Widget>
m_persistedPluginWidget;
On 2013/12/12 20:40:13, eseidel wrote:
> Why do we have two RefPtr<Widget>s in this patch?  Do these classes no longer
> share a parent?

Since "persistence" is a plugin-only concept, and HTMLFrameOwnerElement serves
plugins and non-plugins, I decided to move the pointer to this var while it's
being persisted. This also simplifies the logic in
HTMLFrameOwnerElement::setWidget(), and at any time the plugin widget pointer is
only ever in one of these locations.

[eseidel, 2013-12-18 18:38:17]

What's the benefit to using an extension to the mimetyp instead of just adding a
new one?  You're effectively adding a new one already.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:57: WidgetToParentMap map;
nit: I might have pushed this block into a helper function (just to give it a
nice name, etc.)

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:71: , m_persistedPluginWidget(0)
nit: RefPtr's default construtor sets to 0, no need for this line.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:500: // FIXME: We assume we don't layout
without a renderer, but check this assumption.
I'm not sure I understand this comment.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:507: // FIXME: it may be that nothing has
change (we might be continuing to
No, the recalc style is only needed to trigger renderer creation/destruction.

https://codereview.chromium.org/23618022/diff/581001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:277: if (style()) {
nit: early return if (!style) return;

https://codereview.chromium.org/23618022/diff/581001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:282: widget->hide();
Do we need to cause a repaint to when hiding (to repaint the content behind?) 
If not, then why do we have to cause one on show?

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
File Source/testing/runner/TestPlugin.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:76: // Mac defines these elsewhere and
doesn't like duplicates.
Why are we defining these in TestPlugin.cpp in teh first place?  There must be a
header this should have incldued instead...

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:175: ,
m_canCreateWithoutRenderer(params.mimeType.utf8().find(canCreateWithoutRendererSuffix().utf8())
!= std::string::npos)
Why all the .utf8?  Does this not have a contains()?

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:281: free(nputF8);
Do we have a OwnPtr-like concept for this instead?

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:302:
INT32_TO_NPVARIANT(static_cast<TestPluginNPObject*>(npObject)->m_plugin->instanceId(),
*result);
Should we pull this cast out into a local?

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/We...
File Source/testing/runner/WebTestProxy.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/We...
Source/testing/runner/WebTestProxy.cpp:926: if
(params.mimeType.utf8().find(TestPlugin::mimeType().utf8(), 0) !=
std::string::npos)
TestPlugin::isSupportedMimeType(params.mimeType) would be much better here.

[eseidel, 2013-12-18 18:41:52]

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:137: RefPtr<Widget>
m_persistedPluginWidget;
I would add a comment here explaining why we have this:

// Normally the Widget is stored in HTMLFrameOwnerElement::m_widget.
// However, plugins can persist even when not rendered. In order to
// prevent confusing code which may assume that widget() != null
// means the frame is active, we save off m_widget here while
// the plugin is persisting but not being displayed.

[wjmaclean, 2013-12-23 23:49:52]

CL updated to address comments, and split (new cl is
https://codereview.chromium.org/105633011) to accommodate the move of test
runner from Blink to Chromium.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:57: WidgetToParentMap map;
On 2013/12/18 18:38:18, eseidel wrote:
> nit: I might have pushed this block into a helper function (just to give it a
> nice name, etc.)

Done.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:71: , m_persistedPluginWidget(0)
On 2013/12/18 18:38:18, eseidel wrote:
> nit: RefPtr's default construtor sets to 0, no need for this line.

Done.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:500: // FIXME: We assume we don't layout
without a renderer, but check this assumption.
On 2013/12/18 18:38:18, eseidel wrote:
> I'm not sure I understand this comment.

This was just a reminder to myself ... removed.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.cpp:507: // FIXME: it may be that nothing has
change (we might be continuing to
On 2013/12/18 18:38:18, eseidel wrote:
> No, the recalc style is only needed to trigger renderer creation/destruction.

recalc style removed.

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:137: RefPtr<Widget>
m_persistedPluginWidget;
On 2013/12/18 18:41:53, eseidel wrote:
> I would add a comment here explaining why we have this:
> 
> // Normally the Widget is stored in HTMLFrameOwnerElement::m_widget.
> // However, plugins can persist even when not rendered. In order to
> // prevent confusing code which may assume that widget() != null
> // means the frame is active, we save off m_widget here while
> // the plugin is persisting but not being displayed.

Done.

https://codereview.chromium.org/23618022/diff/581001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:277: if (style()) {
On 2013/12/18 18:38:18, eseidel wrote:
> nit: early return if (!style) return;

Done.

https://codereview.chromium.org/23618022/diff/581001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:282: widget->hide();
On 2013/12/18 18:38:18, eseidel wrote:
> Do we need to cause a repaint to when hiding (to repaint the content behind?) 
> If not, then why do we have to cause one on show?

I'm afraid I don't really know the answer to that ... I'm just replicating the
previous behaviour.

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
File Source/testing/runner/TestPlugin.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:76: // Mac defines these elsewhere and
doesn't like duplicates.
On 2013/12/18 18:38:18, eseidel wrote:
> Why are we defining these in TestPlugin.cpp in teh first place?  There must be
a
> header this should have incldued instead...

I'm not sure, though I would imagine that one reason would be just what is
described here, namely that some system headers duplicate them already, and
duplicated them from gl2.h as indicated earlier in the file. But including gl2.h
would cause similar collisions, and in a file that's in 3rd party and thus
harder to fix.

I'm really not sure what the right fix is here.

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:175: ,
m_canCreateWithoutRenderer(params.mimeType.utf8().find(canCreateWithoutRendererSuffix().utf8())
!= std::string::npos)
On 2013/12/18 18:38:18, eseidel wrote:
> Why all the .utf8?  Does this not have a contains()?

No longer using find()

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:281: free(nputF8);
On 2013/12/18 18:38:18, eseidel wrote:
> Do we have a OwnPtr-like concept for this instead?

I'm unsure ... aren't all the owned-ptr types based on new/delete?

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/Te...
Source/testing/runner/TestPlugin.cpp:302:
INT32_TO_NPVARIANT(static_cast<TestPluginNPObject*>(npObject)->m_plugin->instanceId(),
*result);
On 2013/12/18 18:38:18, eseidel wrote:
> Should we pull this cast out into a local?

Done.

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/We...
File Source/testing/runner/WebTestProxy.cpp (right):

https://codereview.chromium.org/23618022/diff/581001/Source/testing/runner/We...
Source/testing/runner/WebTestProxy.cpp:926: if
(params.mimeType.utf8().find(TestPlugin::mimeType().utf8(), 0) !=
std::string::npos)
On 2013/12/18 18:38:18, eseidel wrote:
> TestPlugin::isSupportedMimeType(params.mimeType) would be much better here.

Done.

[eseidel, 2013-12-24 00:02:04]

This looks good.  I'll need to give it another read though.  (I'm not
technically in the office again until Jan.)

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:245: if (ownerElement && ownerElement->widget()
== this)
I woudl just ASSERT(ownerElement->widget() == this);.  I'm having trouble
imagining when that would not be the case...

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:459: 
spurious.

https://codereview.chromium.org/23618022/diff/601001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/601001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:287: repaint();
Since we don't understand the unpaired repaint() here, I would leave a FIXME:
asking why we repaint in one case but not the other.

[wjmaclean, 2013-12-24 00:04:30]

On 2013/12/24 00:02:04, eseidel wrote:
> This looks good.  I'll need to give it another read though.  (I'm not
> technically in the office again until Jan.)
> 
>
https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.cpp:245: if (ownerElement &&
ownerElement->widget()
> == this)
> I woudl just ASSERT(ownerElement->widget() == this);.  I'm having trouble
> imagining when that would not be the case...
> 
>
https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.h (left):
> 
>
https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.h:459: 
> spurious.
> 
>
https://codereview.chromium.org/23618022/diff/601001/Source/core/rendering/Re...
> File Source/core/rendering/RenderWidget.cpp (right):
> 
>
https://codereview.chromium.org/23618022/diff/601001/Source/core/rendering/Re...
> Source/core/rendering/RenderWidget.cpp:287: repaint();
> Since we don't understand the unpaired repaint() here, I would leave a FIXME:
> asking why we repaint in one case but not the other.

Thanks, I'll update in the meantime.

[wjmaclean, 2013-12-24 00:14:31]

Comments addressed.

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:245: if (ownerElement && ownerElement->widget()
== this)
On 2013/12/24 00:02:04, eseidel wrote:
> I woudl just ASSERT(ownerElement->widget() == this);.  I'm having trouble
> imagining when that would not be the case...

Done.

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/23618022/diff/601001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:459: 
On 2013/12/24 00:02:04, eseidel wrote:
> spurious.

Done.

https://codereview.chromium.org/23618022/diff/601001/Source/core/rendering/Re...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/23618022/diff/601001/Source/core/rendering/Re...
Source/core/rendering/RenderWidget.cpp:287: repaint();
On 2013/12/24 00:02:04, eseidel wrote:
> Since we don't understand the unpaired repaint() here, I would leave a FIXME:
> asking why we repaint in one case but not the other.

Done.

[eseidel, 2014-01-07 22:54:27]

lgtm.  I suspect we could test more.

I also suspect that you may have fallout from this change that we'll have to
fix.  But that's to be expected when changing large, not-well-tested pieces of
blink like this. :)

https://codereview.chromium.org/23618022/diff/701001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/23618022/diff/701001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:245: if (ownerElement && ownerElement->widget()
== this)
I would just ASSERT(ownerElement->widghet() == this) here, no?  If we were
detatched from our owner element we should also ahve cleared our pointer to it,
no?

[wjmaclean, 2014-01-08 14:36:19]

https://codereview.chromium.org/23618022/diff/701001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/23618022/diff/701001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:245: if (ownerElement && ownerElement->widget()
== this)
On 2014/01/07 22:54:28, eseidel wrote:
> I would just ASSERT(ownerElement->widghet() == this) here, no?  If we were
> detatched from our owner element we should also ahve cleared our pointer to
it,
> no?

Maybe I'm misunderstanding, but ownerElement has been cleared, wouldn't the
ASSERT seg fault? I (vaguely) recall the reason I wrote it this way is that
sometimes ownerElement is 0x0.

[wjmaclean, 2014-01-08 15:59:21]

I've rebased this patch, and found I needed to add some guards on access to the
frame->loader().client() return value in
ProgressTracker::finalProgressComplete() as I was seeing seg-faults in a unit
test:  WebViewTest.HelperPlugin

The failure doesn't occur without this CL, though I am at present unsure about
how this CL changes the client lifetime. Let me know if the guards seem
reasonable (if perhaps only for the short term - I can file a bug against this).

[eseidel, 2014-01-08 17:22:34]

Frame and FrameLoader client lifetimes have been changign as of late thanks to
some OOPI changes from dcheng.  I wouldn't expect your change to change
FrameLoaderClient (WebFrame) lifetimes, no.

[dcheng, 2014-02-25 07:02:33]

https://codereview.chromium.org/23618022/diff/1951001/Source/core/loader/Prog...
File Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/23618022/diff/1951001/Source/core/loader/Prog...
Source/core/loader/ProgressTracker.cpp:139: if (!m_finalProgressChangedSent &&
frame->loader().client()) {
Do you still need these? I think I moved a few things around to prefer null
derefs here and these should (hopefully) no longer be needed.

[wjmaclean, 2014-02-25 14:07:11]

On 2014/02/25 07:02:33, dcheng wrote:
>
https://codereview.chromium.org/23618022/diff/1951001/Source/core/loader/Prog...
> File Source/core/loader/ProgressTracker.cpp (right):
> 
>
https://codereview.chromium.org/23618022/diff/1951001/Source/core/loader/Prog...
> Source/core/loader/ProgressTracker.cpp:139: if (!m_finalProgressChangedSent &&
> frame->loader().client()) {
> Do you still need these? I think I moved a few things around to prefer null
> derefs here and these should (hopefully) no longer be needed.

I'll test and let you know.If they're not needed anymore I'll upload a new CL
without them.

[wjmaclean, 2014-03-11 16:00:08]

eseidel@ - I think this is ready to try, did you want to give it another look? I
had to add a path for WebPluginContainer to notify its WebPlugin when the former
is detached from the tree.

[eseidel, 2014-03-11 18:00:54]

lgtm

[wjmaclean, 2014-03-11 19:33:58]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-03-11 19:34:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/23618022/2281001

[commit-bot: I haz the power, 2014-03-11 20:20:59]

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

[commit-bot: I haz the power, 2014-03-11 20:21:00]

Try jobs failed on following builders: mac_blink_rel

[wjmaclean, 2014-03-27 19:07:58]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-03-27 19:08:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/23618022/2361001

[commit-bot: I haz the power, 2014-03-27 19:08:39]

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

[commit-bot: I haz the power, 2014-03-27 19:08:40]

Failed to apply patch for Source/core/loader/ProgressTracker.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/loader/ProgressTracker.cpp
  Hunk #1 FAILED at 136.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/loader/ProgressTracker.cpp.rej

Patch:       Source/core/loader/ProgressTracker.cpp
Index: Source/core/loader/ProgressTracker.cpp
diff --git a/Source/core/loader/ProgressTracker.cpp
b/Source/core/loader/ProgressTracker.cpp
index
be2b29abe4f1b4b93e6d5951452749f064df525e..f87ac6efeecb7c2997d36a95607c9ec6a2a76a27
100644
--- a/Source/core/loader/ProgressTracker.cpp
+++ b/Source/core/loader/ProgressTracker.cpp
@@ -136,6 +136,7 @@ void ProgressTracker::finalProgressComplete()
 
     // Before resetting progress value be sure to send client a least one
notification
     // with final progress value.
+    ASSERT(frame->loader().client());
     if (!m_finalProgressChangedSent) {
         m_progressValue = 1;
         frame->loader().client()->postProgressEstimateChangedNotification();

[wjmaclean, 2014-03-27 19:24:48]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-03-27 19:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/23618022/2371001

[commit-bot: I haz the power, 2014-03-27 19:58:34]

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

[commit-bot: I haz the power, 2014-03-27 19:58:35]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[wjmaclean, 2014-04-03 19:33:57]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-04-03 19:34:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/23618022/2391001

[commit-bot: I haz the power, 2014-04-03 20:51:48]

Change committed as 170791