Closed Bug 1272472 Opened 4 years ago Closed 4 years ago

Isolate widget access to CompositorBridgeParent

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(6 files)

Parts of Gecko assumse that CompositorBridgeParent lives in the same process. Worst offenders look like nsBaseWidget, TabParent, and ContentParent. We'll have to build an abstraction layer that lets this code work either in-process or out.

Initially this layer will simply redirect calls in-process, but on the GPU process branch we can start building the actual layers needed.
Just some cleanup, this method is all but duplicated in Android/Gonk for one bool flip. Might as well remove it and inline it into CreateCompositor.
Attachment #8751996 - Flags: review?(bugmail.mozilla)
Replace direct access to CompositorBridgeParent with IPC calls.
Attachment #8751997 - Flags: review?(nical.bugzilla)
Attachment #8751996 - Flags: review?(bugmail.mozilla) → review+
Going to do DOM access in a separate bug to keep this one small.
Summary: Isolate widget/DOM access to CompositorBridgeParent → Isolate widget access to CompositorBridgeParent
This introduces a new CompositorSession class, which now owns the CompositorBridgeParent and CompositorBridgeChild pointers in a widget. Widgets can still access the Child as normal, but the Parent pointer will not exist in out-of-process mode. Widgets that use it wthout auditing will not support out-of-process compositing.
Attachment #8753096 - Flags: review?(matt.woodrow)
We're not doing GPU process for Android initially, so for now we can just bypass the CompositorSession to access the parent. It looks like most of these calls could go through CompositorBridgeChild in the future.
Attachment #8753098 - Flags: review?(bugmail.mozilla)
Attachment #8753098 - Attachment description: part 5, fix android build → part 4.1, fix android build
Attachment #8753096 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8753098 [details] [diff] [review]
part 4.1, fix android build

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

I don't really understand the name "CompositorSession" - in particular the "session" part. Are there going to be multiple "composition sessions" during the browser's lifetime?

::: widget/android/nsWindow.cpp
@@ +1071,5 @@
>  
>      void SyncResumeCompositor()
>      {
> +        if (RefPtr<CompositorBridgeParent> bridge = window.GetCompositorBridgeParent()) {
> +          if (bridge->ScheduleResumeOnCompositorThread()) {

indentation in this file is 4 spaces. I don't care too much, we should be moving towards 2 space indentation anyway. Ditto the code below.

@@ +3512,5 @@
>  nsWindow::ComputeRenderIntegrity()
>  {
> +    if (gGeckoViewWindow) {
> +      if (RefPtr<CompositorBridgeParent> bridge = gGeckoViewWindow->GetCompositorBridgeParent()) {
> +        return bridge->ComputeRenderIntegrity();

I'll file a bug to get rid of this function, I don't think we use it for anything anymore.
Attachment #8753098 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Comment on attachment 8753098 [details] [diff] [review]
> part 4.1, fix android build
> 
> Review of attachment 8753098 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really understand the name "CompositorSession" - in particular the
> "session" part. Are there going to be multiple "composition sessions" during
> the browser's lifetime?

Yeah, indeed. Each widget will have its own session, and the session might be recreated if something bad happens (like the process dies or a device resets). Other name suggests are welcome, it was like the least terrible candidate (others were "CompositorProxy" and "CompositorBroker").
Attachment #8751997 - Flags: review?(nical.bugzilla) → review+
Attachment #8752078 - Flags: review?(jmathies) → review+
(In reply to David Anderson [:dvander] from comment #9)
> Yeah, indeed. Each widget will have its own session, and the session might
> be recreated if something bad happens (like the process dies or a device
> resets). Other name suggests are welcome, it was like the least terrible
> candidate (others were "CompositorProxy" and "CompositorBroker").

Yeah I can't come up with anything better either. CompositorSession is ok.
Comment on attachment 8753100 [details] [diff] [review]
part 4.2, fix gonk build

Thanks David!
Attachment #8753100 - Flags: review?(anygregor) → review+
You need to log in before you can comment on or make changes to this bug.