Fix media player painting bug.

At times the Windows media player plugin would not render video. This would occur if it initially
received a geometry update of size 0. The plugin has a bug where it ignores subsequent geometry updates.

Based on the webkit plugin implementation they have a quirk which handles this case for media player and divx.
To ensure that this quirk works correctly in cases where we only receive one geometry update we send out
geometry updates during paint as well if needed.

Fix is to mimic this behavior for Chromium.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=43916

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

[stuartmorgan, 2010-06-14 21:15:01]

http://codereview.chromium.org/2752009/diff/11001/12001
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/2752009/diff/11001/12001#newcode145
webkit/glue/plugins/webplugin_delegate_impl.cc:145: return;
Do you actually want to bypass the whole plugin host, as opposed to just the
NPP_SetWindow call? Seems like this could have ripple effects in other Chromium
code.

Also, the description is specifically about 0x0; shouldn't this target that,
rather than any first call, to prevent accidentally blocking a non-zero first
call?

http://codereview.chromium.org/2752009/diff/11001/12002
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/2752009/diff/11001/12002#newcode76
webkit/glue/plugins/webplugin_delegate_impl.h:76:
PLUGIN_QUIRK_DEFER_FIRST_SETWINDOW_CALL = 65536,  // Windows for now.
The code isn't deferring it, it's ignoring it, so this should be s/DEFER/IGNORE/

[ananta, 2010-06-14 21:58:21]

As per our discussion, changed the quirk to
PLUGIN_QUIRK_IGNORE_FIRST_SETWINDOW_CALL and added a comment indicating that
turning this on for other platforms could cause issues.

-Ananta

http://codereview.chromium.org/2752009/diff/11001/12002
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/2752009/diff/11001/12002#newcode76
webkit/glue/plugins/webplugin_delegate_impl.h:76:
PLUGIN_QUIRK_DEFER_FIRST_SETWINDOW_CALL = 65536,  // Windows for now.
On 2010/06/14 21:15:01, stuartmorgan wrote:
> The code isn't deferring it, it's ignoring it, so this should be
s/DEFER/IGNORE/

Done.

[jam, 2010-06-14 22:09:39]

lgtm

http://codereview.chromium.org/2752009/diff/18001/19002
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/2752009/diff/18001/19002#newcode78
webkit/glue/plugins/webplugin_delegate_impl.h:78: // cause issues.
I'm confused as to why we should add this comment for this flag, but leave all
the other ones with just // Windows or // Linux.  I think they should all be
consistent, i.e. just add // Windows after this.

[ananta, 2010-06-14 22:21:50]

http://codereview.chromium.org/2752009/diff/18001/19002
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/2752009/diff/18001/19002#newcode78
webkit/glue/plugins/webplugin_delegate_impl.h:78: // cause issues.
On 2010/06/14 22:09:39, John Abd-El-Malek wrote:
> I'm confused as to why we should add this comment for this flag, but leave all
> the other ones with just // Windows or // Linux.  I think they should all be
> consistent, i.e. just add // Windows after this.

ok. Restored to as in the previous patch set.

[stuartmorgan, 2010-06-14 22:33:39]

On 2010/06/14 22:09:39, John Abd-El-Malek wrote:
> I'm confused as to why we should add this comment for this flag, but leave all
> the other ones with just // Windows or // Linux.  I think they should all be
> consistent, i.e. just add // Windows after this.

The concern is that the implementation of this flag is entirely in
cross-platform code, which suggests that it would be a simple matter of enabling
the flag on another platform if it were necessary. In reality, the
implementation there would not work for the Mac, because it's not a
cross-platform implementation even though it's in cross-platform code, and that
makes me very nervous since in the past we have re-used existing flags,
including implementations sometimes, on the other platforms.

Maybe we should have a comment at the implementation site, rather than the flag
declaration.

[jam, 2010-06-14 22:41:48]

On Mon, Jun 14, 2010 at 3:33 PM, <stuartmorgan@chromium.org> wrote:

> On 2010/06/14 22:09:39, John Abd-El-Malek wrote:
>
>> I'm confused as to why we should add this comment for this flag, but leave
>> all
>> the other ones with just // Windows or // Linux.  I think they should all
>> be
>> consistent, i.e. just add // Windows after this.
>>
>
> The concern is that the implementation of this flag is entirely in
> cross-platform code, which suggests that it would be a simple matter of
> enabling
> the flag on another platform if it were necessary.


