Fix Silverlight windowless plugin painting issues. This fixes the following...

Fix Silverlight windowless plugin painting issues. This fixes the
following
issues:-
1. http://code.google.com/p/chromium/issues/detail?id=4272
2. http://code.google.com/p/chromium/issues/detail?id=301 (Partially)

The fixes are as below:-
1. Silverlight in the NPP_HandleEvent call for WM_PAINT,
  calls NPN_GetProperty for a bunch of properties like
  pageXOffset, pageYOffset, etc. It expects these
  properties to have integer types on return. We always
  return double. Firefox returns integer for these
  properties. Added a check in the conversion
  to NPVariant function in v8 to check for whether the
  value is an integer and return the intger type.

2. When the windowless plugin calls NPN_InvalidateRect we
  ask the plugin to paint in the same call, which the
  Silverlight plugin does not like. It relies
  on the fact that browsers would initiate invalidation
  asynchronously and eventually paint. This is a perfectly
  legal assumption. The Flash plugin does work if we
  synchronously ask it to paint. However other plugins
  could have similar issues. I verified with
  worldofwarcraft.com to see if the async paint
  has any sideeffects and there were none.

3.If the Silverlight plugin is not visible initially as on
  msdn.microsoft.com, it does not paint. This occurs
  because the plugin expects proper paints to
  come from the browser. It does not invalidate itself in
  UpdateGeometry as Flash. The plugin widget on msdn is not
  dynamic. It does call NPN_InvalidateRect
  initially when it is not visible. We don't send the call
  to the renderer as the plugin is not visible. To workaround
  this we now track the damaged rect even if the rect does not
  intersect the clipping rect of the plugin. In a geometry update
  if the plugin is visible, we send over the accumulated damaged rect
  to the renderer.

The Silverlight plugin instance on msdn.microsoft.com does not work correctly even with these fixes. However it paints correctly.

R=jam
Bug=4272, 301
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=5816

[jam, 2008-11-19 22:36:47]

http://codereview.chromium.org/11492/diff/1/6
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/11492/diff/1/6#newcode101
Line 101: return;
I'm wondering if instead of adding this extra state, we can accomplish the same
thing by adding the new rect to damaged_rect_.  That way when the renderer
paints next time, we'll get a DidPaint notification and then invalidate the
accumulated damaged_rect_.  what do you think?

http://codereview.chromium.org/11492/diff/1/6#newcode116
Line 116: // have similar issues.
Until we see this issue in other plugins, I don't think we should delay the
painting for all of them.

http://codereview.chromium.org/11492/diff/1/5
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/11492/diff/1/5#newcode282
Line 282: }
why is this code in WebpluginDelegateImpl?  this behavior only happens when
running out of process, so can be it be put in WebPluginProxy::UpdateGeometry? 
That way you also don't need to change web_plugin.h

http://codereview.chromium.org/11492/diff/1/5#newcode873
Line 873: damage_rect_win.right  = damage_rect_win.left + damage_rect.width();
why do we suddenly need to change this?

http://codereview.chromium.org/11492/diff/1/2
File webkit/port/bindings/v8/v8_np_utils.cpp (right):

http://codereview.chromium.org/11492/diff/1/2#newcode57
Line 57: INT32_TO_NPVARIANT(object->NumberValue(), *result);
are we sure this won't break any other plugins?  What does Firefox/Safari do for
this case?

[ananta, 2008-11-19 23:52:12]

Replies below:-

On 2008/11/19 22:36:47, John Abd-El-Malek wrote:
> http://codereview.chromium.org/11492/diff/1/6
> File chrome/plugin/webplugin_proxy.cc (right):
> 
> http://codereview.chromium.org/11492/diff/1/6#newcode101
> Line 101: return;
> I'm wondering if instead of adding this extra state, we can accomplish the
same
> thing by adding the new rect to damaged_rect_.  That way when the renderer
> paints next time, we'll get a DidPaint notification and then invalidate the
> accumulated damaged_rect_.  what do you think?
> 
Done :- Based on our discussion, we now accumulate the damaged rect in the case
when it does not intersect the clip rect. We call InvalidateRect in the geometry
update sequence to send this to the renderer.

> http://codereview.chromium.org/11492/diff/1/6#newcode116
> Line 116: // have similar issues.
> Until we see this issue in other plugins, I don't think we should delay the
> painting for all of them.
Valid point. I think on a whole it is good to not have reentrancies in plugins.
We have had trouble with this in the past. If you feel strongly about this, then
we could have a quirk for Silverlight. I did not see any difference in painting
behavior on a 256 MB VM with Flash 9 on worldofwarcraft.com.

