Bug 52594 - Move chromium iframe shim code to cross-platform file
Summary: Move chromium iframe shim code to cross-platform file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-01-17 13:30 PST by Robert Hogan
Modified: 2011-02-04 13:59 PST (History)
9 users (show)

See Also:


Attachments
Patch (10.27 KB, patch)
2011-01-29 04:52 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (22.21 KB, patch)
2011-02-02 11:44 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (21.50 KB, patch)
2011-02-02 11:58 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (21.19 KB, patch)
2011-02-02 14:46 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2011-02-03 13:28 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (21.06 KB, patch)
2011-02-04 10:50 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2011-01-17 13:30:04 PST
See https://bugs.webkit.org/show_bug.cgi?id=46223 for more detail

Relevant unit test is plugins/iframe-shims.html

If you try out plugins/iframe-shims.html in QtTestBrowser, you'll see the iframe is hidden behind the plugin.

So the z-order is wrong. Hoping Kenneth can shed some light!
Comment 1 Robert Hogan 2011-01-17 13:40:25 PST
The layout test passes btw, but I think that may be bug with the test.
Comment 2 Robert Hogan 2011-01-29 04:52:23 PST
Created attachment 80559 [details]
Patch
Comment 3 Kenneth Rohde Christiansen 2011-01-29 15:03:41 PST
Comment on attachment 80559 [details]
Patch

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

> Source/WebCore/plugins/PluginView.cpp:1531
> +static void getObjectStack(const RenderObject* ro,
> +                           Vector<const RenderObject*>* roStack)

Keep this one line

> Source/WebCore/plugins/PluginView.cpp:1543
> +static bool checkStackOnTop(
> +        const Vector<const RenderObject*>& iframeZstack,
> +        const Vector<const RenderObject*>& pluginZstack)

Also here.

I do not like the name very much, what about isIframeAbovePlugin or so? that seems more webkitish

> Source/WebCore/plugins/PluginView.cpp:1547
> +    for (size_t i1 = 0, i2 = 0;
> +         i1 < iframeZstack.size() && i2 < pluginZstack.size();
> +         i1++, i2++) {

Are i1 and i2 ever different?

> Source/WebCore/plugins/PluginView.cpp:1584
> +            // Inspect the document order.  Later order means higher
> +            // stacking.

One line

> Source/WebCore/plugins/PluginView.cpp:1609
> +void PluginView::windowCutOutRects(const IntRect& frameRect,
> +                                               Vector<IntRect>& cutOutRects)

One line. I'm not sure cutOut rects is the most descriptive name

> Source/WebCore/plugins/PluginView.cpp:1619
> +    // Get the parent widget

I find the comment a bit redundant

> Source/WebCore/plugins/PluginView.cpp:1624
> +    FrameView* parentFrameView = static_cast<FrameView*>(parentWidget);

There is no toFrameView method?

> Source/WebCore/plugins/PluginView.cpp:1633
> +        const FrameView* frameView =
> +            static_cast<const FrameView*>((*it).get());

one line :-)

> Source/WebCore/plugins/PluginView.cpp:1649
> +                IntPoint point =
> +                    roundedIntPoint(iframeRenderer->localToAbsolute());