Can't this be said for many of the other flags?  I'd just like to be
consistent with how we comment them.


> In reality, the
> implementation there would not work for the Mac, because it's not a
> cross-platform implementation even though it's in cross-platform code, and
> that
> makes me very nervous since in the past we have re-used existing flags,
> including implementations sometimes, on the other platforms.
>
> Maybe we should have a comment at the implementation site, rather than the
> flag
> declaration.
>
>
> http://codereview.chromium.org/2752009/show
>

[stuartmorgan, 2010-06-14 22:49:07]

On 2010/06/14 22:41:48, John Abd-El-Malek wrote:
> > The concern is that the implementation of this flag is entirely in
> > cross-platform code, which suggests that it would be a simple matter of
> > enabling
> > the flag on another platform if it were necessary.
> 
> Can't this be said for many of the other flags?  I'd just like to be
> consistent with how we comment them.

From what I've seen, that would be a correct belief for all the other flags with
implementations in the cross-platform code--i.e., they would do what they are
supposed to do on all platforms, if we needed it. Here though, if we ever needed
to skip the first SetWindow call on the Mac, enabling this flag would *not* work
correctly, and would in fact cause serious problems in some circumstances.

[ananta, 2010-06-14 23:30:18]

On 2010/06/14 22:49:07, stuartmorgan wrote:
> On 2010/06/14 22:41:48, John Abd-El-Malek wrote:
> > > The concern is that the implementation of this flag is entirely in
> > > cross-platform code, which suggests that it would be a simple matter of
> > > enabling
> > > the flag on another platform if it were necessary.
> > 
> > Can't this be said for many of the other flags?  I'd just like to be
> > consistent with how we comment them.
> 
> From what I've seen, that would be a correct belief for all the other flags
with
> implementations in the cross-platform code--i.e., they would do what they are
> supposed to do on all platforms, if we needed it. Here though, if we ever
needed
> to skip the first SetWindow call on the Mac, enabling this flag would *not*
work
> correctly, and would in fact cause serious problems in some circumstances.

I added a comment explaining the quirk in the
WebPluginDelegateImpl::UpdateGeometry function.

-Ananta

[stuartmorgan, 2010-06-15 00:13:44]

