Closed Bug 1281575 Opened 8 years ago Closed 8 years ago

Extract interface of APZCTreeManager for moving to a GPU process

Categories

(Core :: Graphics, defect)

Unspecified
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(2 files, 16 obsolete files)

89.81 KB, patch
Details | Diff | Splinter Review
61.16 KB, patch
Details | Diff | Splinter Review
To be able to move APZ to a GPU process we need an minimal interface for APZCTreeManager that we could swap for a remote implementation.
Keywords: feature
Whiteboard: [gfx-noted]
Assignee: nobody → rhunt
OS: Unspecified → All
The first step is to extract the interface of APZCTreeManager as it is used by nsBaseWidget and other classes into an interface. The original APZCTreeManager then inherits from this interface, and references in nsBaseWidget are replaced with the interface.
Attachment #8764356 - Attachment is obsolete: true
Step 2, create a protocol for the APZCTreeManager interface. This part is tough. The two methods that give us trouble are the ReceiveInputEvent's, because WidgetInputEvent, and InputData need to be marshaled across process. This is possible to do, and this patch does this.

Another idea would be to refactor ReceiveInputEvent into smaller functions that don't need a full WidgetInputEvent. We'd be moving some logic from APZCTreeManager to the places that call APZCTreeManager, so that Widgets don't need to be sent across process.

One component missing from this patch is in the APZCTreeManager code. There are a lot of asserts in ReceiveInputEvent to make sure that it is running on the main thread or controller thread. That won't work when APZCTreeManager is out of process. I'm not sure what should be done about this.
Flags: needinfo?(bugmail.mozilla)
This next patch is just a test to run the protocol in-process underneath PCompostitorBridge. It can compile and run, as long as you remove all the assert-on-main-threads from APZCTreeManager. Which is not a solution.
I looked over the patch briefly, and the approach seems pretty good. I think we can simplify it some by not having to send the WidgetInputEvent instances over the IPDL bridge. In APZCTreeManager, the ReceiveInputEvent function that takes a WidgetInputEvent pretty much always converts it to a InputData version for processing. There's only one branch that doesn't do this, when we end up in the function at [1].

I think in general we should be able to draw a line between the code that uses WidgetInputEvent, and code that uses InputData, and have it so that the WidgetInputEvent code remains in the main process and the InputData code moves over to the GPU process. We might have to move a few things around to make this separation cleaner, but I think in general it should work.

As for the thread assertions, we need to define what will be the "controller thread" on the GPU process. From what I remember there's only going to be one thread in the GPU process, so I guess that would be the controller thread? We can abstract away the is-main-thread checks behind a wrapper function in APZThreadUtils, and in the case of the GPU process make those return true if we're on the (only) GPU process thread. Does that sound like it would work? Also correct me if I'm wrong about there only being one thread in the GPU process.

[1] http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/gfx/layers/apz/src/APZCTreeManager.cpp#1109
Flags: needinfo?(bugmail.mozilla)
Okay, looking at the code I think that's doable. It would certainly cut down on the amount of similar methods on the protocol. One sticking point is UpdateWheelTransaction which is used in ReceiveInputEvents, and can't be in the main process because it uses mInputQueue. I'm thinking of exposing it along with ProcessEvent on the protocol, but not passing a full WidgetInputEvent. Instead we'd just pass the relevant information which is a ref point and the class.