> 
> http://codereview.chromium.org/11492/diff/1/5
> File webkit/glue/plugins/webplugin_delegate_impl.cc (right):
> 
> http://codereview.chromium.org/11492/diff/1/5#newcode282
> Line 282: }
> why is this code in WebpluginDelegateImpl?  this behavior only happens when
> running out of process, so can be it be put in WebPluginProxy::UpdateGeometry?

> That way you also don't need to change web_plugin.h
> 
This code has been taken out.

> http://codereview.chromium.org/11492/diff/1/5#newcode873
> Line 873: damage_rect_win.right  = damage_rect_win.left + damage_rect.width();
> why do we suddenly need to change this?
> 
Found this while debugging Silverlight with Firefox. The painting code in
Firefox does this. I thought emulating this is good.

> http://codereview.chromium.org/11492/diff/1/2
> File webkit/port/bindings/v8/v8_np_utils.cpp (right):
> 
> http://codereview.chromium.org/11492/diff/1/2#newcode57
> Line 57: INT32_TO_NPVARIANT(object->NumberValue(), *result);
> are we sure this won't break any other plugins?  What does Firefox/Safari do
for
> this case?
Firefox returns INT in this case which is why Silverlight started working in
windowless mode in Firefox. Safari returns double and hence this does not work
in Safari.

CB updated.

Thanks
Ananta

[jam, 2008-11-20 02:44:10]

http://codereview.chromium.org/11492/diff/202/17
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/11492/diff/202/17#newcode97
Line 97: damaged_rect_ = damaged_rect_.Union(rect);
instead of writing this line twice, it can jsut be moved to before the if
statement.

