Implement "iframe shim" behavior for windowed plugins....

Implement "iframe shim" behavior for windowed plugins.

In FF and IE on windows, iframes are implemented as native HWNDs.  This has
the side effect that iframes display on top of windowed plugins, regardless
of z-order.  This side effect has long been known as a workaround for
allowing HTML elements to appear above plugin content.

BUG=1788

[ojan, 2008-09-08 05:39:11]

Drive-by review:

http://codereview.chromium.org/1612/diff/1/6
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/1612/diff/1/6#newcode545
Line 545: if (n && n->hasTagName(WebCore::HTMLNames::iframeTag)) {
I think you also need to make sure here that the z-index of the iframe is
greater than that of the plugin? You wouldn't want plugins that are on top of
iframes to not render their content.

[Thatcher Ulrich, 2008-09-08 22:06:11]

On 2008/09/08 05:39:11, ojan wrote:
> Drive-by review:
> 
> http://codereview.chromium.org/1612/diff/1/6
> File webkit/glue/webplugin_impl.cc (right):
> 
> http://codereview.chromium.org/1612/diff/1/6#newcode545
> Line 545: if (n && n->hasTagName(WebCore::HTMLNames::iframeTag)) {
> I think you also need to make sure here that the z-index of the iframe is
> greater than that of the plugin? You wouldn't want plugins that are on top of
> iframes to not render their content.

[I don't see my email replies in rietveld, so pardon the spam if you have seen
this before]

Here's a test page: http://tulrich.com/iframe_shims/

Actually, other browsers do not respect the z-order of the iframe for
the purposes of clipping a plugin window.  This is an artifact of the
way they are implemented on FF/Windows and IE.  An iframe uses a
distinct Windows HWND which clips the plugin, regardless of its
z-order and transparency.

In fact, iframe shims are often used by giving the iframe a large
negative z-index, and putting an ordinary div on top of it with a
higher z-index.  The iframe shim clips the plugin but does not
otherwise interfere with the page.

I know it's weird :)  But it's the only reasonable way I know of to
solve this problem.

Here are a couple references to the practice:
http://www.dotnetjunkies.com/WebLog/jking/archive/2003/07/21/488.aspx
http://codingforums.com/showthread.php?s=&threadid=28261

-T

[Thatcher Ulrich, 2008-09-08 22:07:45]

On 2008/09/08 22:06:11, Thatcher Ulrich wrote:
> 
> Here's a test page: http://tulrich.com/iframe_shims/

FYI I updated the patch to correct scrolling behavior.

-T

[Thatcher Ulrich, 2008-09-09 13:36:38]

[reitveld doesn't seem to be following the email thread so I'm quoting manually]

On Mon, Sep 8, 2008 at 7:50 PM, Darin Fisher <darin@google.com> wrote:
> This looks pretty good to me.  I'm curious about the z-order issue that Ojan
> raised.  Should a plugin contained by an IFRAME really be obscured by the
> containing IFRAME?

Good question.  I added an iframe test page:
http://tulrich.com/iframe_shims/iframe_index.html

IE7 and the patched Chrome behave in a consistent and expected way.  FF3 seems
to have some problems.

I think Chrome is doing the right thing because the
WebPluginImpl::GetIframeBounds() traversal starts with the plugin's frame's
document, which is inside the containing iframe.

-T

On 2008/09/08 22:07:45, Thatcher Ulrich wrote:
> On 2008/09/08 22:06:11, Thatcher Ulrich wrote:
> > 
> > Here's a test page: http://tulrich.com/iframe_shims/
> 
> FYI I updated the patch to correct scrolling behavior.
> 
> -T

[Thatcher Ulrich, 2008-09-09 13:39:47]

On 2008/09/09 13:36:38, Thatcher Ulrich wrote:
> [reitveld doesn't seem to be following the email thread so I'm quoting
manually]
> 
> On Mon, Sep 8, 2008 at 7:50 PM, Darin Fisher <darin@google.com> wrote:
> > This looks pretty good to me.  I'm curious about the z-order issue that Ojan
> > raised.  Should a plugin contained by an IFRAME really be obscured by the
> > containing IFRAME?
> 
> Good question.  I added an iframe test page:
> http://tulrich.com/iframe_shims/iframe_index.html
> 
> IE7 and the patched Chrome behave in a consistent and expected way.  FF3 seems
> to have some problems.

To be more specific, FF3 does OK with the first level of iframe, but if you
recursively navigate to two or more levels of iframe, FF3 messes up the plugin
clipping.

-T

[ojan, 2008-09-10 01:17:54]

OK. Talked to Hyatt and he's fine with making a change like this to WebKit. His
concern is that the actual behavior of IE/FF is probably a bit more complex.
Would you mind throwing together some more tests so that we can say definitively
what each of them does?

Specifically, it seems that z-index doesn't matter, but the order they're
appended to the document does? Also, what happens with plugins that are inside
iframes? In your example page, it seems that if you scroll the page, then it
stops clipping.

Again, as far as actual implementation goes, I defer to Darin and John. I just
want to make sure we get the behavior right. I still feel pretty strongly that
we should respect z-index ordering even if it breaks some sites.

Ojan

P.S. Sorry for taking so long to get back to you. It took forever to get ahold
of hyatt. I think he was away.

On 2008/09/09 13:39:47, Thatcher Ulrich wrote:
> On 2008/09/09 13:36:38, Thatcher Ulrich wrote:
> > [reitveld doesn't seem to be following the email thread so I'm quoting
> manually]
> > 
> > On Mon, Sep 8, 2008 at 7:50 PM, Darin Fisher <darin@google.com> wrote:
> > > This looks pretty good to me.  I'm curious about the z-order issue that
Ojan
> > > raised.  Should a plugin contained by an IFRAME really be obscured by the
> > > containing IFRAME?
> > 
> > Good question.  I added an iframe test page:
> > http://tulrich.com/iframe_shims/iframe_index.html
> > 
> > IE7 and the patched Chrome behave in a consistent and expected way.  FF3
seems
> > to have some problems.
> 
> To be more specific, FF3 does OK with the first level of iframe, but if you
> recursively navigate to two or more levels of iframe, FF3 messes up the plugin
> clipping.
> 
> -T

[Thatcher Ulrich, 2008-09-11 20:49:32]

Hi Ojan,

I updated the iframe test page at http://tulrich.com/iframe_shims/ to test
z-index scenarios and get better information.

IE7 has a more coherent treatment of iframe shims than FF, and it seems to obey
z-index / stacking order pretty well.  So I updated the patch to match IE7's
stacking order behavior.

Unfortunately the code for stacking order traversal is a bit involved, so maybe
it should be moved, and probably needs unit tests etc, but I haven't figured
that stuff out yet.  Anyway, it works pretty well via manual testing so I wanted
to post it.  Please let me know your thoughts on code location etc.

-T

On 2008/09/10 01:17:54, ojan wrote:
> OK. Talked to Hyatt and he's fine with making a change like this to WebKit.
His
> concern is that the actual behavior of IE/FF is probably a bit more complex.
> Would you mind throwing together some more tests so that we can say
definitively
> what each of them does?
> 
> Specifically, it seems that z-index doesn't matter, but the order they're
> appended to the document does? Also, what happens with plugins that are inside
> iframes? In your example page, it seems that if you scroll the page, then it
> stops clipping.
> 
> Again, as far as actual implementation goes, I defer to Darin and John. I just
> want to make sure we get the behavior right. I still feel pretty strongly that
> we should respect z-index ordering even if it breaks some sites.
> 
> Ojan
> 
> P.S. Sorry for taking so long to get back to you. It took forever to get ahold
> of hyatt. I think he was away.
> 
> On 2008/09/09 13:39:47, Thatcher Ulrich wrote:
> > On 2008/09/09 13:36:38, Thatcher Ulrich wrote:
> > > [reitveld doesn't seem to be following the email thread so I'm quoting
> > manually]
> > > 
> > > On Mon, Sep 8, 2008 at 7:50 PM, Darin Fisher <darin@google.com> wrote:
> > > > This looks pretty good to me.  I'm curious about the z-order issue that
> Ojan
> > > > raised.  Should a plugin contained by an IFRAME really be obscured by
the
> > > > containing IFRAME?
> > > 
> > > Good question.  I added an iframe test page:
> > > http://tulrich.com/iframe_shims/iframe_index.html
> > > 
> > > IE7 and the patched Chrome behave in a consistent and expected way.  FF3
> seems
> > > to have some problems.
> > 
> > To be more specific, FF3 does OK with the first level of iframe, but if you
> > recursively navigate to two or more levels of iframe, FF3 messes up the
plugin
> > clipping.
> > 
> > -T

[jam, 2008-09-15 18:37:44]

some nits

http://codereview.chromium.org/1612/diff/601/810
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/1612/diff/601/810#newcode121
Line 121: std::vector<gfx::Rect>,
put the variable name in a comment like the other parameters

http://codereview.chromium.org/1612/diff/601/809
File chrome/test/ui/layout_plugin_uitest.cpp (right):

http://codereview.chromium.org/1612/diff/601/809#newcode100
Line 100: // Try to click green button.
this comment should move down a line

http://codereview.chromium.org/1612/diff/601/807
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/1612/diff/601/807#newcode829
Line 829: cutout_rects_.resize(0);
just do a .clear() for consistency with the rest of our code

http://codereview.chromium.org/1612/diff/601/805
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/1612/diff/601/805#newcode1013
Line 1013: Reset(NULL);
why call this if Reset(NULL) does nothing?

http://codereview.chromium.org/1612/diff/601/805#newcode1057
Line 1057: && next_neg_ < layer_->negZOrderList()->size();
we put the && on the first line

http://codereview.chromium.org/1612/diff/601/805#newcode1059
Line 1059: Context NextNeg() {
we put a line between each function

http://codereview.chromium.org/1612/diff/601/805#newcode1159
Line 1159: //   if (!root) {
take this code out?

[Thatcher Ulrich, 2008-09-15 19:30:58]

http://codereview.chromium.org/1612/diff/601/810
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/1612/diff/601/810#newcode121
Line 121: std::vector<gfx::Rect>,
On 2008/09/15 18:37:48, jam wrote:
> put the variable name in a comment like the other parameters

Done.

http://codereview.chromium.org/1612/diff/601/809
File chrome/test/ui/layout_plugin_uitest.cpp (right):

http://codereview.chromium.org/1612/diff/601/809#newcode100
Line 100: // Try to click green button.
On 2008/09/15 18:37:48, jam wrote:
> this comment should move down a line 

Done.

http://codereview.chromium.org/1612/diff/601/807
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/1612/diff/601/807#newcode829
Line 829: cutout_rects_.resize(0);
On 2008/09/15 18:37:48, jam wrote:
> just do a .clear() for consistency with the rest of our code

Done.

http://codereview.chromium.org/1612/diff/601/805
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/1612/diff/601/805#newcode1013
Line 1013: Reset(NULL);
On 2008/09/15 18:37:48, jam wrote:
> why call this if Reset(NULL) does nothing?

No need, removed.

http://codereview.chromium.org/1612/diff/601/805#newcode1057
Line 1057: && next_neg_ < layer_->negZOrderList()->size();
On 2008/09/15 18:37:48, jam wrote:
> we put the && on the first line

Done.

http://codereview.chromium.org/1612/diff/601/805#newcode1059
Line 1059: Context NextNeg() {
On 2008/09/15 18:37:48, jam wrote:
> we put a line between each function

Done.

http://codereview.chromium.org/1612/diff/601/805#newcode1159
Line 1159: //   if (!root) {
On 2008/09/15 18:37:48, jam wrote:
> take this code out?

Done.

[ojan, 2008-09-16 01:34:18]

Couple nits about the test. I'd really like to see if there is more we can push
down into webcore since our behavior and Safari's really should match, given the
web compat issues. When we talked to hyatt about this, he was urging us to try
and do this in a cross-platform friendly way. 

In either case, this discussion ended with Darin saying he needed to think about
this more. Darin, you think you could just chime on the higher-level
architecture? Is there a reasonable way we could have more of this code be in
webcore?

Just so we're all on the same page, I'm pasting in the discussion we had on
#webkit:

<ojan>	someone actually sent us a patch that clips plugins where they are
overlapped by iframes
<ojan>	it simulates the same behavior
<Lebannen>	it's rendered obsolete if you have access to either the flash source,
so you can set wmode there, or the page source, so you can stick wmode in the
object definition, but it still gets used for CMSes and the like
<hyatt>	ojan: i guess one thing to investigate is whether that could be a quirk
<ojan>	hyatt: as in, only do that in quirksmode?
<hyatt>	yes
<roc>	KHTML does that
<roc>	on X
<ojan>	hyatt: i don't really see it as a historical quirk. the problem occurs
just the same in standards mode.
<ojan>	hyatt: don't get me wrong, it's a gross hack
<ojan>	hyatt: and i strongly prefer to not support it
<hyatt>	i have no real aversion to supporting it
<ojan>	hyatt: but in practice, this is a real pain for develpers
<hyatt>	however just clipping seems wrong
<hyatt>	i assume the iframe should also be above the plugin in zorder
<pkasting>	What if under the hood the engine could do gross hacks to make
plugins appear to obey Z-order?  This might involve arbitrary HWND creation, I
guess :(
<hyatt>	since the plugin presumably *should* sit on top of iframes that came
before it in the document
<hyatt>	if you're really modeling hwnd behavior
<ojan>	hyatt: yeah, i thought the same thing. in practice that's not how IE/FF
work, but that just seems wrong.
<hyatt>	ojan: wait, seriously?
<hyatt>	if the plugin is above the iframe, the plugin still won't draw over the
iframe?
<roc>	it depends
<ojan>	hyatt: the pushback i've gotten is that a common form of this hack is to
give the iframe a large negative z-index and the dialog/menu a positive one
<ojan>	hyatt: i was suprised, but that's how they work
<roc>	in Gecko, widget order normally tracks proper z-order, but not always
<hyatt>	css z-index is probably not relevant
<hyatt>	i'm more thinking of when they get added to the window hierarchy
<hyatt>	since probably that's all ie does
<hyatt>	probably more just document order
<ojan>	ah, yeah. i think you're right
<ojan>	it is document order
<ojan>	i'd like to do something sane here.
<hyatt>	anyway, i have no aversion to doing the hack, although it would be
essential to do it only for windowed plugins
<hyatt>	we have a method in webcore called windowClipRect
<hyatt>	that is used for this
<hyatt>	so it could be patched
<ojan>	hyatt: i agree. doing the hack seems fine to me too. the only big thing i
have an aversion to is ignoring z-index
<ojan>	that just seems wrong
<hyatt>	well the best thing you could do is get examples
<hyatt>	so we could see the hack in action
<hyatt>	get a feel for what the html looks like
<hyatt>	so we know whether we have to worry about real css stacking order
<ojan>	http://code.google.com/p/chromium/issues/detail?id=1788
<hyatt>	ooooh!
<hyatt>	pretty sure we do have this bug
<hyatt>	just nobody identified why
<ojan>	here's the thing. if youtube does actually use a negative z-index, i'm
able to evangelize to them to make the z-index make sense
<hyatt>	well they might use the negative z-index inside a positioned parent
<hyatt>	that is still above the plugin
<hyatt>	thats what i would need to see
<hyatt>	"negative z-index" by itself doesn't mean you're behind the plugin
necessarily :)
<hyatt>	gotta look at the whole RenderLayer tree to see
<hyatt>	there is very much a cross-platform function right in webcore that tells
you the clip rect for a given plugin though
<ojan>	sure, my point was just that it should respect the z-index you set
<hyatt>	so this could be made to work on all platforms
<hyatt>	which would be nice
<hyatt>	ojan: yeah i'm not sure we respect z-index now
<hyatt>	on the plugins themselves
<hyatt>	if you overlap two windowed plugins
<hyatt>	that's a whole other issue though i guess :)
<hyatt>	we wouldn't have to though if we just include plugins and iframes both
as things you check
<hyatt>	then the real HWND order in Windows wouldn't really matter
<ojan>	not sure i follow, you mean things you check z-index of?
<hyatt>	yeah i mean if you've clipped properly, the order of HWND siblings won't
matter
<hyatt>	the Windows OS z-order
<hyatt>	since they won't overlap each other at all
<ojan>	right
<ojan>	hyatt: only this this screws up is transparency
<ojan>	hyatt: but i think we just have to live with that.
<hyatt>	ojan: can a windowed plugin be transparent?
<fishd>	iframes can be transparent
<ojan>	what he said
<fishd>	what does it mean to have a transparent iframe above a windowed plugin
<hyatt>	that is easy to detect
<hyatt>	we could just not clip out transparent iframes
<hyatt>	that's very easy to detect
<fishd>	i think it turns out that people rely on the clipping
<ojan>	but what if it's semi-transparent?
<fishd>	and they expect the transparent iframe to blend with the parent frame
<hyatt>	ojan: also easyto detect
<ojan>	hyatt: do we clip in that case or no?
<hyatt>	oh i guess transparent iframes blend over the plugin
<hyatt>	bah
<ojan>	right
<hyatt>	if we don't clip, then you draw over the transparent iframe
<hyatt>	argh
<ojan>	but like i said, i think we jsut need to live with transparent iframes
clipping plugins.
<fishd>	ojan: i think that is required
<hyatt>	i mean, we *really* don't want HWNDs for iframes
<fishd>	at least i know of one usage where the transparent iframe is meant to
clip the plugin and blend with the parent frame
<ojan>	given that the primary current use of these is for shims for menus and
modal dialogs, it  more orless does something reasonable
<fishd>	hyatt: agreed... no way
<fishd>	hyatt: it would break our multiprocess arch
<ojan>	hyatt: seems like you're ok with clipping. we're ok with clipping. the
only question is z-index. seems like you don't care much either way?
<hyatt>	ojan: it would be good to get some test cases to get a feel for what IE
does
<hyatt>	e.g., document order vs. positioning etc.
<ojan>	hyatt: totally.
<hyatt>	ojan: to help us know what widgets we should clip out
<hyatt>	but yeah the hack sounds reasonable
*	hyatt assumes you guys are using windowClipRect from WebCore too
<hyatt>	to set the window rgn on your plugin hwnds
<hyatt>	so patching windowClipRect i would think would fix everybody
<hyatt>	(mac uses that too now)
<ojan>	fishd: you know the answer to that?
<ojan>	hyatt: i don't actually know our plugin code at all. :)
<hyatt>	i can't see why you would duplicate all the code that checks overflow
and iframes etc.
<hyatt>	so you probably use it
<fishd>	ojan: yes, we use windowClipRect
<hyatt>	sweet
<hyatt>	so that is the patch spot
<fishd>	but we actually do the following:
<fishd>	we assemble an array of clip rects... exclusion rects.. then we form a
HRGN in the browser process, and then apply that to the plugin HWND
<fishd>	hyatt: we need a clipping region, not a clipping rect
<hyatt>	fishd: why?
<hyatt>	fishd: there's no way in css to create a non-rectangular clip region
<eseidel>	WebView is kinda a bad name for us, given that it's not an NSView
<fishd>	hyatt: consider a plugin with an iframe in the center of it
<hyatt>	oh 
<fishd>	hyatt: consider a plugin that is inthe corner, clipped by horz and vert
scroll bars
<hyatt>	fishd: yes that will have to change
<hyatt>	windowClipRect only clipped to iframes and overflow sections
<fishd>	hyatt: right
<hyatt>	which was guaranteed to leave you with a single rect still
<hyatt>	once you clip to other plugins and iframes yeah
<hyatt>	it gets more complicated
<fishd>	:)
<fishd>	we send an array of rects to the browser
<hyatt>	are you clipping out other plugins or something?
*	hyatt is trying to understand why you'd already have had to deal with this
*	hyatt wonders if windowed plugins ever really overlap
<hyatt>	windowClipRect could maybe be renamed to windowClipRects
<hyatt>	and could return both a plugin rect and then a set of rects to clip out
or something
<hyatt>	or there could be a second method
<hyatt>	a peer to windowClipRect
<hyatt>	maybe that would be better
<ojan>	hyatt: btw, talking to pkasting and maruel, it sounds like we're not too
attached to our current RenderThemeWin. that was written really really long ago
and then tweaked a bunch recently to get the metrics to match IE/FF.
<fishd>	hyatt: i think i'd need to think about this some more
<hyatt>	fishd: yeah i guess we probably have run into this but nobody did the
analysis
<hyatt>	fishd: i knew about the youtube problem
<fishd>	hyatt: i wasn't too worried about the iframe-shim problem originally...
now i guess it is a commonly expected thing.
<ojan>	hyatt: i know of at least half a dozen google properties that do this of
the top of my head. i'm sure libraries like Dojo do this as well.
<ojan>	s/of/off
<hyatt>	fishd: it definitely sounds like something that should be solved in a
cross-platform way though, so that we don't have to revisit it on m ac/linux

http://codereview.chromium.org/1612/diff/604/851
File chrome/test/data/npapi/iframe_shims.html (right):

http://codereview.chromium.org/1612/diff/604/851#newcode31
Line 31: <div style="position: relative;">
For completeness sake, can you also add test cases for a different document
order? As in, where the iframes come before the embed.

http://codereview.chromium.org/1612/diff/604/851#newcode37
Line 37: <div id="red_square"
Maybe add comments below to clarify the thing being tests:

<!-- Div with z-index below the embed -->
<!-- Div with z-index above the embed -->

http://codereview.chromium.org/1612/diff/604/851#newcode53
Line 53: <p>Test that <a href="http://tulrich.com/iframe_shims/">iframe
I don't think we need the link here. The description is good though.

http://codereview.chromium.org/1612/diff/604/840
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/1612/diff/604/840#newcode1008
Line 1008: // TODO: this could go in webkit.
I would definitely prefer to see this in the appropriate place in WebKit. I
think they would be able to do a better code review for it to ensure
correctness, but also this code has little to do with plugins really.

[Thatcher Ulrich, 2008-09-16 20:56:16]

http://codereview.chromium.org/1612/diff/604/851
File chrome/test/data/npapi/iframe_shims.html (right):

http://codereview.chromium.org/1612/diff/604/851#newcode1
Line 1: <html><head>
(I figured out how to run this as a webkit layout_test, and moved it to
webkit/data/layout_test/pending/plugins/iframe-shims.html)

http://codereview.chromium.org/1612/diff/604/851#newcode31
Line 31: <div style="position: relative;">
On 2008/09/16 01:34:18, ojan wrote:
> For completeness sake, can you also add test cases for a different document
> order? As in, where the iframes come before the embed.

Done.

http://codereview.chromium.org/1612/diff/604/851#newcode37
Line 37: <div id="red_square"
On 2008/09/16 01:34:18, ojan wrote:
> Maybe add comments below to clarify the thing being tests:
> 
> <!-- Div with z-index below the embed -->
> <!-- Div with z-index above the embed -->
> 

Done.

http://codereview.chromium.org/1612/diff/604/851#newcode53
Line 53: <p>Test that <a href="http://tulrich.com/iframe_shims/">iframe
On 2008/09/16 01:34:18, ojan wrote:
> I don't think we need the link here. The description is good though.

Done.

http://codereview.chromium.org/1612/diff/604/844
File chrome/test/ui/layout_plugin_uitest.cpp (right):

http://codereview.chromium.org/1612/diff/604/844#newcode1
Line 1: // Copyright 2008, Google Inc.
I reverted this from the patch, since I managed to move the test to
webkit/data/layout_tests/

http://codereview.chromium.org/1612/diff/604/840
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/1612/diff/604/840#newcode1008
Line 1008: // TODO: this could go in webkit.
On 2008/09/16 01:34:18, ojan wrote:
> I would definitely prefer to see this in the appropriate place in WebKit. I
> think they would be able to do a better code review for it to ensure
> correctness, but also this code has little to do with plugins really.

I read hyatt's comments, and looked at the windowClipRect() API he mentioned. 
This is a method of WebCore::Widget, which is used for platform-dependent stuff
such as plugins.  This particular method is documented as a way to apply a clip
rect to a windowed plugin.  So I think we should add a windowCutoutRects() API
alongside it, as hyatt suggested.  I went ahead and made the changes to chrome's
WebPluginContainer (a subclass of WebCore::Widget) and hooked it up to
WebPluginImpl here.

However, I did not actually add the call to WebCore::Widget.  The main reason is
that this will involve changing third_party/webkit and webkit/pending, and I
don't want to fall into a build-system rathole.  I would like to leave it as a
TODO, to isolate the change which is already starting to sprawl.

Likewise, the RenderLayerIterator / StackingOrderIterator should go into webkit,
but I would like to do that in a separate change, for the same reasons.

I added TODO() comments in the appropriate places, with my name on them for
reference.

[Thatcher Ulrich, 2008-09-23 18:27:24]

Ping?

On 2008/09/16 20:56:16, Thatcher Ulrich wrote:
> http://codereview.chromium.org/1612/diff/604/851
> File chrome/test/data/npapi/iframe_shims.html (right):
> 
> http://codereview.chromium.org/1612/diff/604/851#newcode1
> Line 1: <html><head>
> (I figured out how to run this as a webkit layout_test, and moved it to
> webkit/data/layout_test/pending/plugins/iframe-shims.html)
> 
> http://codereview.chromium.org/1612/diff/604/851#newcode31
> Line 31: <div style="position: relative;">
> On 2008/09/16 01:34:18, ojan wrote:
> > For completeness sake, can you also add test cases for a different document
> > order? As in, where the iframes come before the embed.
> 
> Done.
> 
> http://codereview.chromium.org/1612/diff/604/851#newcode37
> Line 37: <div id="red_square"
> On 2008/09/16 01:34:18, ojan wrote:
> > Maybe add comments below to clarify the thing being tests:
> > 
> > <!-- Div with z-index below the embed -->
> > <!-- Div with z-index above the embed -->
> > 
> 
> Done.
> 
> http://codereview.chromium.org/1612/diff/604/851#newcode53
> Line 53: <p>Test that <a href="http://tulrich.com/iframe_shims/">iframe
> On 2008/09/16 01:34:18, ojan wrote:
> > I don't think we need the link here. The description is good though.
> 
> Done.
> 
> http://codereview.chromium.org/1612/diff/604/844
> File chrome/test/ui/layout_plugin_uitest.cpp (right):
> 
> http://codereview.chromium.org/1612/diff/604/844#newcode1
> Line 1: // Copyright 2008, Google Inc.
> I reverted this from the patch, since I managed to move the test to
> webkit/data/layout_tests/
> 
> http://codereview.chromium.org/1612/diff/604/840
> File webkit/glue/webplugin_impl.cc (right):
> 
> http://codereview.chromium.org/1612/diff/604/840#newcode1008
> Line 1008: // TODO: this could go in webkit.
> On 2008/09/16 01:34:18, ojan wrote:
> > I would definitely prefer to see this in the appropriate place in WebKit. I
> > think they would be able to do a better code review for it to ensure
> > correctness, but also this code has little to do with plugins really.
> 
> I read hyatt's comments, and looked at the windowClipRect() API he mentioned. 
> This is a method of WebCore::Widget, which is used for platform-dependent
stuff
> such as plugins.  This particular method is documented as a way to apply a
clip
> rect to a windowed plugin.  So I think we should add a windowCutoutRects() API
> alongside it, as hyatt suggested.  I went ahead and made the changes to
chrome's
> WebPluginContainer (a subclass of WebCore::Widget) and hooked it up to
> WebPluginImpl here.
> 
> However, I did not actually add the call to WebCore::Widget.  The main reason
is
> that this will involve changing third_party/webkit and webkit/pending, and I
> don't want to fall into a build-system rathole.  I would like to leave it as a
> TODO, to isolate the change which is already starting to sprawl.
> 
> Likewise, the RenderLayerIterator / StackingOrderIterator should go into
webkit,
> but I would like to do that in a separate change, for the same reasons.
> 
> I added TODO() comments in the appropriate places, with my name on them for
> reference.

[darin (slow to review), 2008-09-23 23:20:00]

LGTM

I'm fine with this landing initially in our code, but please do work with
webkit.org to see about upstreaming whatever portions are appropriate.

http://codereview.chromium.org/1612/diff/624/899
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/1612/diff/624/899#newcode612
Line 612: if (cutout_rects.size()) {
nit: it seems like it would be nifty to make this a helper function into
base/gfx/gdi_util.{h,cc} so that it doesn't have to be duplicated.  my
recommendation is to just "svn mv" bitmap_header.h to become gdi_util.h (same
for bitmap_header.cc).

http://codereview.chromium.org/1612/diff/624/897
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/1612/diff/624/897#newcode590
Line 590: class StackingOrderIterator {
nit: please move these classes into webkit/glue/stacking_order_iterator.{h,cc}
(or some other file name if you prefer).

http://codereview.chromium.org/1612/diff/624/896
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/1612/diff/624/896#newcode72
Line 72: virtual void windowCutoutRects(WTF::Vector<WebCore::IntRect>* cutouts)
const;
why is this virtual?

http://codereview.chromium.org/1612/diff/624/896#newcode183
Line 183: virtual void windowCutoutRects(WTF::Vector<WebCore::IntRect>* rects)
const;
why is this virtual?

i don't see this method on WebCore::Widget, so it probably shouldn't be in the
section for "Widget implementation"

[ojan, 2008-09-24 02:37:52]

LGTM pending addressing darin's comments. Thanks for getting the layout test
version working. That looks great.

[Thatcher Ulrich, 2008-09-25 14:56:22]

Comments addressed, see notes below.  I'm going to submit ASAP, once I'm
confident that the build is OK.

http://codereview.chromium.org/1612/diff/624/899
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/1612/diff/624/899#newcode612
Line 612: if (cutout_rects.size()) {
On 2008/09/23 23:20:01, Darin Fisher (Google) wrote:
> nit: it seems like it would be nifty to make this a helper function into
> base/gfx/gdi_util.{h,cc} so that it doesn't have to be duplicated.  my
> recommendation is to just "svn mv" bitmap_header.h to become gdi_util.h (same
> for bitmap_header.cc).

Done.

http://codereview.chromium.org/1612/diff/624/897
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/1612/diff/624/897#newcode590
Line 590: class StackingOrderIterator {
On 2008/09/23 23:20:01, Darin Fisher (Google) wrote:
> nit: please move these classes into webkit/glue/stacking_order_iterator.{h,cc}
> (or some other file name if you prefer).

Done.

http://codereview.chromium.org/1612/diff/624/896
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/1612/diff/624/896#newcode72
Line 72: virtual void windowCutoutRects(WTF::Vector<WebCore::IntRect>* cutouts)
const;
On 2008/09/23 23:20:01, Darin Fisher (Google) wrote:
> why is this virtual?

The idea is that it should become a virtual method of WebCore::Widget (hyatt
suggested this in one of the chats), so this would be the overload.

http://codereview.chromium.org/1612/diff/624/896#newcode183
Line 183: virtual void windowCutoutRects(WTF::Vector<WebCore::IntRect>* rects)
const;
On 2008/09/23 23:20:01, Darin Fisher (Google) wrote:
> why is this virtual?
> 
> i don't see this method on WebCore::Widget, so it probably shouldn't be in the
> section for "Widget implementation"

Moving to WebCore::Widget is a TODO, so I added a comment here.