Bug 28914 - Add a test for iframe shims and windowed plugin
Summary: Add a test for iframe shims and windowed plugin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Victor Wang
URL:
Keywords:
Depends on: 29100
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-02 11:39 PDT by Victor Wang
Modified: 2009-09-22 10:19 PDT (History)
8 users (show)

See Also:


Attachments
Add layout test (10.73 KB, patch)
2009-09-02 11:44 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Proposed patch (10.12 KB, patch)
2009-09-02 11:49 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Updated patch (indent 4 spaces instead of 2) (10.93 KB, patch)
2009-09-02 17:41 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Updated patch (10.90 KB, patch)
2009-09-03 10:45 PDT, Victor Wang
dglazkov: review-
Details | Formatted Diff | Diff
Updated patch with style fixes (10.79 KB, patch)
2009-09-03 11:48 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Updated patch with style fixes (10.70 KB, patch)
2009-09-03 13:21 PDT, Victor Wang
dglazkov: review-
Details | Formatted Diff | Diff
Patch with style fixes (10.57 KB, patch)
2009-09-09 11:01 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
More style fixes (10.58 KB, patch)
2009-09-09 11:05 PDT, Victor Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 2009-09-02 11:39:17 PDT
Add a test to test iframe shims can be used to overlay html above a windows plugin.
Comment 1 Victor Wang 2009-09-02 11:44:42 PDT
Created attachment 38935 [details]
Add layout test
Comment 2 Victor Wang 2009-09-02 11:49:16 PDT
Created attachment 38936 [details]
Proposed patch

Remove the incorrect entry in Changlog in previous attachment
Comment 3 Victor Wang 2009-09-02 17:41:41 PDT
Created attachment 38956 [details]
Updated patch (indent 4 spaces instead of 2)
Comment 4 Victor Wang 2009-09-03 10:45:49 PDT
Created attachment 38996 [details]
Updated patch
Comment 5 Dimitri Glazkov (Google) 2009-09-03 10:54:52 PDT
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.
Comment 6 Victor Wang 2009-09-03 11:48:51 PDT
Created attachment 39000 [details]
Updated patch with style fixes
Comment 7 Sam Weinig 2009-09-03 13:06:36 PDT
What is an iframe shim?  I am not familiar with this term in this context.
Comment 8 Victor Wang 2009-09-03 13:21:33 PDT
Created attachment 39004 [details]
Updated patch with style fixes
Comment 9 Ojan Vafai 2009-09-03 13:37:06 PDT
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.
Comment 10 Thatcher Ulrich 2009-09-03 13:46:04 PDT
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/
Comment 11 Victor Wang 2009-09-03 13:51:18 PDT
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 12 Dimitri Glazkov (Google) 2009-09-09 10:45:58 PDT
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?
Comment 13 Victor Wang 2009-09-09 11:01:24 PDT
Created attachment 39279 [details]
Patch with style fixes
Comment 14 Victor Wang 2009-09-09 11:05:47 PDT
Created attachment 39280 [details]
More style fixes
Comment 15 Victor Wang 2009-09-09 11:07:07 PDT
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 16 Dimitri Glazkov (Google) 2009-09-09 11:35:47 PDT
Comment on attachment 39280 [details]
More style fixes

r=me.
Comment 17 Eric Seidel (no email) 2009-09-09 11:47:27 PDT
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 18 Eric Seidel (no email) 2009-09-09 11:55:36 PDT
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.
Comment 19 Eric Seidel (no email) 2009-09-09 14:13:30 PDT
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.
Comment 20 Victor Wang 2009-09-09 14:19:07 PDT
(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?
Comment 21 Eric Seidel (no email) 2009-09-09 14:20:12 PDT
No, I think our scripts are just broken.
Comment 22 Victor Wang 2009-09-09 14:22:06 PDT
(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 23 Eric Seidel (no email) 2009-09-21 16:34:35 PDT
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 24 Eric Seidel (no email) 2009-09-21 16:43:48 PDT
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 25 WebKit Commit Bot 2009-09-22 10:19:28 PDT
Comment on attachment 39280 [details]
More style fixes

Clearing flags on attachment: 39280

Committed r48638: <http://trac.webkit.org/changeset/48638>
Comment 26 WebKit Commit Bot 2009-09-22 10:19:35 PDT
All reviewed patches have been landed.  Closing bug.