http://codereview.chromium.org/11492/diff/202/17#newcode113
Line 113: MessageLoop::current()->PostTask(FROM_HERE,
I still don't think this is the right thing to do.  NPAPI doesn't say that we
can't call into plugins to paint when they call us to invalidate.  So always
doing a posttask just slows things down for all other plugins with no gain for
them.  If silverlight has issues, then we should just do it as a quirk.

Also I think the expanded comment above is not really necessary.  It should just
say something like silverlight doesn't paint if we call it during an invalidate.

http://codereview.chromium.org/11492/diff/202/16
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/11492/diff/202/16#newcode865
Line 865: damage_rect_win.top    = damage_rect.y() + window_rect_.y();
This change still confuses me.  If we really needed to offset the rectangle,
then painting shouldn't have worked before.  How was it displaying correctly?

[ananta, 2008-11-20 05:44:19]

On 2008/11/20 02:44:10, John Abd-El-Malek wrote:
> http://codereview.chromium.org/11492/diff/202/17
> File chrome/plugin/webplugin_proxy.cc (right):
> 
> http://codereview.chromium.org/11492/diff/202/17#newcode97
> Line 97: damaged_rect_ = damaged_rect_.Union(rect);
> instead of writing this line twice, it can jsut be moved to before the if
> statement.

Done.

> 
> http://codereview.chromium.org/11492/diff/202/17#newcode113
> Line 113: MessageLoop::current()->PostTask(FROM_HERE,
> I still don't think this is the right thing to do.  NPAPI doesn't say that we
> can't call into plugins to paint when they call us to invalidate.  So always
> doing a posttask just slows things down for all other plugins with no gain for
> them.  If silverlight has issues, then we should just do it as a quirk.
> 
> Also I think the expanded comment above is not really necessary.  It should
just
> say something like silverlight doesn't paint if we call it during an
invalidate.

Done. Added a quirk for Silverlight.
> 
> http://codereview.chromium.org/11492/diff/202/16
> File webkit/glue/plugins/webplugin_delegate_impl.cc (right):
> 
> http://codereview.chromium.org/11492/diff/202/16#newcode865
> Line 865: damage_rect_win.top    = damage_rect.y() + window_rect_.y();
> This change still confuses me.  If we really needed to offset the rectangle,
> then painting shouldn't have worked before.  How was it displaying correctly?

The reason it works is Flash ignores the damage rect. It looks like Flash just
paints into the clip rect which is passed in the last NPP_SetWindow call. I did
not debug Silverlight to see what it does. This is Firefox's code:-
  // we got dirty rectangle in relative window coordinates, but we
  // need it in absolute units and in the (left, top, right, bottom) form
  RECT drc;
  drc.left   = relDirtyRectInPixels.x + window->x;
  drc.top    = relDirtyRectInPixels.y + window->y;
  drc.right  = drc.left + relDirtyRectInPixels.width;
  drc.bottom = drc.top + relDirtyRectInPixels.height;

  nsPluginEvent pluginEvent;
  pluginEvent.event = WM_PAINT;
  pluginEvent.wParam = (uint32)ndc;
  pluginEvent.lParam = (uint32)&drc;
  PRBool eventHandled = PR_FALSE;
  mInstance->HandleEvent(&pluginEvent, &eventHandled);

Safari's implementation does not add the offsets. As I said the most prevalent
windowless plugins are Flash which ignores it. I think Firefox'es implementation
is probably the way to go.

The CB has been updated.

Thanks
Ananta

[jam, 2008-11-20 21:03:38]

http://codereview.chromium.org/11492/diff/404/407
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/11492/diff/404/407#newcode302
Line 302: // Send over any pending invalidates.
can you explain a little why we're doing this?  i.e. 

Send any pending invalidates that occurred while the plugin was offscreen.

Also, I think this should only be called when this situation occurs.  i.e.

do gfx::Rect old_clip_rect = delegate_->clip_rect() at the top.

and

if (delegate_->windowless() && visible && !clip_rect.IsEmpty() &&
old_clip_rect.IsEmpty() && !damaged_rect_.IsEmpty())

http://codereview.chromium.org/11492/diff/404/406
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/11492/diff/404/406#newcode869
Line 869: damage_rect_win.top    = damage_rect.y() + window_rect_.y();
ok, but can you file a bug on webkit to fix this (or even send a patch too), so
that we're consistent?

Also, please file a bug for the other script double/int problem that you find on
webkit.

[ananta, 2008-11-20 23:10:20]

On 2008/11/20 21:03:38, John Abd-El-Malek wrote:
> http://codereview.chromium.org/11492/diff/404/407
> File chrome/plugin/webplugin_proxy.cc (right):
> 
> http://codereview.chromium.org/11492/diff/404/407#newcode302
> Line 302: // Send over any pending invalidates.
> can you explain a little why we're doing this?  i.e. 
> 
> Send any pending invalidates that occurred while the plugin was offscreen.

Done.

> 
> Also, I think this should only be called when this situation occurs.  i.e.
> 
> do gfx::Rect old_clip_rect = delegate_->clip_rect() at the top.
> 
> and
> 
> if (delegate_->windowless() && visible && !clip_rect.IsEmpty() &&
> old_clip_rect.IsEmpty() && !damaged_rect_.IsEmpty())

Done.

> 
> http://codereview.chromium.org/11492/diff/404/406
> File webkit/glue/plugins/webplugin_delegate_impl.cc (right):
> 
> http://codereview.chromium.org/11492/diff/404/406#newcode869
> Line 869: damage_rect_win.top    = damage_rect.y() + window_rect_.y();
> ok, but can you file a bug on webkit to fix this (or even send a patch too),
so
> that we're consistent?

I don't have any data on whether Firefox is doing the right thing here.
Silverlight and Flash work fine. We know that Flash ignores it. Silverlight may
be doing the same. I have reverted this change.

> 
> Also, please file a bug for the other script double/int problem that you find
on
> webkit.

Will do

CB updated.

Thanks
Ananta

[jam, 2008-11-21 00:26:06]

lgtm

http://codereview.chromium.org/11492/diff/229/46
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/11492/diff/229/46#newcode306
Line 306: if (delegate_->windowless() && !clip_rect.IsEmpty() &&
shouldn't this also have && visible?

http://codereview.chromium.org/11492/diff/229/47
File chrome/plugin/webplugin_proxy.h (right):

http://codereview.chromium.org/11492/diff/229/47#newcode111
Line 111: // Handler for sending over the actual paint event to the plugin.
"actual" seems redundant

[ananta, 2008-11-21 02:37:57]

On 2008/11/21 00:26:06, John Abd-El-Malek wrote:
> lgtm
> 
> http://codereview.chromium.org/11492/diff/229/46
> File chrome/plugin/webplugin_proxy.cc (right):
> 
> http://codereview.chromium.org/11492/diff/229/46#newcode306
> Line 306: if (delegate_->windowless() && !clip_rect.IsEmpty() &&
> shouldn't this also have && visible?

Added the visible flag to the if check. This caused the silverlight plugin to
not paint. Was a regression with the change I made a while back to send the
visible flag as false until the plugin receives the first paint. This was only
applicable for Windowed plugins as they can move around in SetwindowPos calls
made to position the plugin. Fixed this in webplugin_impl.cc/.h

> 
> http://codereview.chromium.org/11492/diff/229/47
> File chrome/plugin/webplugin_proxy.h (right):
> 
> http://codereview.chromium.org/11492/diff/229/47#newcode111
> Line 111: // Handler for sending over the actual paint event to the plugin.
> "actual" seems redundant
Removed actual.

Thanks
Ananta

[jam, 2008-11-21 02:57:44]

lgtm

http://codereview.chromium.org/11492/diff/64/66
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/11492/diff/64/66#newcode304
Line 304: InvalidateRect(damaged_rect_);
nit: i think you need brace brackets here