Closed Bug 1095754 Opened 6 years ago Closed 6 years ago

Sync remote plugin widget clipping, bounds, and visibility with the render frame

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s m4+ ---

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(18 files, 41 obsolete files)

2.93 KB, patch
Details | Diff | Splinter Review
12.75 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
10.09 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
7.21 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
6.35 KB, patch
roc
: review+
Details | Diff | Splinter Review
31.59 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.77 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
5.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.23 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
4.24 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
12.51 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
9.65 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
15.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.35 KB, patch
Details | Diff | Splinter Review
5.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.35 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.56 KB, patch
roc
: review+
Details | Diff | Splinter Review
141.27 KB, patch
Details | Diff | Splinter Review
In the work in bug 669200 we add support for plugin widgets which are owned by chrome but are managed by content. Currently clipping, size, position, and visibility are sent over the main thread during layout. We need to change this such that:

1) layout accumulates this information rather than set it directly on the widget

2) send this information along with a suitable window identifier through the compositor to chrome. The data should come over with the render frame that matches the plugin widget updates.

3) create a bridge between the compositor thread in chrome and the main thread such that the compositor can fire off async messages to the main thread  prior to call to paint/present.

4) send widget updates to the main thread through the bridge and set them on the widget.
Also, we only need this on windows and linux.
Blocks: 1095930
Blocks: 1099512
Attached patch wip - base pluse bridge (obsolete) — Splinter Review
Part 1 - getting the data from layout in the content process down into the compositor and over to the main thread. 

I've basically attached this data like a side car to layer updates. There may be a way to integrate this into the Edits we pass around, which would avoid some of the plugin params I'm having to add. I didn't see an obvious connection between the layer associated with the plugin and the point at which layout configures widgets though, so I put this off.
Attached patch wip (obsolete) — Splinter Review
A few issues to sort out:

1) getting tab offsets on the parent side down in CompositorChild vs. grabbing the offset from the parent when we setup the data in the child.

2) there's an issue with Linux where I can't gain access to the nsPluginNativeWindowGtk* PluginWidgetParent holds.

3) scrolling static content doesn't produce layer updates so no plugin data gets sent. We call layout and do some sort of reflow to update plugin position. This needs to get hooked up to the compositor despite us not wanting to update content.
Attachment #8525563 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Fixing issue #3, turned out this had nothing to do with layer updates, the widget just needed invalidating when we update clipping or bounds.

I'm also seeing a lot of bug 1095776 during scroll which breaks window refresh in minor ways. I'll be looking at that next.
Attachment #8526258 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
lots of cleanup, comment, etc..
Attachment #8526299 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Added gtk support (in theory, I haven't bounced it over yet and built) plus various other changes.

This is basically ready, although I'm seeing some paint oddness on Windows. Part of this is bug 1095776 which I looked at today but couldn't come up with anything on. Also these changes shift window manipulation to a much later point in our refresh cycle.. I wonder if this might be throwing something off in flash.
Attachment #8526891 - Attachment is obsolete: true
Duplicate of this bug: 1095776
Attached patch wip (obsolete) — Splinter Review
This has the painting problems fixed. To do:

- there's a minor chrome clipping problem on Windows I want to look at.
- build and test on gtk
Attachment #8527067 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
latest wip - updated and working in gtk.

open issues:

- with click-to-play, when a plugin is activated there's no composition of content triggered, so plugins stay hidden until the next layer update. I need to find a way to trigger this so plugin data gets flushed.

- still looking for a way to get the tab parent ptr from a tab child id
Attachment #8527124 - Attachment is obsolete: true
Almost forgot, because of the way this sends update requests asynchronously to plugin instances, we often run into bug 1091766. The only way I can see fixing this is to fix that bug.
Depends on: 1091766
Attached patch pt1 - add plugin widget tracking (obsolete) — Splinter Review
I use this to lookup plugin widgets from the compositor.
Attachment #8527347 - Flags: review?(aklotz)
Attached patch pt2 - add update window support (obsolete) — Splinter Review
Provide a way to invoke PluginInstanceChild's UpdateWindow from the content process for a particular PluginInstanceParent.
Attachment #8527348 - Flags: review?(aklotz)
Attached patch pt2 - add update window support (obsolete) — Splinter Review
- include the ipdl change for UpdateWindow.
Attachment #8527348 - Attachment is obsolete: true
Attachment #8527348 - Flags: review?(aklotz)
Attachment #8527350 - Flags: review?(aklotz)
Comment on attachment 8527347 [details] [diff] [review]
pt1 - add plugin widget tracking

Review of attachment 8527347 [details] [diff] [review]:
-----------------------------------------------------------------

Other than my comments about the instantiation of the hash table, the rest of this is just fine.

::: widget/nsBaseWidget.cpp
@@ +63,5 @@
>  #ifdef XP_MACOSX
>  #include "nsCocoaFeatures.h"
>  #endif
>  
> +nsRefPtrHashtable<nsVoidPtrHashKey, nsIWidget> sPluginWidgetList;

Putting on my perf team hat, this is going to add static initializers for the hash table. I'd prefer that this be made into a pointer and be initialized and torn down elsewhere.
Attachment #8527347 - Flags: review?(aklotz) → feedback+
Comment on attachment 8527350 [details] [diff] [review]
pt2 - add update window support

Review of attachment 8527350 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +77,5 @@
> +/**
> + * e10s specific, used in cross referencing hwnds with plugin instances so we
> + * can access methods here from PluginWidgetChild.
> +*/
> +static nsClassHashtable<nsPtrHashKey<uintptr_t>, PluginInstanceParent> sPluginInstanceList;

