1443292 - Move TouchBehaviorFlags from APZUtils.h to LayersTypes.h

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Botond Ballo [:botond]]

Right now, modifying APZUtils.h results in a long recompile because it's included from many places (notably, PBrowser.ipdl, PCompositorBridge.ipdl, and a couple of other ipdl files).

However, the only declaration in APZUtils.h that's actually used in these places is the typedef 'TouchBehaviorFlags'; everything else is only used by APZ code.

We should move the typedef 'TouchBehaviorFlags' from APZUtils.h to LayersTypes.h (which is already a lost-cause modifying-this-will-recompile-the-world header), so that modifying the other declarations in APZUtils.h only causes a short recompile.

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> Right now, modifying APZUtils.h results in a long recompile because it's
> included from many places (notably, PBrowser.ipdl, PCompositorBridge.ipdl,
> and a couple of other ipdl files).

Actually, PBrowser.ipdl, PCompositorBridge.ipdl, and PAPZ.ipdl, all of which include APZUtils.h, don't even use TouchBehaviorFlags; only PAPZCTreeManager.ipdl does. We should make sure we remove those usused includes.

Comment 2

[peter.bacalso]

Hello! I would like to work on this as a good first bug

Comment 3

[Botond Ballo [:botond]]

Great, thanks for your interest!

The first step is to build Firefox as described on this page:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Once you've done that, post back here and I'll give some specific guidance for this bug.

Let me know if you run into any issues!

Comment 4

[peter.bacalso]

Thanks! I have the Firefox build up and running now. How should I get started with this bug?

Comment 5

[Botond Ballo [:botond]]

Great! The general idea is:

  - Make the desired changes to the source code
  - Run |mach build| again to make sure the code still compiles
    with your modifications
  - Post a patch file containing your changes for review

In terms of what changes to make in this bug, the main change is to move the declaration of the typedef |TouchBehaviorFlags| from APZUtils.h [1] to LayersTypes.h [2]. We want to keep the typedef in the namespace mozilla::layers, so be sure you're adding it somewhere in LayersTypes.h that's inside mozilla::layers (most declarations in that file are, except for a few near the top).

In addition, we want to update some files that are only including APZUtils.h to get the TouchBehaviorFlags typedef, to include LayersTypes.h instead. You can use the Searchfox tool to find all the places that include APZUtils.h, with a query like this [3]. Any include where you see an include of APZUtils.h with the comment "// for TouchBehaviorFlags", you can replace with |#include "LayersTypes.h"| instead, if the file doesn't already include LayersTypes.h. (In IAPZCTreeManager.h, the comment says "// for TouchBehaviorFLags, etc", but in spite of the "etc", TouchBehaviorFlags is the only thing being used from that file, so we can replace that as well.)

You'll notice a few of the search results in that query are files with extension .ipdl, where instead of an #include you have a statement of this form:

  using mozilla::layers::TouchBehaviorFlags from "mozilla/layers/APZUtils.h";

IPDL is our interprocess communication mechanism, and .ipdl files are processed by a code generator to produce C++ header and source files. A "using" statement like this will result in the file in question being included from the generated files. IPDL headers are included fairly widely in our codebase, so it's particularly important to update these "using" statements. The replacement in this case is:

  using mozilla::layers::TouchBehaviorFlags from "mozilla/layers/LayersTypess.h";

Finally, as observed in comment 1, three of the four .ipdl files that have this "using" statement don't actually use TouchBehaviorFlags; in those files, the "using" statement can be removed altogether.

That should be it - let me know if you have any questions!

[1] https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/gfx/layers/apz/src/APZUtils.h#54
[2] https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/gfx/layers/LayersTypes.h
[3] https://searchfox.org/mozilla-central/search?q=APZUtils.h&path=

Comment 6

[Botond Ballo [:botond]]

