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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jim Mathies [:jimm]]

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.

Comment 1

[Jim Mathies [:jimm]]

Also, we only need this on windows and linux.

Comment 2

[Jim Mathies [:jimm]]

Attachment: wip - base pluse bridge

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.

Comment 3

[Jim Mathies [:jimm]]

Attachment: wip

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.

Comment 4

[Jim Mathies [:jimm]]

Attachment: wip

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.

Comment 5

[Jim Mathies [:jimm]]

Attachment: wip

lots of cleanup, comment, etc..

Comment 6

[Jim Mathies [:jimm]]

Attachment: wip

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.

Comment 8

[Jim Mathies [:jimm]]

Attachment: wip

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

Comment 9

[Jim Mathies [:jimm]]

Attachment: wip

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

Comment 10

[Jim Mathies [:jimm]]

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.

Comment 11

[Jim Mathies [:jimm]]

Attachment: pt1 - add plugin widget tracking

I use this to lookup plugin widgets from the compositor.

Comment 12

[Jim Mathies [:jimm]]

Attachment: pt2 - add update window support

Provide a way to invoke PluginInstanceChild's UpdateWindow from the content process for a particular PluginInstanceParent.

Comment 13

[Jim Mathies [:jimm]]

Attachment: pt2 - add update window support

- include the ipdl change for UpdateWindow.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 16

[Jim Mathies [:jimm]]

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

Comment 17

[Jim Mathies [:jimm]]

Attachment: wip

Comment 18

[Jim Mathies [:jimm]]

This is currently hung up on some lifetime management work. (bug 1104999 has some detail.)

Comment 19

[Jim Mathies [:jimm]]

Attachment: wip

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.

Comment 21

[Jim Mathies [:jimm]]

Attachment: wip

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 22

[Jim Mathies [:jimm]]

Comment on attachment 8545505 [details] [diff] [review]
wip

oops, I had some reviews at the base of my queue. will repost in a sec.

Comment 23

[Jim Mathies [:jimm]]

Attachment: wip

Comment 24

[Jim Mathies [:jimm]]

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.

Comment 25

[Jim Mathies [:jimm]]

Attachment: wip

Comment 26

[Jim Mathies [:jimm]]

Attachment: pt1 - line ending cleanup from bug 669200

Comment 27

[Jim Mathies [:jimm]]

Attachment: pt2 - chrome side plugin widget tracking

Comment 28

[Jim Mathies [:jimm]]

Attachment: pt3 - content side plugin instance tracking

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.

Comment 29

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 30

[Jim Mathies [:jimm]]

Attachment: pt4 - collect vs. apply plugin updates during reflow

Layout bits which route plugin widget configuration to the compositor vs. trying to applying the config on puppet widget.

Comment 31

[Jim Mathies [:jimm]]

Attachment: pt5 - child side compositor routing of plugin updates

Plumbing to move plugin configuration updates over to the chrome side.

Comment 32

[Jim Mathies [:jimm]]

Attachment: pt6 - get tab offset api

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.

Comment 33

[Jim Mathies [:jimm]]

Attachment: pt7 - updates to child side PluginWidgetProxy

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.

Comment 34

[Jim Mathies [:jimm]]

Attachment: pt8 - retrieve widget configs for plugin frame

This is where the get tab offset api comes up.

Comment 35

[Jim Mathies [:jimm]]

Attachment: pt3.5 - sandboxed flash helper

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.

Comment 36

[Jim Mathies [:jimm]]

Attachment: pt10 - parent side compositor changes

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.

Comment 37

[Jim Mathies [:jimm]]

Attachment: pt11 - cleanup PluginWidgetParent

Ripping out the main thread apis we currently use to update chrome side plugin widgets.

Comment 38

[Jim Mathies [:jimm]]

Attachment: pt12 - PluginWidgetParent assertions

Add a bunch of assertion checks for debug builds to this code.

Comment 39

[Jim Mathies [:jimm]]

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

Comment 40

[Jim Mathies [:jimm]]

Attachment: pt13 - update PPluginWidget teardown

Updating the teardown logic for PPluginWidget.

Comment 41

[Jim Mathies [:jimm]]

Attachment: pt14 - UpdateWindow routing

This is the other side of comment 28.

Comment 42

[Jim Mathies [:jimm]]

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.

Comment 43

[Jim Mathies [:jimm]]

Attachment: pt14 - UpdateWindow routing

Went ahead and combined those two.

Comment 44

[Jim Mathies [:jimm]]

I also have some gtk specific changes and some nits to post. I need to test on gtk fiorst, will post these tomorrow.

Comment 45

[Jim Mathies [:jimm]]

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.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 48

[Jim Mathies [:jimm]]

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

Comment 49

[Jim Mathies [:jimm]]

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

Comment 50

[Jim Mathies [:jimm]]

Attachment: test case

Comment 51

[Jim Mathies [:jimm]]

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 52

[Jim Mathies [:jimm]]

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.

Comment 53

[Jim Mathies [:jimm]]

Attachment: wip

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

Comment 54

[Jim Mathies [:jimm]]

Attachment: wip

Comment 55

[Jim Mathies [:jimm]]

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.

Comment 56

[Jim Mathies [:jimm]]

Attachment: pt8 - retrieve widget configs for plugin frame

Comment 57

[Jim Mathies [:jimm]]

Attachment: pt10 - parent side compositor changes

Comment 58

[Jim Mathies [:jimm]]

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.

Comment 59

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 62

[Jim Mathies [:jimm]]

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

Comment 63

[Jim Mathies [:jimm]]

latest try builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=498330aaf018

Comment 64

[Jim Mathies [:jimm]]