Same concern with respect to this one.
Attachment #8527350 - Flags: review?(aklotz) → feedback+
(In reply to Aaron Klotz [:aklotz] from comment #14)
> Comment on attachment 8527347 [details] [diff] [review]
> pt1 - add plugin widget tracking
> 
> Review of attachment 8527347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Other than my comments about the instantiation of the hash table, the rest
> of this is just fine.
> 
> ::: widget/nsBaseWidget.cpp
> @@ +63,5 @@
> >  #ifdef XP_MACOSX
> >  #include "nsCocoaFeatures.h"
> >  #endif
> >  
> > +nsRefPtrHashtable<nsVoidPtrHashKey, nsIWidget> sPluginWidgetList;
> 
> Putting on my perf team hat, this is going to add static initializers for
> the hash table. I'd prefer that this be made into a pointer and be
> initialized and torn down elsewhere.

good catch. I've already updated this locally to use a pointer and new. Just didn't post. will post updates here in a bit.
Depends on: 1104999
No longer depends on: 1091766
Attached patch wip (obsolete) — Splinter Review
Attachment #8527344 - Attachment is obsolete: true
Attachment #8527347 - Attachment is obsolete: true
Attachment #8527350 - Attachment is obsolete: true
This is currently hung up on some lifetime management work. (bug 1104999 has some detail.)
Attached patch wip (obsolete) — Splinter Review
Updates to solve the problem of PluginWidgetParents tearing down in different ways.

I've come across another annoyance with this work though: when we tab switch, plugins don't get hidden since visibility information doesn't get flushed over to the widget via the compositor. This information is cached but is never picked up because we only composite the plugins for the new tab.

Looking for a fix to this next.
Attachment #8529129 - Attachment is obsolete: true
Depends on: 1106147
Blocks: 1106243
No longer depends on: 1106147
Duplicate of this bug: 1106147
Attachment #8529290 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
This is getting close. I've solved some problems with plugin state updates and tab switching that were hounding me for a bit. Painting of flash while scrolling still has some issues, I've tried to compensate for a number of things I was thinking might be the cause, but thus far no good fix. I think I'm close enough that this could land and we could deal with remaining issues in follow ups.
Comment on attachment 8545505 [details] [diff] [review]
wip

oops, I had some reviews at the base of my queue. will repost in a sec.
Attachment #8545505 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8545508 - Attachment is patch: true
The remaining paint problem I see on Windows appears to be bug 762948. I'm not a fan of that fix, but I do need to find a way to work around the same issue.
Attached patch wip (obsolete) — Splinter Review
Attachment #8545508 - Attachment is obsolete: true
Attachment #8546126 - Attachment is obsolete: true
Attachment #8546163 - Flags: review?(aklotz)
Provides a way to call UpdateWindow on plugin instances via PluginWidgetChild. In a follow up patch these calls will get routed from here - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindowGfx.cpp#197

I wanted to keep the two tracking mechanisms I'm using together in my queue.
Attachment #8546167 - Flags: review?(aklotz)
Comment on attachment 8546163 [details] [diff] [review]
pt2 - chrome side plugin widget tracking

Review of attachment 8546163 [details] [diff] [review]:
-----------------------------------------------------------------

r+ provided that my assumption is correct.

::: widget/nsBaseWidget.cpp
@@ +1776,5 @@
> +  MOZ_ASSERT(aWindowId);
> +  MOZ_ASSERT(aWidget);
> +  MOZ_ASSERT(aUserArg);
> +  const nsTArray<uintptr_t>* visible = static_cast<const nsTArray<uintptr_t>*>(aUserArg);
> +  if (!visible->Contains((uintptr_t)aWindowId) && !aWidget->Destroyed()) {

Am I correct to assume that visible is expected to be reasonably small (array vs hashtable)?
Attachment #8546163 - Flags: review?(aklotz) → review+
Attachment #8546167 - Flags: review?(aklotz) → review+
Layout bits which route plugin widget configuration to the compositor vs. trying to applying the config on puppet widget.
Plumbing to move plugin configuration updates over to the chrome side.
Attached patch pt6 - get tab offset api (obsolete) — Splinter Review
This is an unfortunate but needed sync api from content to chrome to retrieve the offset of the tab from the parent browser window. We need this in nsPluginFrame when we assemble the plugin configurations we'll forward over the compositor to chrome. 

An alternative to this sync call would be some sort of a tracking system on the chrome side where the compositor can look up and access a TabParent pointer based on a child tab id. The accessor would have to be static. I ran out of time on this but can file a follow up.
Rips out stuff we no longer need, adds the ability to grab the stored clip rects of plugins when we're ready to ship a layer tree update over.
This is where the get tab offset api comes up.
This is a simple patch that splits the code from bug 762948 out so that the compositor and and nsWindow on Windows can share it.
Receiving plugin widget config on the compositor thread, bouncing that to the main thread and applying the changes. This code also handles updating plugin widgets in background tabs that the compositor knows nothing about.
Ripping out the main thread apis we currently use to update chrome side plugin widgets.
Add a bunch of assertion checks for debug builds to this code.
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #29)
> Am I correct to assume that visible is expected to be reasonably small
> (array vs hashtable)?

Yes, size is the total number of plugin windows found in any random web page, so maybe worst case 100 or less? Usually < 10.
Updating the teardown logic for PPluginWidget.
Attached patch pt14 - UpdateWindow routing (obsolete) — Splinter Review
This is the other side of comment 28.
Comment on attachment 8546261 [details] [diff] [review]
pt14 - UpdateWindow routing

sorry, this is the setup for nsWindowGfx, nsWindowGfx changes coming up next. These two patches could probably be combined.
Attachment #8546261 - Attachment description: pt14 - nsWindowGfx update window fix → pt14 - UpdateWindow routing
Attached patch pt14 - UpdateWindow routing (obsolete) — Splinter Review
Went ahead and combined those two.
Attachment #8546261 - Attachment is obsolete: true
I also have some gtk specific changes and some nits to post. I need to test on gtk fiorst, will post these tomorrow.
Before I start spamming people with review requests.. roc, would you mind taking a quick scan of this work, specifically compositor and layout parts 4, 5, and 10? I think I'll end up having to flag you on these anyway, so I'd like to make sure you're ok with the approach.

Note we spoke at the all-hands about the layer system's involvement in this. One of the things I've found is that we do not build a layer in the child associated with windowed plugins that ships over in layer tree updates. This isn't a problem though since it's easy to get this data over independently when we ship updates over. With this said I think it would be pretty easy to swap out a layer for the plugin data struct I have now, although I don't see much point in it.
Flags: needinfo?(roc)
Comment on attachment 8546232 [details] [diff] [review]
pt5 - child side compositor routing of plugin updates

Review of attachment 8546232 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +259,5 @@
>  
> +// See nsIWidget Configurations
> +struct PluginWindowData {
> +  uintptr_t windowId;
> +  nsIntRect[] clips;

Just 'clip' or 'clipRects'.
(In reply to Jim Mathies [:jimm] from comment #45)
> Note we spoke at the all-hands about the layer system's involvement in this.
> One of the things I've found is that we do not build a layer in the child
> associated with windowed plugins that ships over in layer tree updates. This
> isn't a problem though since it's easy to get this data over independently
> when we ship updates over. With this said I think it would be pretty easy to
> swap out a layer for the plugin data struct I have now, although I don't see
> much point in it.

Yeah, let's think about this a bit.

The only significant problem I see right now with your setup is that the geometry and clipping computed by the content process are applied directly by the compositor. That could cause problems, e.g.:
a) If a <browser> element is inside a container element that gets async-scrolled, its plugins won't async-scroll.
b) In fact, it seems to me that if a <browser> element is moved at all, its plugins won't move.
c) If a <browser> element is inside a container that clips it, the plugins won't get clipped.
d) It seems to me that if chrome content covers a plugin we won't exclude that area from the plugin. This is really important.
Tell me if I'm wrong :-)