http://codereview.chromium.org/2752009/diff/32001/33005
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/2752009/diff/32001/33005#newcode327
webkit/glue/plugins/webplugin_delegate_impl_win.cc:327: } else if
(instance_->mime_type() == "video/divx") {
Oops, I forgot to comment about this amid the other concerns earlier. The VLC
plugin handles this MIME type as well, so detecting by MIME will over-trigger
this quirk; you'll need to use the name instead.

[ananta, 2010-06-15 00:34:43]

http://codereview.chromium.org/2752009/diff/32001/33005
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/2752009/diff/32001/33005#newcode327
webkit/glue/plugins/webplugin_delegate_impl_win.cc:327: } else if
(instance_->mime_type() == "video/divx") {
On 2010/06/15 00:13:44, stuartmorgan wrote:
> Oops, I forgot to comment about this amid the other concerns earlier. The VLC
> plugin handles this MIME type as well, so detecting by MIME will over-trigger
> this quirk; you'll need to use the name instead.

It appears that the divx plugin no longer has this bug. I tested this with the
latest plugin and it works without this   quirk. Removed the addition of the
quirk for divx to avoid needless complexity.

[stuartmorgan, 2010-06-15 15:59:19]

http://codereview.chromium.org/2752009/diff/53001/38007
File webkit/glue/plugins/webplugin_impl.cc (right):

http://codereview.chromium.org/2752009/diff/53001/38007#newcode302
webkit/glue/plugins/webplugin_impl.cc:302: // a geometry update during paint
which should go out correctly as the
A bunch of work was done a while back in webplugin_delegate_impl to prevent
NPP_SetWindow from being called during paint when it was possible to avoid it.
Isn't this regressing that? I don't want to regress all plugins just because of
a bug in one or two.

[stuartmorgan, 2010-06-15 16:00:40]

http://codereview.chromium.org/2752009/diff/53001/38006
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/2752009/diff/53001/38006#newcode328
webkit/glue/plugins/webplugin_delegate_impl_win.cc:328: std::wstring::npos) {
Also, your comment said DivX doesn't need this any more; is that incorrect?

[ananta, 2010-06-15 17:06:56]

http://codereview.chromium.org/2752009/diff/53001/38006
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/2752009/diff/53001/38006#newcode328
webkit/glue/plugins/webplugin_delegate_impl_win.cc:328: std::wstring::npos) {
On 2010/06/15 16:00:40, stuartmorgan wrote:
> Also, your comment said DivX doesn't need this any more; is that incorrect?

That comment was an oversight. The Divx plugin does have the same issue as media
player.

http://codereview.chromium.org/2752009/diff/53001/38007
File webkit/glue/plugins/webplugin_impl.cc (right):

http://codereview.chromium.org/2752009/diff/53001/38007#newcode302
webkit/glue/plugins/webplugin_impl.cc:302: // a geometry update during paint
which should go out correctly as the
On 2010/06/15 15:59:19, stuartmorgan wrote:
> A bunch of work was done a while back in webplugin_delegate_impl to prevent
> NPP_SetWindow from being called during paint when it was possible to avoid it.
> Isn't this regressing that? I don't want to regress all plugins just because
of
> a bug in one or two.

This would only cause one additional geometry update to be sent out in the first
paint. The geometry update cache would kick in after that which would prevent
additional setwindow calls from being sent out to the plugin. I think this is a
reasonable approach.

[stuartmorgan, 2010-06-15 17:17:59]

http://codereview.chromium.org/2752009/diff/53001/38007
File webkit/glue/plugins/webplugin_impl.cc (right):

http://codereview.chromium.org/2752009/diff/53001/38007#newcode302
webkit/glue/plugins/webplugin_impl.cc:302: // a geometry update during paint
which should go out correctly as the
On 2010/06/15 17:06:56, ananta wrote:
> This would only cause one additional geometry update to be sent out in the
first
> paint. [...] I think this is a reasonable approach. 

According to the original change:
http://codereview.chromium.org/159717
it was done primarily to fix crashes in Flash. Re-introducing a crash in Flash
doesn't seem like an acceptable trade-off for the issue this change is fixing.

[ananta, 2010-06-15 17:42:52]

On 2010/06/15 17:17:59, stuartmorgan wrote:
> http://codereview.chromium.org/2752009/diff/53001/38007
> File webkit/glue/plugins/webplugin_impl.cc (right):
> 
> http://codereview.chromium.org/2752009/diff/53001/38007#newcode302
> webkit/glue/plugins/webplugin_impl.cc:302: // a geometry update during paint
> which should go out correctly as the
> On 2010/06/15 17:06:56, ananta wrote:
> > This would only cause one additional geometry update to be sent out in the
> first
> > paint. [...] I think this is a reasonable approach. 
> 
> According to the original change:
> http://codereview.chromium.org/159717
> it was done primarily to fix crashes in Flash. Re-introducing a crash in Flash
> doesn't seem like an acceptable trade-off for the issue this change is fixing.

Well we also have checks in the plugin code where we don't invoke the underlying
setwindow if the geometry did not change. So all this change does is sent out an
additional async IPC which does not have much overhead.

[ananta, 2010-06-15 18:12:59]

On 2010/06/15 17:42:52, ananta wrote:
> On 2010/06/15 17:17:59, stuartmorgan wrote:
> > http://codereview.chromium.org/2752009/diff/53001/38007
> > File webkit/glue/plugins/webplugin_impl.cc (right):
> > 
> > http://codereview.chromium.org/2752009/diff/53001/38007#newcode302
> > webkit/glue/plugins/webplugin_impl.cc:302: // a geometry update during paint
> > which should go out correctly as the
> > On 2010/06/15 17:06:56, ananta wrote:
> > > This would only cause one additional geometry update to be sent out in the
> > first
> > > paint. [...] I think this is a reasonable approach. 
> > 
> > According to the original change:
> > http://codereview.chromium.org/159717
> > it was done primarily to fix crashes in Flash. Re-introducing a crash in
Flash
> > doesn't seem like an acceptable trade-off for the issue this change is
fixing.
> 
> Well we also have checks in the plugin code where we don't invoke the
underlying
> setwindow if the geometry did not change. So all this change does is sent out
an
> additional async IPC which does not have much overhead.

I also tested the cases mentioned in the bug
http://code.google.com/p/chromium/issues/detail?id=16114 and the plugin_test
which Dimitri added specifically for this. They all continue to work fine.

-Ananta

[stuartmorgan, 2010-06-15 18:14:12]

On 2010/06/15 17:42:52, ananta wrote:
> Well we also have checks in the plugin code where we don't invoke the
underlying
> setwindow if the geometry did not change. So all this change does is sent out
an
> additional async IPC which does not have much overhead.

True, I guess that would prevent the crashes. However, on Windows the update
geometry calls are sync for some plugins, and on the Mac there's a bit more work
that happens on an update geometry call in some cases.

It worries me how many files I had to look in to figure out what all the side
effects of this change would be and how it would affect different plugins on
different platforms. That's a of places for someone to accidentally regress
something because they don't know about this hack in a completely different
file. Given how delicate plugin code can be, I really don't like the idea of
making the call sequence less intuitive for every plugin, on every platform,
just to work around a bug in a couple of plugins. Can we find a way to make the
change much more targeted to the cases where we need it?

How about instead of making the change here, have webplugin_delegate_impl_win.cc
keep track of whether SetWindow has ever been called, and if Paint is called,
SetWindow hasn't been called, and the quirk is active, call UpdateGeometry from
there?

[jam, 2010-06-15 18:19:54]

lgtm

[ananta, 2010-06-15 18:20:10]

On 2010/06/15 18:14:12, stuartmorgan wrote:
> On 2010/06/15 17:42:52, ananta wrote:
> > Well we also have checks in the plugin code where we don't invoke the
> underlying
> > setwindow if the geometry did not change. So all this change does is sent
out
> an
> > additional async IPC which does not have much overhead.
> 
> True, I guess that would prevent the crashes. However, on Windows the update
> geometry calls are sync for some plugins, and on the Mac there's a bit more
work
> that happens on an update geometry call in some cases.
> 
> It worries me how many files I had to look in to figure out what all the side
> effects of this change would be and how it would affect different plugins on
> different platforms. That's a of places for someone to accidentally regress
> something because they don't know about this hack in a completely different
> file. Given how delicate plugin code can be, I really don't like the idea of
> making the call sequence less intuitive for every plugin, on every platform,
> just to work around a bug in a couple of plugins. Can we find a way to make
the
> change much more targeted to the cases where we need it?
> 
> How about instead of making the change here, have
webplugin_delegate_impl_win.cc
> keep track of whether SetWindow has ever been called, and if Paint is called,
> SetWindow hasn't been called, and the quirk is active, call UpdateGeometry
from
> there?

The Paint function in webplugin_delegate_impl_win.cc does not get invoked for
windowed plugins. We don't support windowless media player plugins in Chrome
anyway. I tend to agree with the concern about regressing other platforms and
plugins with this change. However I still think we have reasonable checks in the
geometry update code paths in the renderer and the plugin to handle an
additional geometry update.

[stuartmorgan, 2010-06-15 18:34:14]

On 2010/06/15 18:20:10, ananta wrote:
> We don't support windowless media player plugins in Chrome anyway.

On Windows. On the Mac, all plugins are windowless, so this change will cause
some change, benign or not, in the call sequence for every single plugin. That
may be why I am a lot more concerned by this :)

