Move TouchBehaviorFlags from APZUtils.h to LayersTypes.h

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: botond, Assigned: peter.bacalso, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla61
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 months ago
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.
(Reporter)

Comment 1

11 months ago
(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.
Priority: -- → P3
(Assignee)

Comment 2

11 months ago
Hello! I would like to work on this as a good first bug
(Reporter)

Comment 3

11 months ago
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!
(Assignee)

Comment 4

11 months ago
Thanks! I have the Firefox build up and running now. How should I get started with this bug?
(Reporter)

Comment 5

11 months ago
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=
(Reporter)

Comment 6

11 months ago
(Assigning the bug to you to indicate that you're working on it.)
Assignee: nobody → peter.bacalso
(Assignee)

Comment 7

10 months ago
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
(Reporter)

Comment 8

10 months ago
(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.
(Assignee)

Comment 9

10 months ago
(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?
(Reporter)

Comment 10

10 months ago
(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
(Assignee)

Comment 11

10 months ago
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.
(Reporter)

Comment 12

10 months ago
> 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.
(Assignee)

Comment 13

10 months ago
Created attachment 8958203 [details] [diff] [review]
moved TouchBehaviorFlags from APZUtils.h to LayersTypes.h
Attachment #8958203 - Flags: review?(botond)
(Reporter)

Comment 14

10 months ago
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!
Attachment #8958203 - Flags: review?(botond) → review+
(Reporter)

Comment 15

10 months ago
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
(Reporter)

Comment 16

10 months ago
(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?
(Assignee)

Comment 17

10 months ago
Created attachment 8959000 [details] [diff] [review]
re-added APZUtils.h into InputBlockState.h
(Assignee)

Comment 18

10 months ago
Created attachment 8959032 [details] [diff] [review]
re-added APZUtils.h into InputBlockState.h
Attachment #8959032 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8959000 - Attachment is obsolete: true
(Assignee)

Comment 19

10 months ago
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.
(Reporter)

Comment 20

10 months ago
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!
Attachment #8959032 - Flags: review?(botond)
(Assignee)

Comment 21

10 months ago
Created attachment 8959137 [details] [diff] [review]
moved TouchBehaviorFlags from APZUtils.h to LayersTypes.h
Attachment #8959137 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8958203 - Attachment is obsolete: true
(Reporter)

Comment 22

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

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

Thanks!
Attachment #8959137 - Flags: review?(botond) → review+
(Reporter)

Updated

10 months ago
Attachment #8959032 - Attachment is obsolete: true

Comment 24

10 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfc3bd70bfa
Move TouchBehaviorFlags from APZUtils.h to LayersTypes.h. r=botond
(Reporter)

Comment 25

10 months ago
(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.
(Assignee)

Comment 26

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

Comment 27

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fbfc3bd70bfa
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Reporter)

Comment 28

10 months ago
(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!
status-firefox60: affected → wontfix
You need to log in before you can comment on or make changes to this bug.