We can avoid those problems by computing more information from the layer tree like we discussed in Portland. There are a couple of options:
1) Let the content process report geometry and clipping data relative to its root layer. In the compositor, adjust that data to take into account the position and clipping of the content process root layer when it's finally composited (i.e. based on the transforms and clipping of its ancestor layers, clipping out the visible regions of any layers that cover the plugin).
2) Go deeper and associate each plugin with a layer, and compute position and clipping from the layer tree like we discussed in Portland (but as we discussed in Portland, we still need the content process to do the display-list work to compute a tight visible region for each plugin).
Some of the work we'd need to do for #2 we'd need for #1 anyway.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> The only significant problem I see right now with your setup is that the
> geometry and clipping computed by the content process are applied directly
> by the compositor. That could cause problems, e.g.:
> a) If a <browser> element is inside a container element that gets
> async-scrolled, its plugins won't async-scroll.

I think you could compensate for this pretty easily over on the chrome side. True though that the data coming over isn't apzc aware, it's static. The apzc would have to adjust everything based on where it positioned the viewable area.

> b) In fact, it seems to me that if a <browser> element is moved at all, its
> plugins won't move.

Wouldn't this trigger a composition for the chrome window? In which case we would get a notification and have the opportunity to update plugin window positions. In the current patches there is a problem with tab offset which is set on the child side. I'm currently looking at this since I'd prefer the offset be calculated on the chrome side.

If there are cases where a <browser> can shift and the compositor has no idea this happened, then yes, that would be a problem. However I'm not sure if that's isolated to the way I'm doing things here, if the layer tree doesn't refresh on the chrome side plugins aren't going to update no matter what. My assumption was that anytime something changes in the desktop browser window, the compositor associated with that window composes the view, triggering DidComposite events. At that point visible plugin data can be adjusted.
 
> c) If a <browser> element is inside a container that clips it, the plugins
> won't get clipped.

Yeah, this may be an issue, does this happen? I think we talked about this briefly in portland. Chrome can't obscure plugin windows aside from removing rectangular areas? But I need to compensate for that in some way.

> We can avoid those problems by computing more information from the layer
> tree like we discussed in Portland. There are a couple of options:
> 1) Let the content process report geometry and clipping data relative to its
> root layer. 

I believe this is where I'm at currently with this work independent of the tab offset number.

> In the compositor, adjust that data to take into account the
> position and clipping of the content process root layer when it's finally
> composited (i.e. based on the transforms and clipping of its ancestor
> layers, clipping out the visible regions of any layers that cover the
> plugin).

I get the sense I can do this right at the point where I'm ready to push the plugin data to the main thread, but I'm not sure how to "adjust that data to take into account the position and clipping of the content process root layer". I am familiar with the remote layer tree data for the tab stored in sIndirectLayerTrees, which has the parent of that tree and a root object I think, which I could probably query or walk up. Are there layer apis for extracting clipping and position? A mxr link or two would be appreciated. :) I'll poke more at this tomorrow.

> 2) Go deeper and associate each plugin with a layer, and compute position
> and clipping from the layer tree like we discussed in Portland (but as we
> discussed in Portland, we still need the content process to do the
> display-list work to compute a tight visible region for each plugin).
> Some of the work we'd need to do for #2 we'd need for #1 anyway.

Sounds very similar to #1, except you add the layer object to hold the data vs. sending that along independently. I ran into this when I started messing around with creating a custom layer for the data.. I didn't see any value in the actual layer object. I do have the wip from the experimenting I did and can resurrect the work if you think a layer is better than the struct. It wouldn't be much additional work.
Looks like the clip is available via Layer. Maybe from mRoot in LayerTreeState.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#1207
Attached file test case (obsolete) —
From poking at this over the weekend, it looks like content is contained within a RefLayerComposite, and that appears to have the clip and origin info I need.

One odd thing I noticed: when we display tab modal prompts, plugins get clipped out of the view (or maybe, just hidden completely) and that this information doesn't appear to be in the layer system at all. Looks like layout is controlli8ng this. I'll have to find a way to mimic this with e10s.
Comment on attachment 8546245 [details] [diff] [review]
pt3.5 - sandboxed flash helper

Review of attachment 8546245 [details] [diff] [review]:
-----------------------------------------------------------------

This just moves some code around.
Attachment #8546245 - Flags: review?(aklotz)
Attachment #8546238 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
- removed the offset calculates on the child side
- pulling offset info in the parent from content's parent ref layer

Open question, when retrieving the layer transform info, should we walk up the layer tree or is the transform in RefLayerComposite sufficient?
Attached patch wip (obsolete) — Splinter Review
Attachment #8547658 - Attachment is obsolete: true
Attachment #8548197 - Attachment is patch: true
Latest updates take care of clipping. Also, I've confirmed with works with apzc. Need to get my patch queue cleaned up then I'll post a new broken down set.
Attachment #8546241 - Attachment is obsolete: true
Attachment #8548197 - Attachment is obsolete: true
Attachment #8546248 - Attachment is obsolete: true
Roc, mind giving this another once over when you have a chance? I think I've addressed all of your concerns.

- pt6 is gone
- pt8 used the offset api in pt6, hence the update
- pt10 encompasses clipping and positioning on the parent side via the compositor.
Flags: needinfo?(roc)
Comment on attachment 8546245 [details] [diff] [review]
pt3.5 - sandboxed flash helper

Review of attachment 8546245 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinUtils.cpp
@@ +1025,5 @@
>  
>    return sGetKnownFolderPath(rfid, dwFlags, hToken, ppszPath);
>  }
>  
> +static BOOL WINAPI EnumFirstChild(HWND hwnd, LPARAM lParam)

nit: can you move the return type onto a separate line?

@@ +1049,5 @@
> +  RECT windowRect;
> +  RECT parentRect;
> +
> +  ::GetWindowRect(current, &parentRect);
> +        

Nit: whitespace on blank line
Attachment #8546245 - Flags: review?(aklotz) → review+
Comment on attachment 8548214 [details] [diff] [review]
pt10 - parent side compositor changes

