Bug 28914

Created attachment 38935 [details] Add layout test

Created attachment 38936 [details] Proposed patch Remove the incorrect entry in Changlog in previous attachment

Created attachment 38956 [details] Updated patch (indent 4 spaces instead of 2)

Created attachment 38996 [details] Updated patch

Comment on attachment 38996 [details] Updated patch Quite a few of style issues below: > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 47984) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,18 @@ > +2009-09-02 Victor Wang <victorw@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=28914 > + > + Add layout test to test iframe shim can be used > + to overlay html above a windowed plugin. It checks > + premutations of iframe shimes in relationship to shimes => shims. > + var HEIGHT = 100; width > + var WIDTH = 100; height > + i.style.filter='progid:DXImageTransform.Microsoft.Alpha(style=0,opacity=0)'; This is IE-specific. Why do we need it here? > + var case_id = items; caseId > + var expect_clickable = tags.expect && tags.expect.indexOf('UNDER') == -1; expectClickable > + expected_clicks[case_id] = expect_clickable; expectedClicks > + var plugin_div_z; pluginDivZ > + var overlay_div_z_iframe; overlayDivZIframe > + var overlay_div_z_overlay; overlayDivZOverlay, and below, lots of underscore_delimited_var_names. WebKit convention is camelCase.

Created attachment 39000 [details] Updated patch with style fixes

What is an iframe shim? I am not familiar with this term in this context.

Created attachment 39004 [details] Updated patch with style fixes

iframe-shims are a fairly common hack to get around z-ordering issues with windowed plugins. For example, for a menu that needs to overlay on top of windowed flash. Since on Windows, windowed plugins are in their own HWND, they don't respect z-index and go on top of the menu. Iframes in IE/FF historically have been implemented using an HWND, so if you put an iframe behind the menu, it will block the paint of the plugin and allow your menu to show on top. A google search for iframe shim shows a lot of hits for this sort of thing. For a real-world example, look at any html dialog in any Google product (e.g. gmail, docs, etc.). Thatcher added support for this behavior to Chromium by clipping plugins when there's an iframe above them. There's a FIXME to move that logic into platform-agnostic code. This is the layout-test for that behavior (matches IE). I don't remember the details, but I think making it platform-agnostic was non-trivial, which is why it wasn't done that way to start with.

A couple notes to add to what Ojan said: On Linux+FF, iframe shims work very similarly and fill the same need as on the Windows browsers. On Mac, iframe shims are largely irrelevant -- there is no such thing as a windowed plugin so the HTML compositing model generally does the right thing. (There remain many complications when working with an OpenGL plugin, but iframe shims never helped with that on Mac, so the Google Earth Plugin has to use even worse workarounds.) There's an early description of the technique here: http://www.macridesweb.com/oltest/IframeShim.html I have a test page that explores some aspects of the technique: http://tulrich.com/iframe_shims/

Thanks Ojan and Thatcher on providing more background about this test. Dimitri Glazkov, I fixed the style issue (sorry, I thought the webkit style guide is for c/c++ code, not layout test). The IE specific filter is not required so I removed it. Please take another look and let me know if you have more comments.

Comment on attachment 39004 [details] Updated patch with style fixes Almost there! > + if (!tags.pluginNorelative) { > + parentdiv.style.position = 'relative'; > + } No braces around one-liners. > + > + if (pluginDivZ) { > + parentdiv.style.zIndex = pluginDivZ; > + } Ditto. > + root.appendChild(parentdiv); > + } else { > + if (!tags.pluginNorelative) { > + pd.style.position = 'relative'; Ditto. > + if (overlayDivZIframe) { > + id.style.zIndex = overlayDivZIframe; Ditto. > + if (overlayDivZOverlay) { > + od.style.zIndex = overlayDivZOverlay; > + } No braces + indent. > + if (overlayDivZOverlay) { > + parentdiv.style.zIndex = overlayDivZOverlay; > + } No braces. > + for (k in expectedClicks) { > + if (expectedClicks[k] && !clicks[k]) { > + waitingForMoreClicks = true; > + } else if (!expectedClicks[k] && clicks[k]) { Indent issue here. Should be one more to indent w/if?

Created attachment 39279 [details] Patch with style fixes

Created attachment 39280 [details] More style fixes

done. please take another look. thanks! (In reply to comment #12) > (From update of attachment 39004 [details]) > Almost there! > > > + if (!tags.pluginNorelative) { > > + parentdiv.style.position = 'relative'; > > + } > > No braces around one-liners. > > > + > > + if (pluginDivZ) { > > + parentdiv.style.zIndex = pluginDivZ; > > + } > > Ditto. > > > + root.appendChild(parentdiv); > > + } else { > > + if (!tags.pluginNorelative) { > > + pd.style.position = 'relative'; > > Ditto. > > > > + if (overlayDivZIframe) { > > + id.style.zIndex = overlayDivZIframe; > > Ditto. > > > > + if (overlayDivZOverlay) { > > + od.style.zIndex = overlayDivZOverlay; > > + } > > No braces + indent. > > > + if (overlayDivZOverlay) { > > + parentdiv.style.zIndex = overlayDivZOverlay; > > + } > > No braces. > > > + for (k in expectedClicks) { > > + if (expectedClicks[k] && !clicks[k]) { > > + waitingForMoreClicks = true; > > + } else if (!expectedClicks[k] && clicks[k]) { > > Indent issue here. Should be one more to indent w/if?

Comment on attachment 39280 [details] More style fixes r=me.

Strange. fatal: pathspec 'LayoutTests/plugins/resources/simple_blank.swf' did not match any files But yet svn-apply exit'd 0. I think that something is broken with svn-apply!

Comment on attachment 39280 [details] More style fixes Filed bug 29100. But the commit-queue can't handle this patch, and I think I'm going to have to turn it off until I can fix bug 29100.

Did you modify this patch by hand, or is this a patch produced straight from svn-create-patch? However the binary file was added seems to have confused svn-apply.

(In reply to comment #19) > Did you modify this patch by hand, or is this a patch produced straight from > svn-create-patch? However the binary file was added seems to have confused > svn-apply. The patch was created by svn-create-patch. The binary file has a prop set to svnLmime-type application/octet-stream, should this be changed?

No, I think our scripts are just broken.

(In reply to comment #21) > No, I think our scripts are just broken. OK. Thanks for your help! Let me know of anything need from me.

Comment on attachment 39280 [details] More style fixes OK. I've fixed bug 29100 and am now running the commit-queue with the patch applied. This should land properly now.

Comment on attachment 39280 [details] More style fixes Never mind. I don't have a patched copy running locally. We'll just wait for bug 29100 to be resolved.

Comment on attachment 39280 [details] More style fixes Clearing flags on attachment: 39280 Committed r48638: <http://trac.webkit.org/changeset/48638>

All reviewed patches have been landed. Closing bug.