New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 623716 link

Starred by 4 users
Status: Fixed
Owner:
ccameron@chromium.org
Closed: Jul 2016
Cc:
enne@chromium.org
█ shrike@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Mac: Clean up ui::Compositor<->RenderWidgetHostViewMac interface

Project Member Reported by ccameron@chromium.org, Jun 27 2016 
This has grown organically and has been causing trouble with unified begin frame, and less recently cause all sorts of pain with thumbnailing.

Phase 1 is to clean up this relationship.

Phase 2 may or may not be a good idea, but is:

We should also consider having a non-1:1 relationship between gfx::AcceleratedWidget and ui::AcceleratedWidgetMac.

Instead, we should consider having a 1:1 relationship between RenderWidgetHostViewMac and ui::AcceleratedWidgetMac, and have the gfx::AcceleratedWidgetMac <-> ui::AcceleratedWidget link match the RenderWidgetHostViewMac <-> ui::Compositor link. To make this work we would want to ensure that we get a new CAContext when we recycled compositors.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 28 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e8481c2e211439771d0634043d2addf9fe7b4ec

commit 9e8481c2e211439771d0634043d2addf9fe7b4ec
Author: ccameron <ccameron@chromium.org>
Date: Tue Jun 28 21:04:33 2016

Clean up the RWHVMac to ui::Compositor interface

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

BUG= 623716 

Review-Url: https://codereview.chromium.org/2093113003
Cr-Commit-Position: refs/heads/master@{#402541}

[modify] https://crrev.com/9e8481c2e211439771d0634043d2addf9fe7b4ec/chrome/browser/thumbnails/thumbnail_tab_helper.cc
[modify] https://crrev.com/9e8481c2e211439771d0634043d2addf9fe7b4ec/content/browser/renderer_host/browser_compositor_view_mac.h
[modify] https://crrev.com/9e8481c2e211439771d0634043d2addf9fe7b4ec/content/browser/renderer_host/browser_compositor_view_mac.mm
[modify] https://crrev.com/9e8481c2e211439771d0634043d2addf9fe7b4ec/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/9e8481c2e211439771d0634043d2addf9fe7b4ec/content/browser/renderer_host/render_widget_host_view_mac.mm

Comment 3 by engedy@chromium.org, Jul 8 2016 

Christopher, are the calls to Increment/DecrementCaptureCount going to be gone for good once this refactoring is complete?

If so, then this work will also fix  Issue 605412 .  Otherwise, we should make sure all code paths call Decrement after a call to Increment, otherwise the tab gets stuck in capture mode, which has the side effect of preventing real full-screen (see the bug, which I have assigned to you for the time being).

In any case, we should probably merge the fix back to M52.

Comment 4 by ccameron@chromium.org, Jul 8 2016 

I don't think this can go back to M52 easily -- that refactor touched lots of things, and I had to clean up a handful of follow-on crashes (e.g, issue 625610).

Is M53 okay?
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 8 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4309d664520802fe54ba603622b1222741e427aa

commit 4309d664520802fe54ba603622b1222741e427aa
Author: ccameron <ccameron@chromium.org>
Date: Fri Jul 08 19:51:03 2016

Mac: Clean up visibility state tracking

Trying to fix this has been a source of hung frames.

This makes the DelegatedFrameHostClient's IsVisible callback return
the variable that tracks whether or not the DelegatedFrameHost's owner
has set it to be visible.

BUG= 623716 

Review-Url: https://codereview.chromium.org/2125143004
Cr-Commit-Position: refs/heads/master@{#404469}

[modify] https://crrev.com/4309d664520802fe54ba603622b1222741e427aa/content/browser/renderer_host/browser_compositor_view_mac.h
[modify] https://crrev.com/4309d664520802fe54ba603622b1222741e427aa/content/browser/renderer_host/browser_compositor_view_mac.mm
[modify] https://crrev.com/4309d664520802fe54ba603622b1222741e427aa/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/4309d664520802fe54ba603622b1222741e427aa/content/browser/renderer_host/render_widget_host_view_mac.mm
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 18 2016

Labels: merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd

commit 1dee73bd817af4a1b6ca0bfc84f37a6d276787dd
Author: Christopher Cameron <ccameron@chromium.org>
Date: Mon Jul 18 19:04:05 2016

Mac: Clean up visibility state tracking

Trying to fix this has been a source of hung frames.

This makes the DelegatedFrameHostClient's IsVisible callback return
the variable that tracks whether or not the DelegatedFrameHost's owner
has set it to be visible.

BUG= 623716 

Review-Url: https://codereview.chromium.org/2125143004
Cr-Commit-Position: refs/heads/master@{#404469}
(cherry picked from commit 4309d664520802fe54ba603622b1222741e427aa)

Mac: Further clean up RWHVMac<->DelegatedFrameHost

This removes the strangeness whereby the BrowserCompositorMac would
own the DelegatedFrameHost, but its owner, the RenderWidgetHostViewMac,
would be the DelegatedFrameHostClient.

BUG=625610

Review-Url: https://codereview.chromium.org/2123313002
Cr-Commit-Position: refs/heads/master@{#404006}
(cherry picked from commit 69170b87c992e07df18559ba1a50fb9b41a5988b)

Remove more GpuMemoryBufferId plumbing

This is not used anymore.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review-Url: https://codereview.chromium.org/2129523003
Cr-Commit-Position: refs/heads/master@{#404001}
(cherry picked from commit 1b0250ffc474bcd610e3109d33ece7fa4e87eeca)

Review URL: https://codereview.chromium.org/2156333002 .

Cr-Commit-Position: refs/branch-heads/2785@{#192}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/resources/resource_provider.cc
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/resources/resource_provider.h
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/resources/resource_provider_unittest.cc
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/surfaces/surface.cc
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/surfaces/surface.h
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/surfaces/surface_aggregator.cc
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/surfaces/surface_factory.cc
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/cc/surfaces/surface_factory.h
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/browser_compositor_view_mac.h
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/browser_compositor_view_mac.mm
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/delegated_frame_host.h
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/1dee73bd817af4a1b6ca0bfc84f37a6d276787dd/content/browser/renderer_host/render_widget_host_view_mac.mm

Comment 7 by ccameron@chromium.org, Jul 18 2016

Status: Fixed (was: Started)
Fixed -- the above merge should be under  issue 605412
►

Sign in to add a comment