Review of attachment 8548214 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1687,5 @@
> +          Rect empty;
> +          Rect trect = matrix.ProjectRectBounds(empty);
> +          nsIntPoint offset(trect.x, trect.y);
> +          const nsIntRegion& region = clayer->GetVisibleRegion();
> +          unused << lts.mParent->SendUpdatePluginConfigurations(offset, region, lts.mPluginData);

The general idea is good, but I think we need to generalize this to collect transforms and clipping (but not visible regions) all the way up the layer tree.

Something like this:
  region = clayer->visibleregion();
  offset = 0,0;
  for (layer = clayer; layer; layer = layer->GetParent()) {
    if (!layer->transform->IsTranslation()) {
      // bail out, this is hopeless
    }
    offset += layer->transform->translation;
    region += layer->transform->translation;
    if (layer->HasClipRect()) {
      region = region.intersect(layer->ClipRect());
    }
  }

Put it in a helper function in Layers.cpp.
Attachment #8548214 - Flags: feedback+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> Something like this:
>   region = clayer->visibleregion();
>   offset = 0,0;
>   for (layer = clayer; layer; layer = layer->GetParent()) {
>     if (!layer->transform->IsTranslation()) {
>       // bail out, this is hopeless
>     }
>     offset += layer->transform->translation;
>     region += layer->transform->translation;
>     if (layer->HasClipRect()) {
>       region = region.intersect(layer->ClipRect());
>     }
>   }

You also need to detect and handle cases where a plugin is covered by some chrome layer.

I think before applying layer->transform->translation, you need to do something like this:
  for (sibling = layer->GetNextSibling(); sibling; sibling = sibling->GetNextSibling()) {
    region.Sub(region, sibling->GetVisibleRegion());
  }

Also, you need to call GetLocalTransform instead of GetTransform, and GetEffectiveVisibleRegion instead of GetVisibleRegion, and GetEffectiveClipRect instead of GetClipRect.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #61)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> > Something like this:
> >   region = clayer->visibleregion();
> >   offset = 0,0;
> >   for (layer = clayer; layer; layer = layer->GetParent()) {
> >     if (!layer->transform->IsTranslation()) {
> >       // bail out, this is hopeless
> >     }
> >     offset += layer->transform->translation;
> >     region += layer->transform->translation;
> >     if (layer->HasClipRect()) {
> >       region = region.intersect(layer->ClipRect());
> >     }
> >   }
> 
> You also need to detect and handle cases where a plugin is covered by some
> chrome layer.

Would a tab modal prompt qualify as a chrome layer? That prompt clips correctly. I was looking for other way to test this. If you have any suggestions on test cases please let me know.

Right now I'm looking at positioning when displaying sidebars and the menu bars, scrolling, resizing of the main window, and tab prompts for chrome clipping.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
>   region = clayer->visibleregion();
>   offset = 0,0;
>   for (layer = clayer; layer; layer = layer->GetParent()) {
>     if (!layer->transform->IsTranslation()) {
>       // bail out, this is hopeless
>     }
>     offset += layer->transform->translation;
>     region += layer->transform->translation;
>     if (layer->HasClipRect()) {
>       region = region.intersect(layer->ClipRect());
>     }
>   }

