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

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm4+)

Details

Attachments

(18 attachments, 41 obsolete attachments)

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
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Also, we only need this on windows and linux.
(Assignee)

Updated

5 years ago
Blocks: 1095930
(Assignee)

Updated

5 years ago
Blocks: 1099512
(Assignee)

Comment 2

5 years ago
Posted 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.
(Assignee)

Comment 3

5 years ago
Posted 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.
(Assignee)

Updated

5 years ago
Attachment #8525563 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Posted 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
(Assignee)

Comment 5

5 years ago
Posted patch wip (obsolete) — Splinter Review
lots of cleanup, comment, etc..
Attachment #8526299 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Posted 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
(Assignee)

Updated

5 years ago
Duplicate of this bug: 1095776
(Assignee)

Comment 8

5 years ago
Posted 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
(Assignee)

Comment 9

5 years ago
Posted 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
(Assignee)

Comment 10

5 years ago
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
(Assignee)

Comment 11

5 years ago
I use this to lookup plugin widgets from the compositor.
Attachment #8527347 - Flags: review?(aklotz)
(Assignee)

Comment 12

5 years ago
Provide a way to invoke PluginInstanceChild's UpdateWindow from the content process for a particular PluginInstanceParent.
Attachment #8527348 - Flags: review?(aklotz)
(Assignee)

Comment 13

5 years ago
- 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+
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 1104999
No longer depends on: 1091766
(Assignee)

Comment 17

5 years ago
Posted patch wip (obsolete) — Splinter Review
Attachment #8527344 - Attachment is obsolete: true
Attachment #8527347 - Attachment is obsolete: true
Attachment #8527350 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Comment 19

5 years ago
Posted 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
(Assignee)

Updated

5 years ago
Depends on: 1106147
(Assignee)

Updated

5 years ago
Blocks: 1106243
(Assignee)

Updated

4 years ago
No longer depends on: 1106147
Duplicate of this bug: 1106147
(Assignee)

Updated

4 years ago
Attachment #8529290 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Posted 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.
(Assignee)

Comment 22

4 years ago
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
(Assignee)

Comment 23

4 years ago
Posted patch wip (obsolete) — Splinter Review
(Assignee)

Updated

4 years ago
Attachment #8545508 - Attachment is patch: true
(Assignee)

Comment 24

4 years ago
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.
(Assignee)

Comment 25

4 years ago
Posted patch wip (obsolete) — Splinter Review
Attachment #8545508 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Attachment #8546126 - Attachment is obsolete: true
(Assignee)

Comment 27

4 years ago
Attachment #8546163 - Flags: review?(aklotz)
(Assignee)

Comment 28

4 years ago
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+
(Assignee)

Comment 30

4 years ago
Layout bits which route plugin widget configuration to the compositor vs. trying to applying the config on puppet widget.
(Assignee)

Comment 31

4 years ago
Plumbing to move plugin configuration updates over to the chrome side.
(Assignee)

Comment 32

4 years ago
Posted 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.
(Assignee)

Comment 33

4 years ago
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.
(Assignee)

Comment 34

4 years ago
This is where the get tab offset api comes up.
(Assignee)

Comment 35

4 years ago
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.
(Assignee)

Comment 36

4 years ago
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.
(Assignee)

Comment 37

4 years ago
Ripping out the main thread apis we currently use to update chrome side plugin widgets.
(Assignee)

Comment 38

4 years ago
Add a bunch of assertion checks for debug builds to this code.
(Assignee)

Comment 39

4 years ago
(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.
(Assignee)

Comment 40

4 years ago
Updating the teardown logic for PPluginWidget.
(Assignee)

Comment 41

4 years ago
Posted patch pt14 - UpdateWindow routing (obsolete) — Splinter Review
This is the other side of comment 28.
(Assignee)

Comment 42

4 years ago
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
(Assignee)

Comment 43

4 years ago
Posted patch pt14 - UpdateWindow routing (obsolete) — Splinter Review
Went ahead and combined those two.
Attachment #8546261 - Attachment is obsolete: true
(Assignee)

Comment 44

4 years ago
I also have some gtk specific changes and some nits to post. I need to test on gtk fiorst, will post these tomorrow.
(Assignee)

Comment 45

4 years ago
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)
(Assignee)

Comment 48

4 years ago
(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.
(Assignee)

Comment 49

4 years ago
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
(Assignee)

Comment 50

4 years ago
Posted file test case (obsolete) —
(Assignee)

Comment 51

4 years ago
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.
(Assignee)

Comment 52

4 years ago
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)
(Assignee)

Updated

4 years ago
Attachment #8546238 - Attachment is obsolete: true
(Assignee)

Comment 53

4 years ago
Posted 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?
(Assignee)

Comment 54

4 years ago
Posted patch wip (obsolete) — Splinter Review
Attachment #8547658 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8548197 - Attachment is patch: true
(Assignee)

Comment 55

4 years ago
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.
(Assignee)

Comment 56

4 years ago
Attachment #8546241 - Attachment is obsolete: true
Attachment #8548197 - Attachment is obsolete: true
(Assignee)

Comment 57

4 years ago
Attachment #8546248 - Attachment is obsolete: true
(Assignee)

