Closed Bug 1013612 Opened 10 years ago Closed 10 years ago

Remove APZ-internal headers from export list in gfx/layers/moz.build

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(1 file, 1 obsolete file)

Right now the gfx/layers/moz.build file exports APZ-internal headers like Axis.h and GestureEventListener.h which we shouldn't need to export. We should do forward-declaration or whatever else we need to untangle the build dependencies and remove these exported headers.

One problem I ran into in my quick effort to do this is that ContainerLayer instances keep a refptr to AsyncPanZoomController, which AFAIK requires #include'ing AsyncPanZoomController.h. AsyncPanZoomController in turn requires including GestureEventListener.h and Axis.h.

I'm not sure if there's a way to break the ContainerLayer->APZC dependency short of doing a cast or introducing an interface.
Attached patch WIP (obsolete) — Splinter Review
Having to add new exported headers for every new file is annoying me sufficiently that I found a way around this, I think. WIP attached, try push to verify:

remote: You can view the progress of your build at the following URL:
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d79399ef5d86
remote: Alternatively, view them on TBPL (soon to be deprecated):
remote:   https://tbpl.mozilla.org/?tree=Try&rev=d79399ef5d86
Comment on attachment 8505677 [details] [diff] [review]
WIP

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/layers/AsyncCompositionManager.h"
>  #include <stdint.h>                     // for uint32_t
> +#include "apz/src/AsyncPanZoomController.h"

Why is this better than exporting AsyncPanZoomController.h?
Because now only gfx/layers code can access it, and so it prevents accumulation of additional dependencies. Particularly on classes like Axis, TaskThrottler, and GestureListener which really shouldn't be exposed.

That being said, there's definitely more we can do here in terms of encapsulating the APZ code, but I think this is worth landing by itself.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Because now only gfx/layers code can access it, and so it prevents
> accumulation of additional dependencies. Particularly on classes like Axis,
> TaskThrottler, and GestureListener which really shouldn't be exposed.

If gfx/layers code accesses it via 'apz/src/Header.h', what stops
non-gfx/layers code from accessing it via '../apz/src/Header.h'?
Nothing, but there's no way we can prevent that regardless.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Nothing, but there's no way we can prevent that regardless.

Agreed. My point is that this patch introduces a precedent for including a file that's neither local (in the same directory) nor exported. We could avoid introducing such a precedent by leaving AsyncPanZoomController.h exported.
(In reply to Botond Ballo [:botond] from comment #7)
> Agreed. My point is that this patch introduces a precedent for including a
> file that's neither local (in the same directory) nor exported.

I don't think that's true, there's already code that does that. See [1] [2] [3] for some examples.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?rev=094128a6d5a6#43
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d10/ColorLayerD3D10.cpp?rev=094128a6d5a6#8
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp?rev=094128a6d5a6#30
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> (In reply to Botond Ballo [:botond] from comment #7)
> > Agreed. My point is that this patch introduces a precedent for including a
> > file that's neither local (in the same directory) nor exported.
> 
> I don't think that's true, there's already code that does that. See [1] [2]
> [3] for some examples.

Hmm, good point. I guess what bothers me is that it's not clear whether not exporting an APZ header is intended to mean "only APZ code should use this" (seems to be the case for e.g. InputBlockState.h) or "only gfx/layers code should use this" (seems to be the case for AsyncPanZoomController.h).
I'm not sure I'm following. As far as i can see InputBlockState.h isn't exported anywhere.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> I'm not sure I'm following. As far as i can see InputBlockState.h isn't
> exported anywhere.

Let me try that again :)

It's not clear whether *not exporting* (i.e., failing to export) an APZ header is intended to mean "only APZ code should use this" (seems to be the case for e.g. InputBlockState.h) or "only gfx/layers code should use this" (seems to be the case for AsyncPanZoomController.h).
I think you're trying to look for a "rule" of some sort when in fact there isn't one that's strictly followed. The only rule I can think of that we have now is "export the minimal set of headers needed to compile the codebase" and my patch does nothing to change that. It basically has nothing to do with which code is supposed to be using what.

I think we both agree that (1) having headers used willy-nilly all over the place is bad and (2) not exporting headers makes them harder to use willy-nilly all over the place. Therefore, this patch improves the status quo. Yes, it doesn't take us to where we want to be (in terms of only exposing headers that the non-APZ code should use) but following the mozilla convention of not letting "perfect" be the enemy of "good" I think it's good enough to get in the tree. Unless it actually has downsides that I'm not aware of, that is.

The one tangible benefit I get from this patch is that as extract more helper classes, I don't have to keep adding them to the exports list in moz.build.
Attached patch PatchSplinter Review
If you'd rather I didn't land this until your stage 1 change to CalculatePendingDisplayPort lands that's fine by me.
Assignee: nobody → bugmail.mozilla
Attachment #8505677 - Attachment is obsolete: true
Attachment #8505909 - Flags: review?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> The one tangible benefit I get from this patch is that as extract more
> helper classes, I don't have to keep adding them to the exports list in
> moz.build.

What I had in mind was continuing to export AsyncPanZoomController.h only (since it's used from outside of gfx/layers/apz), not the other files your patch un-exports, nor any new helper files.

Unfortunately, that doesn't seem to work, as AsyncPanZoomController.h includes TaskThrottler.h and Axis.h; if APZC.h is exported and the others are not, then, when processing e.g. |#include "Axis.h"| in APZC.h, the compiler will expect to find Axis.h in the same directory as APZC.h, which (since APZC.h is exported) is dist/include/mozilla/layers, but won't find it (since Axis.h is not exported). In essence, exporting APZC.h "infects" the header files it includes, requiring them to be exported too. (Apologies if this was obvious to you from the get-go.)

Given this state of affairs, I have no objection to your patch.
Attachment #8505909 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> If you'd rather I didn't land this until your stage 1 change to
> CalculatePendingDisplayPort lands that's fine by me.

It doesn't look like a difficult rebase; feel free to land this first if it's ready.
(In reply to Botond Ballo [:botond] from comment #14)
> What I had in mind was continuing to export AsyncPanZoomController.h only
> (since it's used from outside of gfx/layers/apz), not the other files your
> patch un-exports, nor any new helper files.

Ah, I see. I didn't understand that's what you meant. Thanks for clarifying!

> Unfortunately, that doesn't seem to work, as AsyncPanZoomController.h
> includes TaskThrottler.h and Axis.h; if APZC.h is exported and the others
> are not, then, when processing e.g. |#include "Axis.h"| in APZC.h, the
> compiler will expect to find Axis.h in the same directory as APZC.h, which
> (since APZC.h is exported) is dist/include/mozilla/layers, but won't find it
> (since Axis.h is not exported). In essence, exporting APZC.h "infects" the
> header files it includes, requiring them to be exported too. (Apologies if
> this was obvious to you from the get-go.)

Yeah, I had assumed you already knew that was not possible. That's why we exported those other headers in the first place, and filed this bug as a follow-up (see bug 995411 comment 5). :)
https://hg.mozilla.org/mozilla-central/rev/6ddd4cc9a274
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.