(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

Comment 65

[Jim Mathies [:jimm]]

Well from debugging this a bit it seems offset doesn't play a part in the difference between Get and GetEffective calls.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 67

[Jim Mathies [:jimm]]

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

Comment 68

[Jim Mathies [:jimm]]

If you're curious, here's the walking code as it stands now - 

http://pastebin.mozilla.org/8223672

Comment 69

[Jim Mathies [:jimm]]

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

Comment 70

[Jim Mathies [:jimm]]

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

Comment 71

[Jim Mathies [:jimm]]

Attachment: pt9.5 - layer clip helper

Comment 72

[Jim Mathies [:jimm]]

Comment on attachment 8549895 [details] [diff] [review]
pt9.5 - layer clip helper

oops, that was pt 10.

Comment 73

[Jim Mathies [:jimm]]

Attachment: pt9.5 - layer clip helper

Comment 74

[Jim Mathies [:jimm]]

Attachment: pt10 - parent side compositor changes

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 77

[Jim Mathies [:jimm]]

Ok great, we're down to nits. :) Time to reorg this patch set and kick off a final set of reviews.

Comment 78

[Jim Mathies [:jimm]]

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

Comment 79

[Jim Mathies [:jimm]]

Attachment: pt4 - collect vs. apply plugin updates during reflow

Comment 80

[Jim Mathies [:jimm]]

Attachment: pt5 - child side routing of plugin data

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.

Comment 81

[Jim Mathies [:jimm]]

Attachment: pt6 - updates to child side PluginWidgetProxy

Aaron, mind looking this over? Mostly just ripping out code we no longer need. PluginWidgetProxy is the child side plugin widget class layout uses.

Comment 82

[Jim Mathies [:jimm]]

Attachment: pt7 - widget config

Adding some config data to nsIWidget's Configuration.

Comment 83

[Jim Mathies [:jimm]]

Attachment: pt8 - layer clip helper

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.

Comment 84

[Jim Mathies [:jimm]]

Attachment: pt9 - parent side compositor changes

The code that leverages 'layer clip helper' and updates plugins.

Comment 85

[Jim Mathies [:jimm]]

Attachment: pt10 - updates to parent side PluginWidgetParent

100% obsolete code removal.

Comment 86

[Jim Mathies [:jimm]]

Attachment: pt11 - PluginWidgetParent assertions

Adding some assertions to protect against unexpected failures in PluginWidgetParent.

Comment 87

[Jim Mathies [:jimm]]

Attachment: pt12 - update PPluginWidget teardown

Updating PluginWidget tear down. The commenting added here should explain the reasoning pretty well.

Comment 88

[Jim Mathies [:jimm]]

Attachment: pt13 - forward UpdateWindow calls on Windows

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.

Comment 89

[Jim Mathies [:jimm]]

Attachment: pt14 - updates for gtk

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.

Comment 90

[Jim Mathies [:jimm]]

Attachment: pt15 - misc. code nits captured along the way

Comment 91

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 94

[Jim Mathies [:jimm]]

Attachment: pt8 - layer clip helper

Comment 95

[Jim Mathies [:jimm]]

Attachment: pt8 - layer clip helper

fixed a nit in a comment

Comment 96

[Jim Mathies [:jimm]]

Attachment: pt8 - layer clip helper

Comment 97

[Jim Mathies [:jimm]]

Attachment: pt9 - chrome side compositor changes

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 100

[Jim Mathies [:jimm]]

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

Comment 101

[Jim Mathies [:jimm]]

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

Comment 102

[Jim Mathies [:jimm]]

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.

Comment 103

[Jim Mathies [:jimm]]

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

Comment 104

[Jim Mathies [:jimm]]

Hmm, no that doesn't look right. That parent assignment happens in RecvWillStop().

Comment 105

[Jim Mathies [:jimm]]

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.

Comment 106

[Jim Mathies [:jimm]]

pastebin of the layer dumps if anyone is curious - http://pastebin.mozilla.org/8383681

Comment 107

[Jim Mathies [:jimm]]

Attachment: pt9 - chrome side compositor changes

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.

Comment 108

[Jim Mathies [:jimm]]

Attachment: pt9 - chrome side compositor changes

Comment 109

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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"

Comment 111

[Jim Mathies [:jimm]]

Attachment: pt16 - fixes for gtk

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.

Comment 112

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 113

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f592cd6c79
ternatively, view them on TBPL (soon to be deprecated):

Comment 114

[Jim Mathies [:jimm]]

Attachment: rollup

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.

Comment 115

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c41ac1e4405e

Comment 116

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c05f8b0ce6bb
https://hg.mozilla.org/mozilla-central/rev/fc53ac4a756e
https://hg.mozilla.org/mozilla-central/rev/55d0da4cbed3
https://hg.mozilla.org/mozilla-central/rev/69c19887d787
https://hg.mozilla.org/mozilla-central/rev/905d21993e31
https://hg.mozilla.org/mozilla-central/rev/49ad0d06ab60
https://hg.mozilla.org/mozilla-central/rev/f1bac083d4ba
https://hg.mozilla.org/mozilla-central/rev/674f5f49d0d8
https://hg.mozilla.org/mozilla-central/rev/b9b207933991
https://hg.mozilla.org/mozilla-central/rev/f3b07e994214
https://hg.mozilla.org/mozilla-central/rev/51b36ab1efb7
https://hg.mozilla.org/mozilla-central/rev/b18a3b124854
https://hg.mozilla.org/mozilla-central/rev/903ba066e31d
https://hg.mozilla.org/mozilla-central/rev/0bc5fb8363a2
https://hg.mozilla.org/mozilla-central/rev/ac859b6a5de3
https://hg.mozilla.org/mozilla-central/rev/0b7e58a519df
https://hg.mozilla.org/mozilla-central/rev/c41ac1e4405e