Comment 58

4 years ago
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.
(Assignee)

Comment 62

4 years ago
(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.
(Assignee)

Comment 64

4 years ago
(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)
(Assignee)

Comment 65

4 years ago
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.
(Assignee)

Comment 67

4 years ago
> > 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.
(Assignee)

Comment 68

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

http://pastebin.mozilla.org/8223672
(Assignee)

Comment 69

4 years ago
(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.
(Assignee)

Comment 70

4 years ago
(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.
(Assignee)

Comment 71

4 years ago
Posted patch pt9.5 - layer clip helper (obsolete) — Splinter Review
Attachment #8548214 - Attachment is obsolete: true
Attachment #8549895 - Flags: review?(roc)
(Assignee)

Comment 72

4 years ago
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)
(Assignee)

Comment 73

4 years ago
Posted patch pt9.5 - layer clip helper (obsolete) — Splinter Review
Attachment #8549896 - Flags: review?(roc)
(Assignee)

Comment 74

4 years ago
(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-
(Assignee)

Comment 77

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

Comment 78

4 years ago
> 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.
(Assignee)

Comment 79

4 years ago
Attachment #8546231 - Attachment is obsolete: true
Attachment #8551784 - Flags: review?(roc)
(Assignee)

Comment 80

4 years ago
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)
(Assignee)

Comment 81

4 years ago
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)
(Assignee)

Updated

4 years ago
Attachment #8546240 - Attachment is obsolete: true
(Assignee)

Comment 82

4 years ago
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)
(Assignee)

Updated

4 years ago
Attachment #8551806 - Attachment description: pt8 - widget config → pt7 - widget config
(Assignee)

Comment 83

4 years ago
Posted 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)
(Assignee)

Updated

4 years ago
Attachment #8546245 - Attachment description: pt9 - sandboxed flash helper → pt3.5 - sandboxed flash helper
(Assignee)

Comment 84

4 years ago
The code that leverages 'layer clip helper' and updates plugins.
Attachment #8549898 - Attachment is obsolete: true
Attachment #8551811 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #8551811 - Attachment description: pt10 - parent side compositor changes → pt9 - parent side compositor changes
(Assignee)

Comment 85

4 years ago
100% obsolete code removal.
Attachment #8546249 - Attachment is obsolete: true
Attachment #8551812 - Flags: review?(aklotz)
(Assignee)

Comment 86

4 years ago
Adding some assertions to protect against unexpected failures in PluginWidgetParent.
Attachment #8546250 - Attachment is obsolete: true
Attachment #8551814 - Flags: review?(aklotz)
(Assignee)

Comment 87

4 years ago
Updating PluginWidget tear down. The commenting added here should explain the reasoning pretty well.
Attachment #8551815 - Flags: review?(aklotz)
(Assignee)

Comment 88

4 years ago
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)
(Assignee)

Comment 89

4 years ago
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)
(Assignee)

Updated

4 years ago
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-
(Assignee)

Comment 94

4 years ago
Posted patch pt8 - layer clip helper (obsolete) — Splinter Review
Attachment #8551808 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8554828 - Flags: review?(roc)
(Assignee)

Comment 95

4 years ago
Posted 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)
(Assignee)

Updated

4 years ago
Attachment #8554831 - Attachment is obsolete: true
Attachment #8554831 - Flags: review?(roc)
(Assignee)

Comment 96

4 years ago
(Assignee)

Comment 97

4 years ago
Attachment #8551811 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
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+
(Assignee)

Updated

4 years ago
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-
(Assignee)

Comment 100

4 years ago
(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.
(Assignee)

Comment 101

4 years ago
(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.
(Assignee)

Comment 102

4 years ago
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.
(Assignee)

Comment 103

4 years ago
(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.
(Assignee)

Comment 104

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

Comment 105

4 years ago
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.
(Assignee)

Comment 106

4 years ago
pastebin of the layer dumps if anyone is curious - http://pastebin.mozilla.org/8383681
(Assignee)

Comment 107

4 years ago
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
(Assignee)

Comment 108

4 years ago
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+
(Assignee)

Comment 111

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8556117 - Flags: review?(roc)
(Assignee)

Comment 112

4 years ago
Posted patch rollup (obsolete) — Splinter Review
(Assignee)

Comment 113

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

Comment 114

4 years ago
Posted 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

Updated

4 years ago
Depends on: 1127595
(Assignee)

Updated

4 years ago
Blocks: 1127794
(Assignee)

Updated

4 years ago
No longer blocks: 1106243
(Assignee)

Updated

4 years ago
Depends on: 1128079
(Assignee)

Updated

4 years ago
No longer depends on: 1127595

Updated

4 years ago
Depends on: 1152049

Updated

4 years ago
Depends on: 1152080
(Assignee)

Updated

4 years ago
No longer depends on: 1152049

Updated

4 years ago
Depends on: 1212484

Updated

4 years ago
No longer depends on: 1212484

Updated

4 years ago
Depends on: 1212484

Updated

4 years ago
Depends on: 1152049
(Assignee)

Updated

4 years ago
No longer depends on: 1152049
You need to log in before you can comment on or make changes to this bug.