Bug 52594

The layout test passes btw, but I think that may be bug with the test.

Created attachment 80559 [details] Patch

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

Isn't there some way we could put this into shared code?

(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?

(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

Created attachment 80938 [details] Patch

Created attachment 80942 [details] Patch

(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 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 :-)

Created attachment 80973 [details] Patch

(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.

(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 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?

Created attachment 81107 [details] Patch

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.

Created attachment 81245 [details] Patch

Comment on attachment 81245 [details] Patch Clearing flags on attachment: 81245 Committed r77660: <http://trac.webkit.org/changeset/77660>

All reviewed patches have been landed. Closing bug.

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.