Regarding threads, I talked with David and it sounds like the GPU process will have a main thread and a compositor thread (along with an IO thread for ipc), and APZ will most likely live on the main thread. So that would be the controller thread?
(In reply to Ryan Hunt [:rhunt] from comment #5)
> I'm thinking of exposing it
> along with ProcessEvent on the protocol, but not passing a full
> WidgetInputEvent. Instead we'd just pass the relevant information which is a
> ref point and the class.

That sounds reasonable.

> Regarding threads, I talked with David and it sounds like the GPU process
> will have a main thread and a compositor thread (along with an IO thread for
> ipc), and APZ will most likely live on the main thread. So that would be the
> controller thread?

Yeah, I guess so. I mean we could define the controller thread to be the compositor thread, if we wanted. The only requirement is that (a) some thread is designated as "the controller thread" and (b) various functions (ReceiveInputEvent, ContentReceivedInputBlock, SetTargetAPZC, etc.) on APZCTreeManager need to be called on that thread. These functions are going to get remoted through IPDL anyway, so they should end up on the same thread in the GPU process, and if we wanted to make that the compositor thread we could, but I think it would be simpler to make it the main thread. It would also avoid unnecessary contention where input events get blocked waiting for a composition to happen.
The first step is to create an interface of common methods used by nsBaseWidget and other classes, along with refactoring existing code to use this interface. Something needed for later, is to lift up the ReceiveEvent(WidgetInputEvent) method from APZCTreeManager to the new interface. This helps us later to only remote InputData across IPC. It makes this patch a little heavier, sorry!
Attachment #8765754 - Attachment is obsolete: true
Attachment #8765769 - Attachment is obsolete: true
Attachment #8765773 - Attachment is obsolete: true
Attachment #8768462 - Flags: review?(bugmail)
The second step is to create an IPDL protocol to remote APZ to the compositor. A lot of this is glue code, but there's also a chunk of ParamsTraits that had to be written for InputData.
Attachment #8768464 - Flags: review?(dvander)
For reference, this code switches out the normal APZCTreeManager for a remote one.
Comment on attachment 8768464 [details] [diff] [review]
Step2, Create IPDL protocol for APZCTreeManager

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

Nice - looks good at a glance, kats should have the final review here.

My only concern (a problem we've had with ParamTraits before) is that it's easy to get C++ classes out of sync with the serialization code. So we probably want scary comments in these classes that nsGUIEventIPC.h has to be updated.
Attachment #8768464 - Flags: review?(dvander)
Attachment #8768464 - Flags: review?(bugmail)
Attachment #8768464 - Flags: feedback+
Comment on attachment 8768462 [details] [diff] [review]
Step 1, Extract interface of APZCTreeManager

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

This looks pretty good overall. No major issues, just some minor nits and a few easily-addressable comments below. I'd like to see an updated version of the patch before giving a final r+ though, mostly so I can do another pass through it. Please make sure you generate the patch using -U8 in your mercurial settings so that there's a bit more context around the diff, rather than the 3 lines you have here.

Also I'm flagging botond for feedback on this patch, mostly so that he's aware of what's going on as it's a relatively large change to the APZ code.

::: gfx/ipc/CompositorSession.cpp
@@ +7,5 @@
>  #include "mozilla/layers/CompositorBridgeChild.h"
>  #include "mozilla/layers/CompositorBridgeParent.h"
>  #include "mozilla/widget/PlatformWidgetTypes.h"
> +#include "mozilla/layers/APZCTreeManager.h"
> +#include "mozilla/layers/IAPZCTreeManager.h"

Do you need to include APZCTreeManager.h here? Looks like you converted all the uses of that to IAPZCTreeManager.

::: gfx/ipc/IAPZCTreeManager.cpp
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=99: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/IAPZCTreeManager.h"

add a blank line between the boilerplate and the #include

@@ +17,5 @@
> +
> +static bool
> +WillHandleMouseEvent(const WidgetMouseEventBase& aEvent)
> +{
> +return aEvent.mMessage == eMouseMove ||

indentation

@@ +61,5 @@
> +
> +      if (WillHandleMouseEvent(mouseEvent)) {
> +
> +        // Note, we call this before having transformed the reference point.
> +        UpdateWheelTransactionMouse(mouseEvent.mRefPoint, mouseEvent.mReason);

Might as well move this up before the if (WillHandleMouseEvent) condition, since it happens even in the "else" case below. I think it would also be better to do mouseEvent.IsReal() check here, and skip calling UpdateWheelTransactionMouse entirely if it's false. That way you can skip the extra "mouseEvent.mReason" parameter, and more importantly, avoid an unnecessary IPC roundtrip in the case that the event is non-real. Also, see my comment in IAPZCTreeManager.h - I think we can leave the UpdateWheelTransaction function as a single function rather than splitting into a Mouse and Default version.

::: gfx/ipc/IAPZCTreeManager.h
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=99: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */
> +#ifndef include_mozilla_gfx_ipc_IAPZCTreeManager_h

add a blank line between the boilerplate and the #ifndef

@@ +13,5 @@
> +#include "Units.h"                      // for CSSPoint, CSSRect, etc
> +#include "mozilla/layers/APZUtils.h"    // for HitTestResult
> +#include "mozilla/MouseEvents.h"        // for mozilla::WidgetMouseEvent::Reason
> +#include "nsTArrayForwardDeclare.h"     // for nsTArray, nsTArray_Impl, etc
> +#include "nsISupportsImpl.h"            // for MOZ_COUNT_CTOR, etc

Sort this block of includes alphabetically

@@ +38,5 @@
> +};
> +
> +class AsyncDragMetrics;
> +
> +// class AsyncDragMetrics;

Remove this line

@@ +112,5 @@
> +  // Methods for dispatching WidgetInputEvents to InputData
> +
> +  virtual void TransformEventRefPoint(
> +      LayoutDeviceIntPoint* aRefPoint,
> +      ScrollableLayerGuid* aOutTargetGuid) = 0;

Can we make these methods protected instead of public? They don't need to be exposed to consumers of this interface. Also the comment "Methods for dispatching WidgetInputEvents to InputData" is a little unclear. Please instead say "Methods to help process WidgetInputEvents (or manage conversion to/from InputData)"

@@ +119,5 @@
> +      LayoutDeviceIntPoint aRefPoint,
> +      WidgetMouseEvent::Reason aEventMouseReason) = 0;
> +
> +  virtual void UpdateWheelTransactionDefault(
> +      EventMessage aEventMessage) = 0;

I'm not sure there's much value in splitting this function into two. I think we can just have a single function

virtual void UpdateWheelTransaction(
    EventMessage aEventMessage,
    LayoutDeviceIntPoint aRefPoint) = 0;

The APZCTreeManager.cpp implementation would do basically what it does in current m-c, minus the IsReal check. That IsReal check can be put into the callsites of what you have now as UpdateWheelTransactionMouse in IAPZCTreeManager (see my other comment in IAPZCTreeManager.cpp). For the current "default" (i.e. non-mouse) callsites, you can still pass in the event's refPoint, it's just going to be ignored on the other side. Does that make sense?

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +265,4 @@
>      const ParentLayerPoint& aVelocity);
>  
>    /**
> +   * Sets the dpi used value used by all AsyncPanZoomControllers.

remove spurious "used" added here

::: gfx/layers/apz/util/APZThreadUtils.cpp
@@ +43,2 @@
>  
> +  // MOZ_ASSERT(sControllerThread == MessageLoop::current());

In this patch you shouldn't need to touch this, right? Everything that was on the controller thread is still on the controller thread.
Attachment #8768462 - Flags: review?(bugmail) → feedback?(botond)
Comment on attachment 8768464 [details] [diff] [review]
Step2, Create IPDL protocol for APZCTreeManager

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

Fairly straightforward for the most part. But the "Client" vs "Parent" naming is confusing. Renaming "Client" to "Child" would also be confusing, because that's the one that lives in what we call the parent process, if I'm understanding correctly. Sigh.

How about something like APZCTreeManagerMainProcess and APZCTreeManagerGPUProcess?
Or APZCTreeManagerWrapper and APZCTreeManagerImpl?

I don't really have any good ideas here, naming is hard.

::: gfx/ipc/APZCTreeManagerClient.cpp
@@ +6,5 @@
> +
> +#include "mozilla/layers/APZCTreeManagerClient.h"
> +#include "mozilla/EventStateManager.h"  // for WheelPrefs
> +
> +#include "InputData.h"

Move the blank line to before the EventStateManager.h include

@@ +12,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +  nsEventStatus
> +  APZCTreeManagerClient::ReceiveInputEvent(

Unindent everything inside the namespace block by 2 columns

::: gfx/ipc/APZCTreeManagerClient.h
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=99: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */
> +#ifndef include_mozilla_gfx_ipc_APZCTreeManagerClient_h

nit: add blank line before #ifndef

@@ +21,5 @@
> +  {
> +  }
> +
> +  virtual nsEventStatus
> +  ReceiveInputEvent(

style guide says to omit "virtual" if you already have "override" as the latter implies the former. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

::: gfx/ipc/APZCTreeManagerParent.cpp
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=99: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/APZCTreeManagerParent.h"

nit: add blank line before #include

@@ +6,5 @@
> +#include "mozilla/layers/APZCTreeManagerParent.h"
> +
> +#include "mozilla/layers/APZCTreeManager.h"
> +#include "mozilla/layers/APZThreadUtils.h"
> +#include "base/message_loop.h"

move this message_loop.h include up to before APZCTreeManager.h so it's in alphabetical order.

@@ +31,5 @@
> +  nsEventStatus res = mTreeManager->ReceiveInputEvent(
> +    event,
> +    aOutTargetGuid,
> +    aOutInputBlockId
> +    );

Move the closing-paren to the previous line. Ditto in all the functions below.

@@ +161,5 @@
> +
> +bool
> +APZCTreeManagerParent::RecvUpdateWheelTransactionDefault(
> +        const EventMessage& aEventMessage
> +        )

Move parenthesis. But also hopefully this function will get merged into the previous one

::: gfx/ipc/APZCTreeManagerParent.h
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=99: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */
> +#ifndef include_mozilla_gfx_ipc_APZCTreeManagerParent_h

nit: add blank line before #ifndef

::: widget/BasicEvents.h
@@ +616,4 @@
>    WidgetGUIEvent()
>    {
>    }
> +  

nit: trailing whitespace

@@ +625,4 @@
>      , mWidget(aWidget)
>    {
>    }
> +  

nit: trailing whitespace

@@ +735,5 @@
>    {
>    }
>  
> +  WidgetInputEvent()
> +    : mModifiers(0)

Are these changes still needed? It's not clear to me why you made this public. (Same for the changes in MouseEvents.h)

::: widget/InputData.h
@@ +25,5 @@
>  class Touch;
>  } // namespace dom
>  
> +typedef int InputTypeBase;
> +enum InputType : InputTypeBase

Instead of doing this, can we use the ContiguousEnumSerializer stuff? There's a bunch of examples in gfx/ipc/GfxMessageUtils.h. If necessary we can convert these enums to enum classes and fix the styling (something I'd like to do regardless, like in bug 1283556).