I would keep this on one line
Comment 4 Darin Fisher (:fishd, Google) 2011-01-31 13:29:25 PST
Isn't there some way we could put this into shared code?
Comment 5 Robert Hogan 2011-02-01 11:02:59 PST
(In reply to comment #4)
> Isn't there some way we could put this into shared code?

See https://bugs.webkit.org/show_bug.cgi?id=28914#c9

AFAICT Chromium instantiates its own plugin objects and only uses PluginViewNone from the WebCore code.

The functions could be added as simple utility functions in a file PluginViewSupport.cpp and shared there. Would that be acceptable?
Comment 6 Darin Fisher (:fishd, Google) 2011-02-01 14:11:04 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Isn't there some way we could put this into shared code?
> 
> See https://bugs.webkit.org/show_bug.cgi?id=28914#c9
> 
> AFAICT Chromium instantiates its own plugin objects and only uses PluginViewNone from the WebCore code.

I think we subclass PluginViewBase now.  See WebKit/chromium/src/WebPluginContainerImpl.{h,cpp}


> 
> The functions could be added as simple utility functions in a file PluginViewSupport.cpp and shared there. Would that be acceptable?

Yeah, this is what I was suggesting we do.  Maybe the file could be more specifically named... IFrameShimSupport.{h,cpp}?  That's not the best name either :-P
Comment 7 Robert Hogan 2011-02-02 11:44:52 PST
Created attachment 80938 [details]
Patch
Comment 8 Robert Hogan 2011-02-02 11:58:59 PST
Created attachment 80942 [details]
Patch
Comment 9 Robert Hogan 2011-02-02 12:00:30 PST
(In reply to comment #6)
> Yeah, this is what I was suggesting we do.  Maybe the file could be more specifically named... IFrameShimSupport.{h,cpp}?  That's not the best name either :-P

Both names suck, so I chose mine! Please r- if you think a file that can take future cross-platform utility functions is a bad idea.
Comment 10 Darin Fisher (:fishd, Google) 2011-02-02 12:35:58 PST
(In reply to comment #9)
> (In reply to comment #6)
> > Yeah, this is what I was suggesting we do.  Maybe the file could be more specifically named... IFrameShimSupport.{h,cpp}?  That's not the best name either :-P
> 
> Both names suck, so I chose mine! Please r- if you think a file that can take future cross-platform utility functions is a bad idea.

In general, WebCore seems to frown at giving files a "utility" style name as that suggests a future dumping ground.  Instead, if we can use a more specific name, then people won't be tempted to turn this file into a dumping ground.  Implicit in that argument is that dumping grounds are bad :-)
Comment 11 Robert Hogan 2011-02-02 14:46:52 PST
Created attachment 80973 [details]
Patch
Comment 12 Robert Hogan 2011-02-02 14:47:56 PST
(In reply to comment #3)
> 
> Are i1 and i2 ever different?
> 

Updated per your comments. I don't think i1 and i2 are ever different, so have remove the duplication.
Comment 13 Robert Hogan 2011-02-02 14:48:18 PST
(In reply to comment #10)
> In general, WebCore seems to frown at giving files a "utility" style name as that suggests a future dumping ground.  Instead, if we can use a more specific name, then people won't be tempted to turn this file into a dumping ground.  Implicit in that argument is that dumping grounds are bad :-)

Done!
Comment 14 Darin Fisher (:fishd, Google) 2011-02-02 14:58:12 PST
Comment on attachment 80973 [details]
Patch

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

Sorry, just nit-picking wording at this point.  I hope you find this helpful :)

> Source/WebCore/plugins/IframeShimSupport.h:20
> +#ifndef IframeShimSupport_h

nit: IFrame* instead of Iframe* to match HTMLIFrameElement.h ?

> Source/WebCore/plugins/IframeShimSupport.h:30
> +void windowRectsPluginShouldNotPaintOver(Element*, Widget* parentWidget, const IntRect& frameRect, Vector<IntRect>& cutOutRects);

it might work nicely to use the term "occlusions" here.  the plugin is occluded by iframes on the page.

  {compute,calculate,get}PluginOcclusions

I think the Chromium code is using the term "cut-outs" in the same way that I'm trying to use occlusions.  I suppose you could also use the term "clip" here.

  {compute,calculate,get}PluginClipRects

These seem to say the same thing as "window rects the plugin should not paint over" and are a bit shorter and in the case of "clip rects" more canonical?
Comment 15 Robert Hogan 2011-02-03 13:28:54 PST
Created attachment 81107 [details]
Patch
Comment 16 Darin Fisher (:fishd, Google) 2011-02-03 13:58:28 PST
Comment on attachment 81107 [details]
Patch

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

Looks good, just one remaining nit.  R=me w/ that resolved.

> Source/WebCore/plugins/IFrameShimSupport.h:30
> +void getPluginOcclusions(Element*, Widget* parentWidget, const IntRect& frameRect, Vector<IntRect>& cutOutRects);

nit-picky nit: rename |cutOutRects| to |occlusions| to be consistent.
Comment 17 Robert Hogan 2011-02-04 10:50:25 PST
Created attachment 81245 [details]
Patch
Comment 18 WebKit Commit Bot 2011-02-04 12:49:38 PST
Comment on attachment 81245 [details]
Patch

Clearing flags on attachment: 81245

Committed r77660: <http://trac.webkit.org/changeset/77660>
Comment 19 WebKit Commit Bot 2011-02-04 12:49:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Commit Bot 2011-02-04 13:59:52 PST
The commit-queue encountered the following flaky tests while processing attachment 81245 [details]:

http/tests/websocket/tests/handshake-fail-by-sub-protocol-mismatch.html bug 53809 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.