Closed
Bug 1110540
Opened 8 years ago
Closed 8 years ago
Add a generic GeckoContentController for chrome
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(3 files)
17.90 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
10.82 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
Assignee | |
Comment 1•8 years ago
|
||
This patch factors out APZCTM setup code into nsBaseWidget so we don't duplicate it everywhere.
Attachment #8535324 -
Flags: review?(bugmail.mozilla)
![]() |
Assignee | |
Comment 2•8 years ago
|
||
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)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8535324 -
Attachment description: bug1110540-part1.patch → part 1, refactor apzctm setup
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Comment 5•8 years ago
|
||
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.
![]() |
Assignee | |
Comment 6•8 years ago
|
||
fixup android
Attachment #8536278 -
Flags: review?(bugmail.mozilla)
![]() |
Assignee | |
Comment 7•8 years ago
|
||
Tested everywhere but Android - will try that soon and then land.
Comment 8•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 9•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3fd3bc9d9cd2 https://hg.mozilla.org/mozilla-central/rev/71b13c6e679a https://hg.mozilla.org/mozilla-central/rev/70e6bd31ce14
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•