Ditto for all the other enum changes you made below.

@@ +91,5 @@
>    }
>  
> +  InputData()
> +    : mTime(0),
> +      modifiers(0)

I'd prefer to make this protected or private, and then declare the ParamTraits<InputData> as a friend. That way random pieces of code can't start creating improperly-initialized InputData instances, but the deserialization code will still be allowed to. We do that in a few other places, for example the FrameMetrics struct [1].

Ditto for the other constructor changes you made below.

[1] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/gfx/layers/FrameMetrics.h#45

::: widget/nsGUIEventIPC.h
@@ +219,5 @@
>  };
>  
>  template<>
> +struct ParamTraits<mozilla::WidgetMouseEvent::Reason>
> +{

We shouldn't need this if we move the IsReal() check like I described in my previous comment.
Attachment #8768464 - Flags: review?(bugmail) → feedback+
UpdateWheelTransaction[Default,Mouse] are merged together. The two helper functions have also been made protected. The style issues should be all adressed.
Attachment #8768462 - Attachment is obsolete: true
Attachment #8768464 - Attachment is obsolete: true
Attachment #8768465 - Attachment is obsolete: true
Attachment #8768462 - Flags: feedback?(botond)
Attachment #8769323 - Flags: review?(bugmail)
Attachment #8769323 - Flags: feedback?(botond)
Some biggger changes were made from the last patch. I've reworked APZCTreeManagerClient so that it is a PAPZCTreeManagerChild. So now it fits into the IPDL naming scheme, and so I renamed it to APZCTreeManagerChild. It is confusing that the child resides in the parent process, and the parent resides in the sub process. But that's also how PCompositorBridge works, so at least it's consistent now.

I also touched the IPC ParamTraits code so it uses ContiguousEnumSerializers, and there are warnings in the places telling people to make sure to update the serializers. The wording might not be the best, I couldn't find another example to follow. InputData constructors should be appropriately protected now too.
Attachment #8769325 - Flags: review?(bugmail)
This last patch can be used to test the PAPZCTreeManager protocol over PCompositorBridge.
Comment on attachment 8769323 [details] [diff] [review]
Step1-Extract-interface-of-APZCTreeManager-for-moving-to-C.patch

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

Kats, thanks for the heads up. This looks good to me!
Attachment #8769323 - Flags: feedback?(botond) → feedback+
Attachment #8769325 - Flags: review?(bugmail)
Sorry, I was too hasty in getting this patch out and found an issue with my use of ContiguousEnumSerializers. Didn't read that the HighValue is an exclusive bound, so IPC fails intermittently on some tests. I'll try and fix it and test more thoroughly. This just affects the second patch and on.
Comment on attachment 8769323 [details] [diff] [review]
Step1-Extract-interface-of-APZCTreeManager-for-moving-to-C.patch

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

Good stuff. r=me with the comments below addressed. Also make sure to add the bug number to the commit message before landing.

::: gfx/ipc/CompositorSession.cpp
@@ +6,5 @@
>  #include "CompositorSession.h"
>  #include "mozilla/layers/CompositorBridgeChild.h"
>  #include "mozilla/layers/CompositorBridgeParent.h"
>  #include "mozilla/widget/PlatformWidgetTypes.h"
> +#include "mozilla/layers/APZCTreeManager.h" // so we know it's an IAPZCTM

This comment is unclear. What does "it" refer to?

::: gfx/ipc/IAPZCTreeManager.cpp
@@ +60,5 @@
> +
> +      WidgetMouseEvent& mouseEvent = *aEvent.AsMouseEvent();
> +
> +      // Note, we call this before having transformed the reference point.
> +      if (mouseEvent.mReason == WidgetMouseEvent::Reason::eReal) {

nit: use mouseEvent.IsReal()

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1085,5 @@
> +   case eMouseMove:
> +   case eDragOver: {
> +
> +    ScreenIntPoint point =
> +     ViewAs<ScreenPixel>(aRefPoint,

I'd prefer moving this case block back up to the top, to reduce unnecessary changes in the diff and making it harder to navigate the hg blame.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +161,5 @@
>     */
> +  virtual nsEventStatus ReceiveInputEvent(
> +      InputData& aEvent,
> +      ScrollableLayerGuid* aOutTargetGuid,
> +      uint64_t* aOutInputBlockId) override;

According to the [1], "Method declarations must use at most one of the following keywords: virtual, override, or final". i.e., drop the "virtual" from these overridden functions, because it's redundant with the "override" keyword.

@@ +259,3 @@
>     * DPI defaults to 72 if not set using SetDPI() at any point.
>     */
> +  virtual void SetDPI(float aDpiValue) { sDPI = aDpiValue; }

This one should be "override" as well (and then you can drop the "virtual" as noted above)
Attachment #8769323 - Flags: review?(bugmail) → review+
Attachment #8769323 - Attachment is obsolete: true
Attachment #8769325 - Attachment is obsolete: true
Attachment #8769326 - Attachment is obsolete: true
Attachment #8770011 - Flags: review?(bugmail)
Includes fixes for mochitest failures, along with fixes for previous comments.
Attachment #8770012 - Flags: review?(bugmail)
Just for completion, this patch lets you test the IPDL protocol without moving the compositor out of process.
Attachment #8770011 - Flags: review?(bugmail) → review+
Comment on attachment 8770012 [details] [diff] [review]
Step 2, create an IPDL protocol for APZCTreeManager

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

I'm a little unsure if the new files belong in gfx/ipc or gfx/layers/ipc. Jeff should probably decide that. Either way, I think it makes sense to keep the include pathnames and namespaces consistent with the path of the flie. So if they're in gfx/ipc then the namespace should probably mozilla::gfx rather than mozilla::layers and the headers should go into EXPORTS.mozilla.gfx rather than EXPORTS.mozilla.layers (and corresponding the #includes would need updating).

Other than that this looks pretty good. I didn't go through all the nsGUIEventIPC changes to make sure all the fields were present. r+ with comments addressed.

::: gfx/ipc/APZCTreeManagerChild.cpp
@@ +6,5 @@
> +
> +#include "mozilla/layers/APZCTreeManagerChild.h"
> +
> +#include "InputData.h"                  // for InputData
> +#include "mozilla/EventStateManager.h"  // for WheelPrefs

WheelPrefs doesn't appear to be used in this file now

@@ +183,5 @@
> +APZCTreeManagerChild::OnProcessingError(
> +        Result aCode,
> +        const char* aReason)
> +{
> +  MOZ_RELEASE_ASSERT(aCode != MsgDropped);

I'm not familiar with this - dvander should review.

::: gfx/ipc/APZCTreeManagerChild.h
@@ +34,5 @@
> +
> +  void
> +  UpdateWheelTransaction(
> +      LayoutDeviceIntPoint aRefPoint,
> +      EventMessage aEventMessage) override;

Use consistent indenting for the argument list, either 8 or 4 spaces from the start of the method name.

::: gfx/ipc/APZCTreeManagerParent.cpp
@@ +32,5 @@
> +    event,
> +    aOutTargetGuid,
> +    aOutInputBlockId);
> +
> +  *aOutStatus = res;

You don't really need "res" as a local variable, just assign the return value from ReceiveInputEvent directly into *aOutStatus.

@@ +230,5 @@
> +
> +void
> +APZCTreeManagerParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +}

Since this function isn't implemented, maybe remove it from the class?

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +386,5 @@
>    // Protected destructor, to discourage deletion outside of Release():
>    virtual ~APZCTreeManager();
>  
> +  friend class APZCTreeManagerParent;
> +

Instead of making this a friend, just make the TransformEventRefPoint and UpdateWheelTransaction functions public. I wanted them to be protected on the interface, so that they can't be called in the main process, but as only things in the GPU process will hold a reference to an actual APZCTreeManager instance, it should be ok to make those function implementations public.

::: widget/InputData.h
@@ +107,5 @@
> +
> +  InputData()
> +    : mTime(0),
> +      modifiers(0)
> +  {

This constructor doesn't look used, I think you can drop it. Likewise the friend-ness in InputData shouldn't be needed since the protected constructor is only used by subclasses, I think.
Attachment #8770012 - Flags: review?(bugmail) → review+
Jeff, see first paragraph of comment 22 ^
Flags: needinfo?(jmuizelaar)
Oh, one other thing - please make sure the new files you're adding are not marked as executable. When I pulled down your patches from the try push I noticed that all the new files had mode 0755 instead of 0644 which makes them executable. I think it's just an artifact of how windows works and honestly I'm not sure how to make that not the case but I'm sure some googling will find a solution.
My leaning is toward gfx/layers/ipc
Flags: needinfo?(jmuizelaar)
From the last patch
* IAPZCTreeManager is moved to gfx/layers/apz/public
* references to APZCTreeManager in Android and OSX are fixed
* there are additions to IAPZCTreeManager for extra methods needed by Android
Attachment #8770011 - Attachment is obsolete: true
Attachment #8770012 - Attachment is obsolete: true
Attachment #8770014 - Attachment is obsolete: true
Attachment #8772641 - Flags: review?(bugmail)
Changes from last patch:
* IPDL protocol and implementation moved to gfx/layers/ipc
Attachment #8772643 - Flags: review?(bugmail)
Just a test patch tying it all together.

Note that running the IPDL protocol on android is not working at this time because of a main/ui thread issue.

A try run on platforms where it can work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5318a94d2b4
Comment on attachment 8772641 [details] [diff] [review]
Extract interface of APZCTreeManager

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

::: gfx/ipc/InProcessCompositorSession.cpp
@@ +9,5 @@
> +#include "base/process_util.h"
> +// so we can cast an APZCTreeManager to an IAPZCTreeManager
> +#include "mozilla/layers/APZCTreeManager.h"
> +#include "mozilla/layers/IAPZCTreeManager.h"
> +#include "mozilla/widget/PlatformWidgetTypes.h"

Doesn't look like we need base/process_util.h and PlatformWidgetTypes.h in this patch?

::: gfx/layers/apz/public/IAPZCTreeManager.h
@@ +192,5 @@
> +   */
> +  virtual void SetLongTapEnabled(bool aTapGestureEnabled) = 0;
> +
> +  /*
> +   * Process touch velocity.

nit: start comment /** instead of /* (tools like doxygen treat the former as special and tend to ignore the latter).
Attachment #8772641 - Flags: review?(bugmail) → review+
Comment on attachment 8772643 [details] [diff] [review]
Create IPDL protocol for IAPZCTreeManager

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +385,5 @@
>  protected:
>    // Protected destructor, to discourage deletion outside of Release():
>    virtual ~APZCTreeManager();
>  
> +  friend class APZCTreeManagerParent;

IIRC from our previous discussion, this shouldn't be needed if you make the functions below public instead of protected?

::: gfx/layers/ipc/APZCTreeManagerParent.h
@@ +115,5 @@
> +
> +  bool
> +  RecvProcessTouchVelocity(
> +    const uint32_t& aTimestampMs,
> +    const float& aSpeedY) override;

Indent these to be consistent with the rest
Attachment #8772643 - Flags: review?(bugmail) → review+
Attachment #8772641 - Attachment is obsolete: true
Attachment #8772643 - Attachment is obsolete: true
Attachment #8772644 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3119db384fcc
Extract interface of APZCTreeManager for moving to GPUProcess. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95ebdb96050
Create IPDL protocol implementation of IAPZCTreeManager. r=kats
Keywords: checkin-needed
What kind of bustage? https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8a00db57d77ab0ac59a1707c87da8232eeae2699 has no build failures and it's not clear which (if any) of the test failures were blamed on these patches.
Flags: needinfo?(cbook)
kats sorry somehow pasting the link failed here - its https://treeherder.mozilla.org/logviewer.html#?job_id=32299017&repo=mozilla-inbound
Flags: needinfo?(cbook)
Thanks! Looks like it was just missing the "explicit" qualifier on the APZCTreeManagerParent constructor declaration, so I added it and relanded.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02f12f2a1fbf
Extract interface of APZCTreeManager for moving to GPUProcess. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/0055f23a1374
Create IPDL protocol implementation of IAPZCTreeManager. r=kats
https://hg.mozilla.org/mozilla-central/rev/02f12f2a1fbf
https://hg.mozilla.org/mozilla-central/rev/0055f23a1374
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1288720
Since Kartikaya is currently out, Ryan, Milan -- is this something manual QA should be looking at? If so, could you please elaborate on how it can be tested manually?
Flags: qe-verify?
Flags: needinfo?(rhunt)
Flags: needinfo?(milan)
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #40)
> Since Kartikaya is currently out, Ryan, Milan -- is this something manual QA
> should be looking at? If so, could you please elaborate on how it can be
> tested manually?

No manual QA'ing should be needed here, this is plumbing work for the GPU process.
Flags: needinfo?(rhunt)
Flags: needinfo?(milan)
(In reply to David Anderson [:dvander] from comment #41)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #40)
> > Since Kartikaya is currently out, Ryan, Milan -- is this something manual QA
> > should be looking at? If so, could you please elaborate on how it can be
> > tested manually?
> 
> No manual QA'ing should be needed here, this is plumbing work for the GPU
> process.

Thank you for following up on this!
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.