Force layout and invalidation for new plugin content

Force layout and invalidation for new plugin content

Correct paint invalidation was not occurring for the plugin placeholder
content in Chromium when WebPluginContainer paint invalidation is
delayed until the paint invalidation phase of the document lifecycle.

Here we set the needs layout and needs full paint invalidation flags so that layout is re-done on the plugin content at the next opportunity.

Depends on Blink review https://codereview.chromium.org/1056893003

R=tommycli
BUG=472348
Committed: https://crrev.com/c3bbc172f04db541b9da820a10bf4583e2c24e28
Cr-Commit-Position: refs/heads/master@{#324127}

[Stephen Chennney, 2015-04-01 19:57:40]

I think we need some test coverage for this. Any ideas on how to do that?

[Stephen Chennney, 2015-04-02 19:12:32]

schenney@chromium.org changed reviewers:
+ bauerb@chromium.org

[Stephen Chennney, 2015-04-02 19:13:27]

This new patch fixes the issue with both types of desktop plugins, at least for
me. You need the Blink side patch to try it out.

bauerb seems to be OOO, but he's the only owner. Do we land TBR?

[tommycli, 2015-04-02 20:08:08]

On 2015/04/02 19:13:27, Stephen Chenney wrote:
> This new patch fixes the issue with both types of desktop plugins, at least
for
> me. You need the Blink side patch to try it out.
> 
> bauerb seems to be OOO, but he's the only owner. Do we land TBR?

To confirm: This fixes both the BLOCK and DETECT case?

[tommycli, 2015-04-02 20:19:49]

On 2015/04/02 20:08:08, tommycli wrote:
> On 2015/04/02 19:13:27, Stephen Chenney wrote:
> > This new patch fixes the issue with both types of desktop plugins, at least
> for
> > me. You need the Blink side patch to try it out.
> > 
> > bauerb seems to be OOO, but he's the only owner. Do we land TBR?
> 
> To confirm: This fixes both the BLOCK and DETECT case?

I'm skeptical this fixes it, because:

1. LoadPlugin is only called after the user clicks on the plugin placeholder,
whereas the problem manifests itself when the placeholder is loaded. You might
be looking for LoadCallback() instead of LoadPlugin().

2. This fix fixes LoadablePluginPlaceholder, but probably leaves the base class
(PluginPlaceholder) out to dry. PluginPlaceholder is used for
MobileYoutubePlugin, which replaces Youtube embeds on Android. Maybe the
LoadCallback() mechanism needs to be moved to the base class?

[tommycli, 2015-04-02 20:21:03]

On 2015/04/02 20:19:49, tommycli wrote:
> On 2015/04/02 20:08:08, tommycli wrote:
> > On 2015/04/02 19:13:27, Stephen Chenney wrote:
> > > This new patch fixes the issue with both types of desktop plugins, at
least
> > for
> > > me. You need the Blink side patch to try it out.
> > > 
> > > bauerb seems to be OOO, but he's the only owner. Do we land TBR?
> > 
> > To confirm: This fixes both the BLOCK and DETECT case?
> 
> I'm skeptical this fixes it, because:
> 
> 1. LoadPlugin is only called after the user clicks on the plugin placeholder,
> whereas the problem manifests itself when the placeholder is loaded. You might
> be looking for LoadCallback() instead of LoadPlugin().
> 
> 2. This fix fixes LoadablePluginPlaceholder, but probably leaves the base
class
> (PluginPlaceholder) out to dry. PluginPlaceholder is used for
> MobileYoutubePlugin, which replaces Youtube embeds on Android. Maybe the
> LoadCallback() mechanism needs to be moved to the base class?

Sorry, correction: - You might be looking for DidFinishLoadingCallback instead
of LoadCallback/LoadPlugin.

[Stephen Chennney, 2015-04-02 20:49:08]

On 2015/04/02 20:21:03, tommycli wrote:
> On 2015/04/02 20:19:49, tommycli wrote:
> > On 2015/04/02 20:08:08, tommycli wrote:
> > > On 2015/04/02 19:13:27, Stephen Chenney wrote:
> > > > This new patch fixes the issue with both types of desktop plugins, at
> least
> > > for
> > > > me. You need the Blink side patch to try it out.
> > > > 
> > > > bauerb seems to be OOO, but he's the only owner. Do we land TBR?
> > > 
> > > To confirm: This fixes both the BLOCK and DETECT case?
> > 
> > I'm skeptical this fixes it, because:
> > 
> > 1. LoadPlugin is only called after the user clicks on the plugin
placeholder,
> > whereas the problem manifests itself when the placeholder is loaded. You
might
> > be looking for LoadCallback() instead of LoadPlugin().
> > 
> > 2. This fix fixes LoadablePluginPlaceholder, but probably leaves the base
> class
> > (PluginPlaceholder) out to dry. PluginPlaceholder is used for
> > MobileYoutubePlugin, which replaces Youtube embeds on Android. Maybe the
> > LoadCallback() mechanism needs to be moved to the base class?
> 
> Sorry, correction: - You might be looking for DidFinishLoadingCallback instead
> of LoadCallback/LoadPlugin.

The change is in ReplacePlugin, which does the work of actually attaching the
new plugin content to the Web layer, making Blink aware of it. I also tried
adding to DidFinishLoadingCallback and that worked too, but it seemed more
hackish and prone to future failure.

I looked at MobileYoutubePlugin and there is no corresponding code for plugin
replacement or the like. It seems the only path for content to come in is
render_frame()->LoadURLExternally, which will set up layout and invalidation on
the newly loaded content, as far as I know.

Which UI settings correspond to BLOCK and DETECT so I can verify them both?

[Stephen Chennney, 2015-04-02 20:56:49]

On 2015/04/02 20:49:08, Stephen Chenney wrote:
> On 2015/04/02 20:21:03, tommycli wrote:
> > On 2015/04/02 20:19:49, tommycli wrote:
> > > On 2015/04/02 20:08:08, tommycli wrote:
> > > > On 2015/04/02 19:13:27, Stephen Chenney wrote:
> > > > > This new patch fixes the issue with both types of desktop plugins, at
> > least
> > > > for
> > > > > me. You need the Blink side patch to try it out.
> > > > > 
> > > > > bauerb seems to be OOO, but he's the only owner. Do we land TBR?
> > > > 
> > > > To confirm: This fixes both the BLOCK and DETECT case?
> > > 
> > > I'm skeptical this fixes it, because:
> > > 
> > > 1. LoadPlugin is only called after the user clicks on the plugin
> placeholder,
> > > whereas the problem manifests itself when the placeholder is loaded. You
> might
> > > be looking for LoadCallback() instead of LoadPlugin().
> > > 
> > > 2. This fix fixes LoadablePluginPlaceholder, but probably leaves the base
> > class
> > > (PluginPlaceholder) out to dry. PluginPlaceholder is used for
> > > MobileYoutubePlugin, which replaces Youtube embeds on Android. Maybe the
> > > LoadCallback() mechanism needs to be moved to the base class?
> > 
> > Sorry, correction: - You might be looking for DidFinishLoadingCallback
instead
> > of LoadCallback/LoadPlugin.
> 
> The change is in ReplacePlugin, which does the work of actually attaching the
> new plugin content to the Web layer, making Blink aware of it. I also tried
> adding to DidFinishLoadingCallback and that worked too, but it seemed more
> hackish and prone to future failure.
> 
> I looked at MobileYoutubePlugin and there is no corresponding code for plugin
> replacement or the like. It seems the only path for content to come in is
> render_frame()->LoadURLExternally, which will set up layout and invalidation
on
> the newly loaded content, as far as I know.
> 
> Which UI settings correspond to BLOCK and DETECT so I can verify them both?

Opps, it does still manifest a little. Trying the alternative.

[tommycli, 2015-04-02 20:59:13]

On 2015/04/02 20:56:49, Stephen Chenney wrote:
> On 2015/04/02 20:49:08, Stephen Chenney wrote:
> > On 2015/04/02 20:21:03, tommycli wrote:
> > > On 2015/04/02 20:19:49, tommycli wrote:
> > > > On 2015/04/02 20:08:08, tommycli wrote:
> > > > > On 2015/04/02 19:13:27, Stephen Chenney wrote:
> > > > > > This new patch fixes the issue with both types of desktop plugins,
at
> > > least
> > > > > for
> > > > > > me. You need the Blink side patch to try it out.
> > > > > > 
> > > > > > bauerb seems to be OOO, but he's the only owner. Do we land TBR?
> > > > > 
> > > > > To confirm: This fixes both the BLOCK and DETECT case?
> > > > 
> > > > I'm skeptical this fixes it, because:
> > > > 
> > > > 1. LoadPlugin is only called after the user clicks on the plugin
> > placeholder,
> > > > whereas the problem manifests itself when the placeholder is loaded. You
> > might
> > > > be looking for LoadCallback() instead of LoadPlugin().
> > > > 
> > > > 2. This fix fixes LoadablePluginPlaceholder, but probably leaves the
base
> > > class
> > > > (PluginPlaceholder) out to dry. PluginPlaceholder is used for
> > > > MobileYoutubePlugin, which replaces Youtube embeds on Android. Maybe the
> > > > LoadCallback() mechanism needs to be moved to the base class?
> > > 
> > > Sorry, correction: - You might be looking for DidFinishLoadingCallback
> instead
> > > of LoadCallback/LoadPlugin.
> > 
> > The change is in ReplacePlugin, which does the work of actually attaching
the
> > new plugin content to the Web layer, making Blink aware of it. I also tried
> > adding to DidFinishLoadingCallback and that worked too, but it seemed more
> > hackish and prone to future failure.
> > 
> > I looked at MobileYoutubePlugin and there is no corresponding code for
plugin
> > replacement or the like. It seems the only path for content to come in is
> > render_frame()->LoadURLExternally, which will set up layout and invalidation
> on
> > the newly loaded content, as far as I know.
> > 
> > Which UI settings correspond to BLOCK and DETECT so I can verify them both?
> 
> Opps, it does still manifest a little. Trying the alternative.

Yes. The placeholder "replaces" the plugin, but it's different from the
ReplacePlugin() call.

The ReplacePlugin code is called when the user clicks the placeholder and we
replace the placeholder with the real plugin.

Thus, ReplacePlugin() isn't called when the plugin placeholder is loaded, but
rather when the user clicks on the placeholder to load the real plugin. In most
cases, the user will never click on the placeholder, and ReplacePlugin() is
never called.

It's confusing, sorry.

[tommycli, 2015-04-02 21:00:09]

On 2015/04/02 20:59:13, tommycli wrote:
> On 2015/04/02 20:56:49, Stephen Chenney wrote:
> > On 2015/04/02 20:49:08, Stephen Chenney wrote:
> > > On 2015/04/02 20:21:03, tommycli wrote:
> > > > On 2015/04/02 20:19:49, tommycli wrote:
> > > > > On 2015/04/02 20:08:08, tommycli wrote:
> > > > > > On 2015/04/02 19:13:27, Stephen Chenney wrote:
> > > > > > > This new patch fixes the issue with both types of desktop plugins,
> at
> > > > least
> > > > > > for
> > > > > > > me. You need the Blink side patch to try it out.
> > > > > > > 
> > > > > > > bauerb seems to be OOO, but he's the only owner. Do we land TBR?
> > > > > > 
> > > > > > To confirm: This fixes both the BLOCK and DETECT case?
> > > > > 
> > > > > I'm skeptical this fixes it, because:
> > > > > 
> > > > > 1. LoadPlugin is only called after the user clicks on the plugin
> > > placeholder,
> > > > > whereas the problem manifests itself when the placeholder is loaded.
You
> > > might
> > > > > be looking for LoadCallback() instead of LoadPlugin().
> > > > > 
> > > > > 2. This fix fixes LoadablePluginPlaceholder, but probably leaves the
> base
> > > > class
> > > > > (PluginPlaceholder) out to dry. PluginPlaceholder is used for
> > > > > MobileYoutubePlugin, which replaces Youtube embeds on Android. Maybe
the
> > > > > LoadCallback() mechanism needs to be moved to the base class?
> > > > 
> > > > Sorry, correction: - You might be looking for DidFinishLoadingCallback
> > instead
> > > > of LoadCallback/LoadPlugin.
> > > 
> > > The change is in ReplacePlugin, which does the work of actually attaching
> the
> > > new plugin content to the Web layer, making Blink aware of it. I also
tried
> > > adding to DidFinishLoadingCallback and that worked too, but it seemed more
> > > hackish and prone to future failure.
> > > 
> > > I looked at MobileYoutubePlugin and there is no corresponding code for
> plugin
> > > replacement or the like. It seems the only path for content to come in is
> > > render_frame()->LoadURLExternally, which will set up layout and
invalidation
> > on
> > > the newly loaded content, as far as I know.
> > > 
> > > Which UI settings correspond to BLOCK and DETECT so I can verify them
both?
> > 
> > Opps, it does still manifest a little. Trying the alternative.
> 
> Yes. The placeholder "replaces" the plugin, but it's different from the
> ReplacePlugin() call.
> 
> The ReplacePlugin code is called when the user clicks the placeholder and we
> replace the placeholder with the real plugin.
> 
> Thus, ReplacePlugin() isn't called when the plugin placeholder is loaded, but
> rather when the user clicks on the placeholder to load the real plugin. In
most
> cases, the user will never click on the placeholder, and ReplacePlugin() is
> never called.
> 
> It's confusing, sorry.

The MobileYoutubePlugin never 'replaces' the placeholder with a real plugin, but
if there is a bug with the placeholder loading, it will probably affect
MobileYoutubePlugin also.

[Stephen Chennney, 2015-04-02 21:04:06]

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

[commit-bot: I haz the power, 2015-04-02 21:07:02]

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

[Stephen Chennney, 2015-04-02 21:17:52]

Moved it and manual verification indicates this does reliably show the correct
thing, while the previous patch did not. I'm finally understanding how this
works. The DidFinishLoadingCallback is bound to the js didFinishLoading method
so will be invoked when things are indeed ready to display.

I see your point about the MobileYoutubePlugin but I have to head home. I can
add the gin bindings for didFinishLoading to the youtube code and perform the
same actions. Same patch or a different one?

[Stephen Chennney, 2015-04-02 21:18:32]

On 2015/04/02 21:17:52, Stephen Chenney wrote:
> Moved it and manual verification indicates this does reliably show the correct
> thing, while the previous patch did not. I'm finally understanding how this
> works. The DidFinishLoadingCallback is bound to the js didFinishLoading method
> so will be invoked when things are indeed ready to display.
> 
> I see your point about the MobileYoutubePlugin but I have to head home. I can
> add the gin bindings for didFinishLoading to the youtube code and perform the
> same actions. Same patch or a different one?

Failures on current patch are due to the not-yet-landed blink patch.

[commit-bot: I haz the power, 2015-04-02 21:21:25]

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

[commit-bot: I haz the power, 2015-04-02 21:21:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tommycli, 2015-04-02 21:22:02]

On 2015/04/02 21:17:52, Stephen Chenney wrote:
> Moved it and manual verification indicates this does reliably show the correct
> thing, while the previous patch did not. I'm finally understanding how this
> works. The DidFinishLoadingCallback is bound to the js didFinishLoading method
> so will be invoked when things are indeed ready to display.
> 
> I see your point about the MobileYoutubePlugin but I have to head home. I can
> add the gin bindings for didFinishLoading to the youtube code and perform the
> same actions. Same patch or a different one?

schenney: Same patch makes sense. Best thing to do may be to move the whole
DidFinishLoadingCallback mechanism to the base class (PluginPlaceholder).

Sorry this has turned into such a boondoggle, but otherwise there may be some
very unhappy users on mobile.

[tommycli, 2015-04-02 21:24:03]

On 2015/04/02 21:22:02, tommycli wrote:
> On 2015/04/02 21:17:52, Stephen Chenney wrote:
> > Moved it and manual verification indicates this does reliably show the
correct
> > thing, while the previous patch did not. I'm finally understanding how this
> > works. The DidFinishLoadingCallback is bound to the js didFinishLoading
method
> > so will be invoked when things are indeed ready to display.
> > 
> > I see your point about the MobileYoutubePlugin but I have to head home. I
can
> > add the gin bindings for didFinishLoading to the youtube code and perform
the
> > same actions. Same patch or a different one?
> 
> schenney: Same patch makes sense. Best thing to do may be to move the whole
> DidFinishLoadingCallback mechanism to the base class (PluginPlaceholder).
> 
> Sorry this has turned into such a boondoggle, but otherwise there may be some
> very unhappy users on mobile.

This whole thing enables substantial paint performance enhancements to M43?

[Stephen Chennney, 2015-04-03 15:25:45]

On 2015/04/02 21:24:03, tommycli wrote:
> On 2015/04/02 21:22:02, tommycli wrote:
> > On 2015/04/02 21:17:52, Stephen Chenney wrote:
> > > Moved it and manual verification indicates this does reliably show the
> correct
> > > thing, while the previous patch did not. I'm finally understanding how
this
> > > works. The DidFinishLoadingCallback is bound to the js didFinishLoading
> method
> > > so will be invoked when things are indeed ready to display.
> > > 
> > > I see your point about the MobileYoutubePlugin but I have to head home. I
> can
> > > add the gin bindings for didFinishLoading to the youtube code and perform
> the
> > > same actions. Same patch or a different one?
> > 
> > schenney: Same patch makes sense. Best thing to do may be to move the whole
> > DidFinishLoadingCallback mechanism to the base class (PluginPlaceholder).
> > 
> > Sorry this has turned into such a boondoggle, but otherwise there may be
some
> > very unhappy users on mobile.
> 
> This whole thing enables substantial paint performance enhancements to M43?

The patch was to allow us to run the telemetry performance tests for Slimming
Paint, so we could be sure that we would improve painting performance. The
actual Slimming Paint changes are behind a flag so not launching in m43. Rather,
we need the perf tests to be running in order to move toward launching.

My current thinking is that we revert the change until the branch has happened,
then re-land and fix the issues properly.

[chromium-reviews, 2015-04-03 16:02:21]

Yes that sounds good to me.
On Apr 3, 2015 8:25 AM, <schenney@chromium.org> wrote:

> On 2015/04/02 21:24:03, tommycli wrote:
>
>> On 2015/04/02 21:22:02, tommycli wrote:
>> > On 2015/04/02 21:17:52, Stephen Chenney wrote:
>> > > Moved it and manual verification indicates this does reliably show the
>> correct
>> > > thing, while the previous patch did not. I'm finally understanding how
>>
> this
>
>> > > works. The DidFinishLoadingCallback is bound to the js
>> didFinishLoading
>> method
>> > > so will be invoked when things are indeed ready to display.
>> > >
>> > > I see your point about the MobileYoutubePlugin but I have to head
>> home. I
>> can
>> > > add the gin bindings for didFinishLoading to the youtube code and
>> perform
>> the
>> > > same actions. Same patch or a different one?
>> >
>> > schenney: Same patch makes sense. Best thing to do may be to move the
>> whole
>> > DidFinishLoadingCallback mechanism to the base class
>> (PluginPlaceholder).
>> >
>> > Sorry this has turned into such a boondoggle, but otherwise there may be
>>
> some
>
>> > very unhappy users on mobile.
>>
>
>  This whole thing enables substantial paint performance enhancements to
>> M43?
>>
>
> The patch was to allow us to run the telemetry performance tests for
> Slimming
> Paint, so we could be sure that we would improve painting performance. The
> actual Slimming Paint changes are behind a flag so not launching in m43.
> Rather,
> we need the perf tests to be running in order to move toward launching.
>
> My current thinking is that we revert the change until the branch has
> happened,
> then re-land and fix the issues properly.
>
>
> https://codereview.chromium.org/1056643002/
>

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

[Bernhard Bauer, 2015-04-07 11:22:48]

This CL is still relevant, right? If so, LGTM.

Also: I would support adding Tommy to components/plugins/OWNERS :)

