Closed Bug 1110540 Opened 5 years ago Closed 5 years ago

Add a generic GeckoContentController for chrome

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(3 files)

No description provided.
This patch factors out APZCTM setup code into nsBaseWidget so we don't duplicate it everywhere.
Attachment #8535324 - Flags: review?(bugmail.mozilla)
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.
Attachment #8535326 - Flags: review?(bugmail.mozilla)
Attachment #8535324 - Attachment description: bug1110540-part1.patch → part 1, refactor apzctm setup
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
Attachment #8535324 - Flags: review?(bugmail.mozilla) → review+
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)
Attachment #8535326 - Flags: review?(bugmail.mozilla) → review+
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.
fixup android
Attachment #8536278 - Flags: review?(bugmail.mozilla)
Tested everywhere but Android - will try that soon and then land.
Comment on attachment 8536278 [details] [diff] [review]
bug1110540-part3.patch

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

LGTM!
Attachment #8536278 - Flags: review?(bugmail.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.