1110540 - Add a generic GeckoContentController for chrome

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, refactor apzctm setup

This patch factors out APZCTM setup code into nsBaseWidget so we don't duplicate it everywhere.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, ChromeProcessController

This takes the content controller out of nsChildView.mm and puts it in a separate file. nsBaseWidget now creates it by default if no controller override was provided.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8535324 [details] [diff] [review]
part 1, refactor apzctm setup

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

This is really nice, thanks! Some nits/bugs below but r+ with those addressed. I'd like to make sure we do a sanity test on all of these platforms before landing though - I can do OS X, Metro and B2G if you don't have those devices, let me know.

::: widget/cocoa/nsChildView.mm
@@ +2203,5 @@
> +nsChildView::ConfigureAPZCTreeManager()
> +{
> +  if (gNumberOfWidgetsNeedingEventThread == 0) {
> +    [EventThreadRunner start];
> +   }

nit: indent

Also the order here probably doesn't matter, but the old code configured the APZCTM before starting this event thread runner.

::: widget/gonk/nsWindow.cpp
@@ +618,5 @@
>  
> +already_AddRefed<GeckoContentController>
> +nsWindow::CreateRootContentController()
> +{
> +    nsRefPtr<ParentProcessController> controller = NewParentProcessController();

s/NewP/new P/

::: widget/nsBaseWidget.cpp
@@ +921,5 @@
> +void nsBaseWidget::ConfigureAPZCTreeManager()
> +{
> +  uint64_t rootLayerTreeId = mCompositorParent->RootLayerTreeId();
> +  mAPZC = CompositorParent::GetAPZCTreeManager(rootLayerTreeId);
> +  mAPZC->SetDPI(GetDPI());

Add a MOZ_ASSERT(mAPZC) before the call to SetDPI to better document assumptions here.

::: widget/nsBaseWidget.h
@@ +141,5 @@
>    virtual CompositorParent* NewCompositorParent(int aSurfaceWidth, int aSurfaceHeight);
>    virtual void            CreateCompositor();
>    virtual void            CreateCompositor(int aWidth, int aHeight);
> +  virtual void            ConfigureAPZCTreeManager();
> +  virtual already_AddRefed<GeckoContentController> CreateRootContentController();

These should probably be protected rather than public. The CreateCompositor stuff should probably be protected too but no need to change that in this bug if you don't want to since it's pre-existing.

::: widget/windows/winrt/MetroWidget.cpp
@@ +1037,2 @@
>  {
> +  nsresult rv;

This is missing a call to the base widget's ConfigureAPZCTreeManager

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8535326 [details] [diff] [review]
part 2, ChromeProcessController

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

LGTM

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/layers/CompositorParent.h"
> +#include "mozilla/layers/ChromeProcessController.h"

#include ChromeProcessController.h first (I know a lot of files don't do this, but it's in the style guide)

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Btw, feel free to update the widget/android code as well in part 1. The setup is a little uglier but I don't mind if it breaks a bit during cleanup since that's not fully working right now anyway.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1110540-part3.patch

fixup android

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Tested everywhere but Android - will try that soon and then land.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8536278 [details] [diff] [review]
bug1110540-part3.patch

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

LGTM!

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd3bc9d9cd2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71b13c6e679a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/70e6bd31ce14

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fd3bc9d9cd2
https://hg.mozilla.org/mozilla-central/rev/71b13c6e679a
https://hg.mozilla.org/mozilla-central/rev/70e6bd31ce14