> I tend to agree with the concern about regressing other platforms and
> plugins with this change.

Can you comment on why we can't use my suggestion then? When dealing with a hack
to work around a bug in specific plugins, if we have a reasonable option that
limits the scope of the hack--both in terms of classes affected, and changes
when running other plugins--I think we should prefer that to making the general
plugin infrastructure more complex, even if we can prove to ourselves that the
general change isn't actually going to regress anything.

[ananta, 2010-06-15 19:02:32]

On 2010/06/15 18:34:14, stuartmorgan wrote:
> On 2010/06/15 18:20:10, ananta wrote:
> > We don't support windowless media player plugins in Chrome anyway.
> 
> On Windows. On the Mac, all plugins are windowless, so this change will cause
> some change, benign or not, in the call sequence for every single plugin. That
> may be why I am a lot more concerned by this :)
> 
> > I tend to agree with the concern about regressing other platforms and
> > plugins with this change.
> 
> Can you comment on why we can't use my suggestion then? When dealing with a
hack
> to work around a bug in specific plugins, if we have a reasonable option that
> limits the scope of the hack--both in terms of classes affected, and changes
> when running other plugins--I think we should prefer that to making the
general
> plugin infrastructure more complex, even if we can prove to ourselves that the
> general change isn't actually going to regress anything.

As per our discussion ifdefed out the changed code in webplugin_impl to be
windows specific.

[stuartmorgan, 2010-06-15 19:03:09]

LGTM