https://codereview.chromium.org/1056643002/diff/40001/components/plugins/rend...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/1056643002/diff/40001/components/plugins/rend...
components/plugins/renderer/loadable_plugin_placeholder.cc:334: blink::WebFrame*
frame = GetFrame();
Nit: This variable saves only five characters, we can just inline the call :)

[Stephen Chennney, 2015-04-07 15:06:52]

On 2015/04/07 11:22:48, Bernhard Bauer wrote:
> This CL is still relevant, right? If so, LGTM.
> 
> Also: I would support adding Tommy to components/plugins/OWNERS :)
> 
>
https://codereview.chromium.org/1056643002/diff/40001/components/plugins/rend...
> File components/plugins/renderer/loadable_plugin_placeholder.cc (right):
> 
>
https://codereview.chromium.org/1056643002/diff/40001/components/plugins/rend...
> components/plugins/renderer/loadable_plugin_placeholder.cc:334:
blink::WebFrame*
> frame = GetFrame();
> Nit: This variable saves only five characters, we can just inline the call :)

I'm not sure how best to proceed. Most likely, first I'll break things again
then I'll land this to get at least the initial display working. Then I'll file
a separate bug for the missing hover text display.

[Stephen Chennney, 2015-04-07 21:00:34]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2015-04-07 21:00:34]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1056643002/#ps60001
(title: "Rebased, just to be sure.")

[commit-bot: I haz the power, 2015-04-07 21:00:50]

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

[commit-bot: I haz the power, 2015-04-07 21:51:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-07 21:52:05]

Patchset 4 (id:??) landed as
https://crrev.com/c3bbc172f04db541b9da820a10bf4583e2c24e28
Cr-Commit-Position: refs/heads/master@{#324127}