I'm a little confused as to why Get(Effective)VisibleRegion on the parent RefLayerComposite doesn't have the proper information in it. According to the docs, this is all one needs to properly draw the layer to the screen. From testing, it appears to have the right data in it. I'll still put this together, but I'm not clear on why we need to walk up the chain here for this.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#1376
Flags: needinfo?(roc)
Well from debugging this a bit it seems offset doesn't play a part in the difference between Get and GetEffective calls.
Flags: needinfo?(roc)
(In reply to Jim Mathies [:jimm] from comment #62)
> Would a tab modal prompt qualify as a chrome layer?

Yes.

> That prompt clips correctly.

The semi-transparent overlay behind the alert() should hide the plugin completely. Does it?

I don't think it will, unless you write the occlusion-checking code I just suggested.

(In reply to Jim Mathies [:jimm] from comment #64)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> >   region = clayer->visibleregion();
> >   offset = 0,0;
> >   for (layer = clayer; layer; layer = layer->GetParent()) {
> >     if (!layer->transform->IsTranslation()) {
> >       // bail out, this is hopeless
> >     }
> >     offset += layer->transform->translation;
> >     region += layer->transform->translation;
> >     if (layer->HasClipRect()) {
> >       region = region.intersect(layer->ClipRect());
> >     }
> >   }
> 
> I'm a little confused as to why Get(Effective)VisibleRegion on the parent
> RefLayerComposite doesn't have the proper information in it. According to
> the docs, this is all one needs to properly draw the layer to the screen.
> From testing, it appears to have the right data in it. I'll still put this
> together, but I'm not clear on why we need to walk up the chain here for
> this.

For example, a <browser> in a scrolled container, or a <browser> in a translation-only CSS transform, will give you a RefLayerComposite in a ContainerLayer with a translation transform on it: your plugins won't be in the right place, because RefLayerComposite's Get(Effective)VisibleRegion is always relative to the top-left of the RefLayerComposite and doesn't take ancestor transforms into account.

If the <browser> is in a scrolled container, or in a container element with some other kind of clipping, then that clipping will normally reduce the RefLayerComposite visible region, but it won't always. For example if the RefLayerComposite is in a container with an async-animated transform, we will extend the visible region so the container is entirely pre-rendered, even if parts of it will be clipped out by some ancestor.
> > Would a tab modal prompt qualify as a chrome layer?
> 
> Yes.
> 
> > That prompt clips correctly.
> 
> The semi-transparent overlay behind the alert() should hide the plugin
> completely. Does it?

It doesn't, is this data in the sibling layers? I see the dialog clip in the (non)visible region of the effective visible region of the RefLayerComposite as I'm walking up. Maybe I need to walk sideways to find the overlay per your comment 61.

I was thinking the hidden plugins under semi-transparent overlays was a non-e10s bug. Are we sure we want to do that?

> 
> (In reply to Jim Mathies [:jimm] from comment #64)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> > >   region = clayer->visibleregion();
> > >   offset = 0,0;
> > >   for (layer = clayer; layer; layer = layer->GetParent()) {
> > >     if (!layer->transform->IsTranslation()) {
> > >       // bail out, this is hopeless
> > >     }
> > >     offset += layer->transform->translation;
> > >     region += layer->transform->translation;
> > >     if (layer->HasClipRect()) {
> > >       region = region.intersect(layer->ClipRect());
> > >     }
> > >   }
> > 
> > I'm a little confused as to why Get(Effective)VisibleRegion on the parent
> > RefLayerComposite doesn't have the proper information in it. According to
> > the docs, this is all one needs to properly draw the layer to the screen.
> > From testing, it appears to have the right data in it. I'll still put this
> > together, but I'm not clear on why we need to walk up the chain here for
> > this.
> 
> For example, a <browser> in a scrolled container, or a <browser> in a
> translation-only CSS transform, will give you a RefLayerComposite in a
> ContainerLayer with a translation transform on it: your plugins won't be in
> the right place, because RefLayerComposite's Get(Effective)VisibleRegion is
> always relative to the top-left of the RefLayerComposite and doesn't take
> ancestor transforms into account.
> 
> If the <browser> is in a scrolled container, or in a container element with
> some other kind of clipping, then that clipping will normally reduce the
> RefLayerComposite visible region, but it won't always. For example if the
> RefLayerComposite is in a container with an async-animated transform, we
> will extend the visible region so the container is entirely pre-rendered,
> even if parts of it will be clipped out by some ancestor.

Ok this kinda makes some sense. I'll try to put together a xul app or xul test of some sort to recreate the scrolled <browser> so I have something to test with. Do you know if we have any automated tests for stuff like this?

FWIW I have the walking up + accumulating implemented at this point and it seems to be working.
If you're curious, here's the walking code as it stands now - 

http://pastebin.mozilla.org/8223672
(In reply to Jim Mathies [:jimm] from comment #67)
> > > Would a tab modal prompt qualify as a chrome layer?
> > 
> > Yes.
> > 
> > > That prompt clips correctly.
> > 
> > The semi-transparent overlay behind the alert() should hide the plugin
> > completely. Does it?
> 
> It doesn't, is this data in the sibling layers? I see the dialog clip in the
> (non)visible region of the effective visible region of the RefLayerComposite
> as I'm walking up. Maybe I need to walk sideways to find the overlay per
> your comment 61.

Doesn't appear to be in any of the siblings of the layers as I walk up. Mostly what I see here are borders of content. Will keep digging for this overlay, must be in there somewhere.
(In reply to Jim Mathies [:jimm] from comment #69)
> (In reply to Jim Mathies [:jimm] from comment #67)
> > > > Would a tab modal prompt qualify as a chrome layer?
> > > 
> > > Yes.
> > > 
> > > > That prompt clips correctly.
> > > 
> > > The semi-transparent overlay behind the alert() should hide the plugin
> > > completely. Does it?
> > 
> > It doesn't, is this data in the sibling layers? I see the dialog clip in the
> > (non)visible region of the effective visible region of the RefLayerComposite
> > as I'm walking up. Maybe I need to walk sideways to find the overlay per
> > your comment 61.
> 
> Doesn't appear to be in any of the siblings of the layers as I walk up.
> Mostly what I see here are borders of content. Will keep digging for this
> overlay, must be in there somewhere.

Bah - bug 1121970.
Attached patch pt9.5 - layer clip helper (obsolete) — Splinter Review
Attachment #8548214 - Attachment is obsolete: true
Attachment #8549895 - Flags: review?(roc)
Comment on attachment 8549895 [details] [diff] [review]
pt9.5 - layer clip helper

oops, that was pt 10.
Attachment #8549895 - Attachment is obsolete: true
Attachment #8549895 - Flags: review?(roc)
Attached patch pt9.5 - layer clip helper (obsolete) — Splinter Review
Attachment #8549896 - Flags: review?(roc)
(In reply to Jim Mathies [:jimm] from comment #67)
> I was thinking the hidden plugins under semi-transparent overlays was a
> non-e10s bug. Are we sure we want to do that?

Yes. Otherwise it becomes possible for pages to use plugins to hide semi-transparent chrome overlays (such as the malicious-software warnings).

> Ok this kinda makes some sense. I'll try to put together a xul app or xul
> test of some sort to recreate the scrolled <browser> so I have something to
> test with. Do you know if we have any automated tests for stuff like this?

There's layout/base/tests/chrome/test_chrome_over_plugin.xul. For the other issues, I don't think so.
Comment on attachment 8549896 [details] [diff] [review]
pt9.5 - layer clip helper

Review of attachment 8549896 [details] [diff] [review]:
-----------------------------------------------------------------

basically looks OK, but a few details need to be fixed.

::: gfx/layers/Layers.cpp
@@ +859,5 @@
> +  nsIntRegion visibleRegion(GetEffectiveVisibleRegion());
> +
> +  for (Layer* layer = this; layer; layer = layer->GetParent()) {
> +    // AFAICT this is the layer's mTransform, which is relative
> +    // to the parent layer.

remove this comment

@@ +862,5 @@
> +    // AFAICT this is the layer's mTransform, which is relative
> +    // to the parent layer.
> +    gfx::Matrix matrix = layer->GetLocalTransform().As2D();
> +    if (!matrix.IsTranslation()) {
> +      continue;

We can't just continue. we have to bail out in some way.

@@ +870,5 @@
> +    Point currentLayerOffset = matrix.GetTranslation();
> +
> +    // As we step up the layer tree we want to expand visibleRegion to
> +    // include the entire window. As such translate the content visible
> +    // region down and away from the window origin.

This comment isn't clear to me.

@@ +871,5 @@
> +
> +    // As we step up the layer tree we want to expand visibleRegion to
> +    // include the entire window. As such translate the content visible
> +    // region down and away from the window origin.
> +    visibleRegion.MoveBy(currentLayerOffset.x, currentLayerOffset.y);

You're explicitly truncating from float to int here. Better use explicit rounding to make that clear.

@@ +874,5 @@
> +    // region down and away from the window origin.
> +    visibleRegion.MoveBy(currentLayerOffset.x, currentLayerOffset.y);
> +
> +    // If the parent layer clips it's lower layers, clip the visible region
> +    // we're accumulating. 

trailing whitespace

@@ +888,5 @@
> +    for (sibling = layer->GetNextSibling(); sibling; sibling = sibling->GetNextSibling()) {
> +      gfx::Matrix siblingMatrix = sibling->GetLocalTransform().As2D();
> +      // The translation from this layer to it's parent. Our visible region is
> +      // oriented with the parent.
> +      Point siblingOffset = siblingMatrix.GetTranslation();

Need to bail out here if the sibling has a non-translation transform, too.

::: gfx/layers/Layers.h
@@ +1249,5 @@
> +   *  to this layer.
> +   * @param aVisibleRegion - the resulting visible region of this layer
> +   *  after parent layer tree clipping has been applied.
> +   */
> +  void GetTopLevelVisibleRegion(nsIntPoint& aLayerOffset, nsIntRegion& aVisibleRegion);

This should return some kind of error if you encounter a transform that's not a translation.

You should also say how things are rounded if the transform is not an integer translation.

Please make it very clear that the final region is relative to the root layer. In fact I think it would be good to return the new region rather than overwriting aVisibleRegion, since they're in different coordinate systems.
Attachment #8549896 - Flags: review?(roc) → review-
Ok great, we're down to nits. :) Time to reorg this patch set and kick off a final set of reviews.
> Please make it very clear that the final region is relative to the root
> layer. In fact I think it would be good to return the new region rather than
> overwriting aVisibleRegion, since they're in different coordinate systems.

aVisibleRegion wasn't a great name but basically the routine expects an empty region here that it populates. Looking around Layer I don't see a common convention aside from incoming non-modifiable params being marked const.

I could break this up into two methods but then I'd have to do the walk twice, which seems bad. How about something like:

bool GetRootLevelVisibleRegion(nsIntRegion& aResult, nsIntPoint* aLayerOffset);

bool return on bad matrices, aLayerOffset is optional and aResult is the main result. Also aLayerOffset could be pass by reference as well. The header comment can make it clear what the params should contain, and asserts can insure that's the case?

Other requested changes addressed.
Attachment #8546231 - Attachment is obsolete: true
Attachment #8551784 - Flags: review?(roc)
Layout and compositor plumbing for plugin data.

I'm going to go ahead and posit that option 2 in comment 47 may be needed based on a bug I've run into with scrolling in release builds. However I would prefer to bump that out to a follow up so that I have some time to diagnose it.
Attachment #8546232 - Attachment is obsolete: true
Attachment #8551799 - Flags: review?(roc)
Aaron, mind looking this over? Mostly just ripping out code we no longer need. PluginWidgetProxy is the child side plugin widget class layout uses.
Attachment #8551803 - Flags: review?(aklotz)
Attachment #8546240 - Attachment is obsolete: true
Adding some config data to nsIWidget's Configuration.
Attachment #8546547 - Attachment is obsolete: true
Attachment #8548213 - Attachment is obsolete: true
Attachment #8551806 - Flags: review?(roc)
Attachment #8551806 - Attachment description: pt8 - widget config → pt7 - widget config
Attached patch pt8 - layer clip helper (obsolete) — Splinter Review
This should have everything in it you asked for except the return of a new region, I'm still using pass by reference for the results.
Attachment #8549896 - Attachment is obsolete: true
Attachment #8551808 - Flags: review?(roc)
Attachment #8546245 - Attachment description: pt9 - sandboxed flash helper → pt3.5 - sandboxed flash helper
The code that leverages 'layer clip helper' and updates plugins.
Attachment #8549898 - Attachment is obsolete: true
Attachment #8551811 - Flags: review?(roc)
Attachment #8551811 - Attachment description: pt10 - parent side compositor changes → pt9 - parent side compositor changes
100% obsolete code removal.
Attachment #8546249 - Attachment is obsolete: true
Attachment #8551812 - Flags: review?(aklotz)
Adding some assertions to protect against unexpected failures in PluginWidgetParent.
Attachment #8546250 - Attachment is obsolete: true
Attachment #8551814 - Flags: review?(aklotz)
Updating PluginWidget tear down. The commenting added here should explain the reasoning pretty well.
Attachment #8551815 - Flags: review?(aklotz)
This allows widget code in the chrome process to make an async UpdateWindow call on the top level plugin window all the way over in the plugin process.
Attachment #8546258 - Attachment is obsolete: true
Attachment #8546263 - Attachment is obsolete: true
Attachment #8551816 - Flags: review?(aklotz)
When the chrome side native window changes dims via compositor directed changes, we need to update the socket widget managed by nsPluginNativeWindowGtk, which is owned by PluginWidgetParent. This code stashes nsPluginNativeWindowGtk* in nsWindow so it can call that object directly to update the socket widget.
Attachment #8551818 - Flags: review?(roc)
Blocks: 1103177
Attachment #8551814 - Flags: review?(aklotz) → review+
Attachment #8551803 - Flags: review?(aklotz) → review+
Attachment #8551812 - Flags: review?(aklotz) → review+
Attachment #8551816 - Flags: review?(aklotz) → review+
Comment on attachment 8551815 [details] [diff] [review]
pt12 - update PPluginWidget teardown

Review of attachment 8551815 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginWidgetParent.cpp
@@ +147,5 @@
> +PluginWidgetParent::ParentDestroy()
> +{
> +  if (mActorDestroyed || !mWidget) {
> +    return;
> +  } 

nit: trailing whitespace
Attachment #8551815 - Flags: review?(aklotz) → review+
Comment on attachment 8551808 [details] [diff] [review]
pt8 - layer clip helper

Review of attachment 8551808 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +856,5 @@
> +{
> +  IntPoint offset;
> +  aResult = GetEffectiveVisibleRegion();
> +  for (Layer* layer = this; layer; layer = layer->GetParent()) {
> +    gfx::Matrix matrix = layer->GetLocalTransform().As2D();

You need to call Is2D here and handle the case where it's not.

@@ +865,5 @@
> +    // The offset of |layer| to its parent.
> +    IntPoint currentLayerOffset = RoundedToInt(matrix.GetTranslation());
> +
> +    // As we step up the layer tree we want to expand the visible region we're
> +    // accumulating to include the size of new parent layers. As such translate

This sentence doesn't make much sense to me.

@@ +871,5 @@
> +    // of |layer|.
> +    aResult.MoveBy(currentLayerOffset.x, currentLayerOffset.y);
> +
> +    // If the parent layer clips its lower layers, clip the visible region
> +    // we're accumulating. 

trailing whitespace

@@ +882,5 @@
> +    // remove these areas from the visible region as well. This will pick up
> +    // chrome overlays like a tab modal prompt.
> +    Layer* sibling;
> +    for (sibling = layer->GetNextSibling(); sibling; sibling = sibling->GetNextSibling()) {
> +      gfx::Matrix siblingMatrix = sibling->GetLocalTransform().As2D();

Again, call Is2D.

::: gfx/layers/Layers.h
@@ +1253,5 @@
>  
> +  /**
> +   * Retrieve the root level visible region for |this| taking into account
> +   * clipping applied to parent layers of |this| as well as sibling layers
> +   * of each parent. Matrix values in calculating offset of visible regions

"as well as subtracting visible regions of higher siblings of this layer and each ancestor."

@@ +1255,5 @@
> +   * Retrieve the root level visible region for |this| taking into account
> +   * clipping applied to parent layers of |this| as well as sibling layers
> +   * of each parent. Matrix values in calculating offset of visible regions
> +   * and in accumulating aLayerOffset are integer clipped using Point's
> +   * RoundedToInt.

Say "Translation values" instead of "matrix values".

@@ +1264,5 @@
> +   *  transform is found to be non-translational. This method returns early
> +   *  in this case, results will not be valid. Returns true on successful
> +   *  traversal.
> +   */
> +  bool GetRootLevelVisibleRegion(nsIntRegion& aResult, nsIntPoint* aLayerOffset);

Call this GetVisibleRegionRelativeToRootLayer and fix comment accordingly.
Attachment #8551808 - Flags: review?(roc) → review-
Comment on attachment 8551811 [details] [diff] [review]
pt9 - parent side compositor changes

Review of attachment 8551811 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorChild.h
@@ +71,5 @@
>    virtual bool RecvDidComposite(const uint64_t& aId, const uint64_t& aTransactionId) MOZ_OVERRIDE;
>  
> +  virtual bool
> +  RecvUpdatePluginConfigurations(const nsIntPoint& aContentOffset, 
> +                                 const nsIntRegion& aVisibleRegion, 

trailing whitespace

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1699,5 @@
>  void
>  CrossProcessCompositorParent::DidComposite(uint64_t aId)
>  {
>    sIndirectLayerTreesLock->AssertCurrentThreadOwns();
>    LayerTransactionParent *layerTree = sIndirectLayerTrees[aId].mLayerTree;

Delete this line

@@ +1710,5 @@
>      layerTree->SetPendingTransactionId(0);
>    }
> +
> +  // Update plugin window state on the main thread
> +  if (rootHasParent) {

Move all this to a helper function which starts with "if (!lts.mPluginData.Length()) { return; }"

@@ +1720,5 @@
> +      if (contentRoot) {
> +        nsIntPoint offset;
> +        nsIntRegion visibleRegion;
> +        if (contentRoot->GetRootLevelVisibleRegion(visibleRegion, &offset)) {
> +          unused << lts.mParent->SendUpdatePluginConfigurations(offset, visibleRegion,

This means that if a non-translation is introduced the plugin will remain "stuck" in its old configuration. That's a bit crazy and unpredictable. Instead I think you should hide it, which is consistent with what we already do.
Attachment #8551811 - Flags: review?(roc) → review-
Attached patch pt8 - layer clip helper (obsolete) — Splinter Review
Attachment #8551808 - Attachment is obsolete: true
Attachment #8554828 - Flags: review?(roc)
Attached patch pt8 - layer clip helper (obsolete) — Splinter Review
fixed a nit in a comment
Attachment #8554828 - Attachment is obsolete: true
Attachment #8554828 - Flags: review?(roc)
Attachment #8554831 - Flags: review?(roc)
Attachment #8554831 - Attachment is obsolete: true
Attachment #8554831 - Flags: review?(roc)
Attachment #8551811 - Attachment is obsolete: true
Attachment #8554832 - Flags: review?(roc)
Comment on attachment 8554832 [details] [diff] [review]
pt8 - layer clip helper

Review of attachment 8554832 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed

::: gfx/layers/Layers.cpp
@@ +859,5 @@
> +  aResult = GetEffectiveVisibleRegion();
> +  for (Layer* layer = this; layer; layer = layer->GetParent()) {
> +    gfx::Matrix matrix;
> +    if (!layer->GetLocalTransform().Is2D(&matrix)) {
> +      continue;

no, you need to return false here

@@ +888,5 @@
> +    for (sibling = layer->GetNextSibling(); sibling;
> +         sibling = sibling->GetNextSibling()) {
> +      gfx::Matrix siblingMatrix;
> +      if (!sibling->GetLocalTransform().Is2D(&siblingMatrix)) {
> +        continue;

return false here

@@ +893,5 @@
> +      }
> +
> +      if (!siblingMatrix.IsTranslation()) {
> +        return false;
> +      } else if (aResult.IsEmpty()) {

No "else" needed after return.

@@ +895,5 @@
> +      if (!siblingMatrix.IsTranslation()) {
> +        return false;
> +      } else if (aResult.IsEmpty()) {
> +        // No need to walk siblings, our visible region is already empty.
> +        break;

This optimization probably isn't worth doing.

@@ +914,5 @@
> +    offset += currentLayerOffset;
> +  }
> +
> +  if (aLayerOffset) {
> +    *aLayerOffset = nsIntPoint(offset.x, offset.y);

Are you ever going to pass null here? probably not. Just skip this null check.
Attachment #8554832 - Flags: review?(roc) → review+
Attachment #8554834 - Flags: review?(roc)
Comment on attachment 8554834 [details] [diff] [review]
pt9 - chrome side compositor changes

Review of attachment 8554834 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorChild.cpp
@@ +185,5 @@
> +  return false;
> +#else
> +  // Now that we are on the main thread, update plugin widget config.
> +  // This should happen a little before we paint to the screen assuming
> +  // the main thread is running freely.

Hmm, aren't we always going to be doing this after painting to the screen? I think so!

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1759,5 @@
> +    }
> +  }
> +
> +  // Hide all plugins, this remote layer tree is no longer active
> +  if (!rootHasParent || hidePlugins) {

I don't think we need to check rootHasParent here

@@ +1770,5 @@
> +    nsIntRegion region;
> +    unused << aLts.mParent->SendUpdatePluginConfigurations(offset,
> +                                                           region,
> +                                                           aLts.mPluginData);
> +    aLts.mPluginData.Clear();

I don't think we should clear this here.

For example, we might get some plugin info sent to us before rootHasParent is true, and then rootHasParent becomes true and we composite without new plugin info being sent to us.
Attachment #8554834 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #99)
> Comment on attachment 8554834 [details] [diff] [review]
> pt9 - chrome side compositor changes
> 
> Review of attachment 8554834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorChild.cpp
> @@ +185,5 @@
> > +  return false;
> > +#else
> > +  // Now that we are on the main thread, update plugin widget config.
> > +  // This should happen a little before we paint to the screen assuming
> > +  // the main thread is running freely.
> 
> Hmm, aren't we always going to be doing this after painting to the screen? I
> think so!

Ah yes, sorry that's an old comment. I originally triggered this from ShadowLayersUpdated. We may still want to, with the current timing plugins tend to be a little behind scroll. This is something I want to look at in a follow up..

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +1759,5 @@
> > +    }
> > +  }
> > +
> > +  // Hide all plugins, this remote layer tree is no longer active
> > +  if (!rootHasParent || hidePlugins) {
> 
> I don't think we need to check rootHasParent here

I think we do. Will check this again to be sure after all these changes. This case handled hiding plugins after tab switches.
 
> @@ +1770,5 @@
> > +    nsIntRegion region;
> > +    unused << aLts.mParent->SendUpdatePluginConfigurations(offset,
> > +                                                           region,
> > +                                                           aLts.mPluginData);
> > +    aLts.mPluginData.Clear();
> 
> I don't think we should clear this here.
> 
> For example, we might get some plugin info sent to us before rootHasParent
> is true, and then rootHasParent becomes true and we composite without new
> plugin info being sent to us.

Will test to see.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #98)
> Comment on attachment 8554832 [details] [diff] [review]
> pt8 - layer clip helper
> 
> Review of attachment 8554832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with those fixed
> 
> ::: gfx/layers/Layers.cpp
> @@ +859,5 @@
> > +  aResult = GetEffectiveVisibleRegion();
> > +  for (Layer* layer = this; layer; layer = layer->GetParent()) {
> > +    gfx::Matrix matrix;
> > +    if (!layer->GetLocalTransform().Is2D(&matrix)) {
> > +      continue;
> 
> no, you need to return false here

I don't understand why we avoid these layers with three dimensional transforms applied. Lets say you have a div that's rotated, and that contains a plugin. We can extract two dimensional transformation info from that layer and ignore the rotation such that the plugin origin would be in the right place, but the plugin would be flat on the screen. Is this something we would not want to do (and maybe hide the plugin instead?) Or am I missing something larger here?

> 
> @@ +893,5 @@
> > +      }
> > +
> > +      if (!siblingMatrix.IsTranslation()) {
> > +        return false;
> > +      } else if (aResult.IsEmpty()) {
> 
> No "else" needed after return.
> 
> @@ +895,5 @@
> > +      if (!siblingMatrix.IsTranslation()) {
> > +        return false;
> > +      } else if (aResult.IsEmpty()) {
> > +        // No need to walk siblings, our visible region is already empty.
> > +        break;
> 
> This optimization probably isn't worth doing.
> 
> @@ +914,5 @@
> > +    offset += currentLayerOffset;
> > +  }
> > +
> > +  if (aLayerOffset) {
> > +    *aLayerOffset = nsIntPoint(offset.x, offset.y);
> 
> Are you ever going to pass null here? probably not. Just skip this null
> check.

will update.
Hmm, so something seems to have changed in the compositor recently. In the past the following check:

bool rootHasParent = !!aLts.mRoot && !!aLts.mRoot->GetParent();

was a good indicator in CrossProcessCompositorParent::DidComposite that the remote layer tree was just presented to the screen. This doesn't seem to be the case anymore. On mc tip the parent of mRoot is now null after tab switches.
(In reply to Jim Mathies [:jimm] from comment #102)
> Hmm, so something seems to have changed in the compositor recently. In the
> past the following check:
> 
> bool rootHasParent = !!aLts.mRoot && !!aLts.mRoot->GetParent();
> 
> was a good indicator in CrossProcessCompositorParent::DidComposite that the
> remote layer tree was just presented to the screen. This doesn't seem to be
> the case anymore. On mc tip the parent of mRoot is now null after tab
> switches.

Possibly caused by bug 1121713.
Hmm, no that doesn't look right. That parent assignment happens in RecvWillStop().
From debugging this, I guess we can occasionally paint the old tab multiple times prior to rendering the new tab we are switching to. This breaks plugin display currently since I'm assuming I'll get a useful notification in DidComposite immediately after a remote layer tree update.
pastebin of the layer dumps if anyone is curious - http://pastebin.mozilla.org/8383681
To work around this, I've added a flag that tracks shadow layer updates such that we can identify when plugins should be hidden, which must occur after a shadow layer update that contains plugin updates, and after that layer tree has rendered at least once.
Attachment #8554834 - Attachment is obsolete: true
Attachment #8555457 - Attachment is obsolete: true
Attachment #8555460 - Flags: review?(roc)
(In reply to Jim Mathies [:jimm] from comment #101)
> I don't understand why we avoid these layers with three dimensional
> transforms applied. Lets say you have a div that's rotated, and that
> contains a plugin. We can extract two dimensional transformation info from
> that layer and ignore the rotation such that the plugin origin would be in
> the right place, but the plugin would be flat on the screen. Is this
> something we would not want to do (and maybe hide the plugin instead?) Or am
> I missing something larger here?

We want to hide the plugin in that case. That's our current behavior, and I think it's way better than displaying the plugin at some random location.
Comment on attachment 8555460 [details] [diff] [review]
pt9 - chrome side compositor changes

Review of attachment 8555460 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1741,5 @@
> +  bool shouldComposePlugins = !!lts.mRoot &&
> +                              !!lts.mRoot->GetParent() &&
> +                              lts.mUpdatedPluginDataAvailable;
> +
> +  bool shouldHidePlugins = (!lts.mRoot ||

Call these "shouldComposePlugin" and "shouldHidePlugin"
Attachment #8555460 - Flags: review?(roc) → review+
One final patch after getting all this built and tested on Ubuntu linux.

1) In pt2, the id I was using to track a widget was NS_NATIVE_PLUGIN_PORT. However on gtk, we hand the XID of the socket widget embedded in the native window to content. So there was a mismatch when the data came over, content had the socket XID, nsBaseWidget had the widget indexed under the native window XID.

To fix this I've added a new GetNativeData type (NS_NATIVE_PLUGIN_ID) that each platform must support. On gtk, we return the socket here, on windows we just hand back the plugin window hwnd.

2) I missed a Resize handler in gtk, so I broke the socket widget code out into a helper.
Attachment #8556117 - Flags: review?(roc)
Attached patch rollup (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f592cd6c79
ternatively, view them on TBPL (soon to be deprecated):
Attached patch rollupSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97d1bf4a39d4

Whitespace cleanup and I disabled two plugin tests under e10s. Plugins often init after the content process reflows in simple test cases, so plugin data doesn't get shipped over until something else in the page changes. Will file a follow up on this.
Attachment #8556416 - Attachment is obsolete: true
Depends on: 1127595
Blocks: 1127794
No longer blocks: 1106243
Depends on: 1128079
No longer depends on: 1127595
Depends on: 1152049
Depends on: 1152080
No longer depends on: 1152049
Depends on: 1212484
No longer depends on: 1212484
Depends on: 1212484
Depends on: 1152049
No longer depends on: 1152049
You need to log in before you can comment on or make changes to this bug.