Bug 109879

Summary: Plugin in iframe may not display
Product: WebKit Reporter: John Bauman <jbauman>
Component: Plug-insAssignee: John Bauman <jbauman>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, buildbot, darin, eric, esprehn+autocc, gyuyoung.kim, jamesr, ojan.autocc, rakuco, rniwa, simon.fraser, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 111514, 112324    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description John Bauman 2013-02-14 18:22:45 PST
Plugin in iframe may not display
Comment 1 John Bauman 2013-02-14 18:27:12 PST
Created attachment 188462 [details]
Patch
Comment 2 John Bauman 2013-02-14 18:35:53 PST
I'm not certain this is the best way to do this, though it seems to work.

Is there a way to do a layout test with a plugin with chromium?
Comment 3 James Robinson 2013-02-15 10:49:16 PST
(In reply to comment #2)
> I'm not certain this is the best way to do this, though it seems to work.
> 
> Is there a way to do a layout test with a plugin with chromium?

Yes - you can instantiate an NPAPI plugin with mime type "application/x-webkit-test-netscape" in a layout test.  See LayoutTests/plugins/ for some examples.  That will instantiate an instance of the plugin defined in Tools/DumpRenderTree/TestNetscapePlugIn/.
Comment 4 Simon Fraser (smfr) 2013-02-15 12:32:30 PST
Comment on attachment 188462 [details]
Patch

This scares me a little. We call widgetPositionsUpdated() after scrolling (main frame if we have fixed, and overflow scrolling). Seems like this could be a significant perf hit if you have frames with lots of plugins.
Comment 5 John Bauman 2013-02-15 12:51:46 PST
Yeah, I'm not sure how much this changes the performance. We already do a similar thing with frameRectsChanged on scroll, so at least the overall time complexity isn't different. And I don't know if we would expect putting a plugin inside an iframe would improve performance during layout over keeping it outside.

I could try to add a separate method to do this that's only called after layout finishes, if you want.
Comment 6 John Bauman 2013-02-15 16:05:10 PST
Created attachment 188660 [details]
Patch
Comment 7 Darin Adler 2013-02-15 16:16:38 PST
Comment on attachment 188660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188660&action=review

I’m not enough of an expert on our scrolling to know if this will cause performance problems on some platforms or not, so I can’t give this review+ but I do have a few comments.

> Source/WebCore/ChangeLog:11
> +        Changing the cliprect on a layer containing an iframe may change the
> +        cliprect of plugins inside the iframe, so recursively tell all plugins
> +        in iframes that their layout has changed after doing layout on the
> +        outer frame.

Seems obviously wrong that our comment here says “tell plug-ins that their layout has changed”, but the code says “tell plug-ins that the widget positions have changed”, which is not the same thing. This also could be the crux of the bug, that clipping changes should be included too, not just position changes. So I think a rename may be in order.

> Source/WebCore/ChangeLog:17
> +        (WebCore):

Please remove bogus lines like this from the change log. The script is there to help you write a good change log, but if it spits out nonsense like this, please delete it. And consider fixing the script or asking someone else to do that.

> Source/WebCore/ChangeLog:19
> +        (ScrollView):

Ditto.

> Source/WebCore/platform/ScrollView.h:243
> +    virtual void widgetPositionsUpdated();

Should specify OVERRIDE.
Comment 8 Darin Adler 2013-02-15 16:17:36 PST
Comment on attachment 188660 [details]
Patch

review- because apparently Simon Fraser had the same concern I did, and he’s more of a scrolling expert. We should not do this unless we’re sure it doesn’t cause scrolling problems.
Comment 9 Darin Adler 2013-02-15 16:17:59 PST
(In reply to comment #8)
> We should not do this unless we’re sure it doesn’t cause scrolling problems.

We should not do this *in this way* ...
Comment 10 John Bauman 2013-02-15 18:54:27 PST
Created attachment 188683 [details]
Patch
Comment 11 Simon Fraser (smfr) 2013-02-15 19:00:31 PST
It would be nice if you could describe an actual bug on a real web page that is fixed by this patch.
Comment 12 John Bauman 2013-02-15 19:03:25 PST
Sorry, the bug that was reported is that Flash ads in the Youtube sidebar are sometimes not visible - https://code.google.com/p/chromium/issues/detail?id=168900 (though that bug report isn't visible to the public for various reasons).
Comment 13 John Bauman 2013-02-15 20:07:06 PST
Created attachment 188688 [details]
Patch

Checking again, it seems like other ports would also have this issue, so fixing them as well
Comment 14 Early Warning System Bot 2013-02-15 20:20:58 PST
Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16586456
Comment 15 EFL EWS Bot 2013-02-15 20:34:44 PST
Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16592424
Comment 16 Build Bot 2013-02-15 22:34:43 PST
Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16603238
Comment 17 Build Bot 2013-02-17 03:05:25 PST
Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16585844
Comment 18 John Bauman 2013-02-20 08:17:32 PST
Created attachment 189323 [details]
Patch

Fix build
Comment 19 Build Bot 2013-02-20 10:45:21 PST
Comment on attachment 189323 [details]
Patch

Attachment 189323 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16647428

New failing tests:
plugins/plugin-clip-subframe.html
Comment 20 Build Bot 2013-02-21 21:29:51 PST
Comment on attachment 189323 [details]
Patch

Attachment 189323 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16698530

New failing tests:
plugins/plugin-clip-subframe.html
Comment 21 John Bauman 2013-02-25 22:46:43 PST
Created attachment 190209 [details]
Patch

Fix mac WK1 and add WK2 expectations.
Comment 22 Eric Seidel (no email) 2013-02-25 22:55:42 PST
Crazy.  Patch looks sane though.
Comment 23 Simon Fraser (smfr) 2013-02-26 09:08:37 PST
Comment on attachment 190209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190209&action=review

> Source/WebCore/plugins/PluginView.cpp:164
> +void PluginView::clipRectChanged()
> +{
> +    updatePluginWidget();
> +}

Why doesn't this call the base class function?

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:227
> +    Widget::clipRectChanged();

Is this necessary? It would be nice to have a virtual function on Widget that doesn't require remembering to call the base class.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1590
> +    virtual void clipRectChanged()
> +    {
> +        // Changing the clip rect doesn't affect the view hierarchy, so the plugin must be told about the change directly.
> +        [(WebBaseNetscapePluginView *)platformWidget() updateAndSetWindow];
> +    }

Ditto.
Comment 24 John Bauman 2013-02-27 12:07:03 PST
Thanks for the review.

(In reply to comment #23)
> (From update of attachment 190209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190209&action=review
> 
> 
> > Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:227
> > +    Widget::clipRectChanged();
> 
> Is this necessary? It would be nice to have a virtual function on Widget that doesn't require remembering to call the base class.
> 
You're right, this really shouldn't be necessary. I'll remove  it.
Comment 25 John Bauman 2013-02-27 13:08:51 PST
Created attachment 190583 [details]
Patch
Comment 26 WebKit Review Bot 2013-02-27 14:07:03 PST
Comment on attachment 190583 [details]
Patch

Clearing flags on attachment: 190583

Committed r144236: <http://trac.webkit.org/changeset/144236>
Comment 27 WebKit Review Bot 2013-02-27 14:07:10 PST
All reviewed patches have been landed.  Closing bug.