(Assigning the bug to you to indicate that you're working on it.)

Comment 7

[peter.bacalso]

It seems that PBrowser.ipdl needs to include the "using" statement as well. TabChild.h uses TouchBehaviorFlags in [1] and it includes PBrowserChild.h and in turn includes PBrowser.h which (going by your definition of .ipdl files) is generated by PBrowser.ipdl. Can you confirm that this is the case?

[1] https://searchfox.org/mozilla-central/source/dom/ipc/TabChild.h#668

Comment 8

[Botond Ballo [:botond]]

(In reply to peter.bacalso from comment #7)
> It seems that PBrowser.ipdl needs to include the "using" statement as well.
> TabChild.h uses TouchBehaviorFlags in [1] and it includes PBrowserChild.h
> and in turn includes PBrowser.h which (going by your definition of .ipdl
> files) is generated by PBrowser.ipdl.

Ah, good catch! In a case like this, we can include LayersTypes.h directly from TabChild.h (which is not a generated header). That way, at least the other headers which include PBrowser.h don't pull it in unnecessarily.

Comment 9

[peter.bacalso]

(In reply to Botond Ballo [:botond] from comment #8)
> (In reply to peter.bacalso from comment #7)
> > It seems that PBrowser.ipdl needs to include the "using" statement as well.
> > TabChild.h uses TouchBehaviorFlags in [1] and it includes PBrowserChild.h
> > and in turn includes PBrowser.h which (going by your definition of .ipdl
> > files) is generated by PBrowser.ipdl.
> 
> Ah, good catch! In a case like this, we can include LayersTypes.h directly
> from TabChild.h (which is not a generated header). That way, at least the
> other headers which include PBrowser.h don't pull it in unnecessarily.

Hm when I included LayersTypes.h into PBrowser.h with the "using" statement, I did not get any errors when I ran |mach build| but when I directly include LayersTypes.h into TabChild.h, I get an error saying "TouchBehaviorFlags is not declared in the scope". I think it has something to do with the access modifiers but I am not sure how I can fix this. Any advice?

Comment 10

[Botond Ballo [:botond]]

(In reply to peter.bacalso from comment #9)
> Hm when I included LayersTypes.h into PBrowser.h with the "using" statement,
> I did not get any errors when I ran |mach build| but when I directly include
> LayersTypes.h into TabChild.h, I get an error saying "TouchBehaviorFlags is
> not declared in the scope". I think it has something to do with the access
> modifiers but I am not sure how I can fix this. Any advice?

I think the problem there is that TabChild is in namespace mozilla::dom, while TouchBehaviorFlags is in namespace mozilla::layers, so referencing TouchBehaviorFlags without any qualification in TabChild doesn't work.

The IPDL code generator deals with this by generating a typedef in TabChild's base class, PBrowserChild [1].

We can add such a typedef into TabChild itself, near the top where we already have some [2].

Let me know if that solves the problem!

[1] https://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/_ipdlheaders/mozilla/dom/PBrowserChild.h#531
[2] https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/dom/ipc/TabChild.h#259

Comment 11

[peter.bacalso]

Thanks, it solved the problem! 

So far I have done the following changes:
 - moved TouchBehaviorFlags from APZUtils.h to LayersTypes.h
 - updated the include dependency of affected headers and PAPZCTreeManager.ipdl
 - removed the includes from the remaining 3 .ipdl files

It took a while but |mach build| completed successfully and I think I am ready to post a patch for review.

Comment 12

[Botond Ballo [:botond]]

> I think I am ready to post a patch for review.

Great! The easiest way to do that is probably to upload a patch generated using "hg export" or similar as an attachment to this bug. You can then flag the attachment for review by me.

Comment 13

[peter.bacalso]

Attachment: moved TouchBehaviorFlags from APZUtils.h to LayersTypes.h

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8958203 [details] [diff] [review]
moved TouchBehaviorFlags from APZUtils.h to LayersTypes.h

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

Looks good, thanks!

Comment 15

[Botond Ballo [:botond]]

I pushed the change to our Try server to make sure it's building on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec708eeaf78d3cf3aef2ce8f5637c98ba3afd906

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ec708eeaf78d3cf3aef2ce8f5637c98ba3afd906

It looks like InputBlockState.h was using APZUtils.h for more things than just TouchBehaviorFlags, so we need to keep the include of APZUtils.h and add LayersTypes.h in that file.

Could you upload an updated patch that makes that change please?

Comment 17

[peter.bacalso]

Attachment: re-added APZUtils.h into InputBlockState.h

Comment 18

[peter.bacalso]

Attachment: re-added APZUtils.h into InputBlockState.h

Comment 19

[peter.bacalso]

Sorry for the duplicate patch, I forgot the run |mach build| for the first one so I built it and resent an updated patch. Please let me know if there is anything else that needs to be updated.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8959032 [details] [diff] [review]
re-added APZUtils.h into InputBlockState.h

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

Looks great; however, could I ask you to combine this and the previous patch into one patch? You can do this with 'hg histedit'. Thanks!

Comment 21

[peter.bacalso]

Attachment: moved TouchBehaviorFlags from APZUtils.h to LayersTypes.h

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8959137 [details] [diff] [review]
moved TouchBehaviorFlags from APZUtils.h to LayersTypes.h

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

Thanks!

Comment 23

[Botond Ballo [:botond]]

Pushed the updated patch to the Try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5106f104b543ed27d6fd9701d5e2299d6f8b0c0c

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfc3bd70bfa
Move TouchBehaviorFlags from APZUtils.h to LayersTypes.h. r=botond

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [standards meeting March 12-17] from comment #23)
> Pushed the updated patch to the Try server:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5106f104b543ed27d6fd9701d5e2299d6f8b0c0c

Looks good! I pushed the patch to mozilla-inbound. It should merge over to mozilla-central within a day or so.

Comment 26

[peter.bacalso]

Great! Thank you for the thorough guidance on my first bug!

Comment 27

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/fbfc3bd70bfa

Comment 28

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [standards meeting March 12-17] from comment #25)
> Looks good! I pushed the patch to mozilla-inbound. It should merge over to
> mozilla-central within a day or so.

Done now. Thanks for your work here, Peter!

If you're interested in working on another bug, and would like some suggestions, let me know!