1145085 - Move widget/ContentHelper.cpp to gfx/layers/apz/util/

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The widget/ContentHelper.cpp file was originally added to provide touch-action information to APZ. However since it's pretty APZ-specific we should move it into gfx/layers/apz/util (and maybe rename it to TouchActionHelper.cpp?). Also as part of this we should unify the TouchBehaviorFlags typedef in ContentHelper.h with the one in APZUtils.h and do any other general cleanup needed.

Comment 1

[Benoit Girard (:BenWa)]

Tentatively assigning this bug to ernest as a first bug. IS this still relevant kats? I see 'widget/ContentHelper.cpp' is still there.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Yes, still relevant. Thanks! I'm happy to mentor/answer questions.

Comment 3

[Ernest Yim [:ernest]]

Attachment: MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

Review commit: https://reviewboard.mozilla.org/r/55214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55214/

Comment 4

[Ernest Yim [:ernest]]

Attachment: MozReview Request: rename calls ContentHelper->TouchActionHelper

Review commit: https://reviewboard.mozilla.org/r/55216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55216/

Comment 5

[Ernest Yim [:ernest]]

Attachment: MozReview Request: update APZCallbackhelper.cpp include

Review commit: https://reviewboard.mozilla.org/r/55218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55218/

Comment 6

[Ernest Yim [:ernest]]

Attachment: MozReview Request: update helper to use typedef from APZUtils

Review commit: https://reviewboard.mozilla.org/r/55220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55220/

Comment 7

[Ernest Yim [:ernest]]

Attachment: MozReview Request: remove contenthelper.h export in build

Review commit: https://reviewboard.mozilla.org/r/55222/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55222/

Comment 8

[Ernest Yim [:ernest]]

Attachment: MozReview Request: Fixed unified build errors associated with change

Review commit: https://reviewboard.mozilla.org/r/55224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55224/

Comment 9

[Benoit Girard (:BenWa)]

The squashed diff looks good. Great work!

We tend to go for changesets that model one logical change that build on their own. You should be able to bisect through every commit that you post for review for instance. All these changes belong to one commit and thus should be squashed. If you're using mq you should 'qfold' otherwise I *believe* histedit is the right tool to fold these commits.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I agree with BenWa - the changes look good, but please squash them together into a single changeset and push that for review.

Comment 11

[Ernest Yim [:ernest]]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55214/diff/1-2/

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

https://reviewboard.mozilla.org/r/55214/#review52058

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

https://reviewboard.mozilla.org/r/55214/#review52160

Patch looks good, but please update the commit message. When you use qfold it squashes together all the commit messages from the individual patches; you should clean it up so it's just the one line "Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*. r=BenWa,kats". (mercurial will probably also insert a new MozReview-Commit-ID, that's fine.

Comment 14

[Ernest Yim [:ernest]]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55214/diff/2-3/

Comment 15

[Benoit Girard (:BenWa)]

 2:30.69 *** WARNING *** there are active plugins, do not report this as a bug unless you can reproduce it without enabling any plugins.
 2:30.69 Event                            | Plugins
 2:30.69 PLUGIN_FINISH_TYPE               | xgill
 2:30.69 PLUGIN_FINISH_DECL               | xgill
 2:30.69 PLUGIN_FINISH_UNIT               | xgill
 2:30.69 PLUGIN_PRE_GENERICIZE            | xgill
 2:30.69 PLUGIN_ATTRIBUTES                | xgill
 2:30.69 PLUGIN_START_UNIT                | xgill
 2:30.69 In file included from /home/worker/workspace/gecko/dom/base/nsIContent.h:11:0,
 2:30.69                  from /home/worker/workspace/gecko/obj-analyzed/dist/include/mozilla/dom/FragmentOrElement.h:20,
 2:30.69                  from /home/worker/workspace/gecko/obj-analyzed/dist/include/mozilla/dom/Element.h:16,
 2:30.69                  from /home/worker/workspace/gecko/obj-analyzed/dist/include/nsStyledElement.h:18,
 2:30.69                  from /home/worker/workspace/gecko/obj-analyzed/dist/include/nsMappedAttributeElement.h:16,
 2:30.69                  from /home/worker/workspace/gecko/obj-analyzed/dist/include/nsGenericHTMLElement.h:11,
 2:30.69                  from /home/worker/workspace/gecko/obj-analyzed/dist/include/mozilla/dom/HTMLCanvasElement.h:14,
 2:30.69                  from /home/worker/workspace/gecko/widget/nsBaseDragService.h:19,
 2:30.70                  from /home/worker/workspace/gecko/widget/nsBaseDragService.cpp:6,
 2:30.70                  from /home/worker/workspace/gecko/obj-analyzed/widget/Unified_cpp_widget1.cpp:20:
 2:30.70 /home/worker/workspace/gecko/dom/base/nsINode.h: In member function 'virtual size_t nsINode::SizeOfIncludingThis(mozilla::MallocSizeOf) const':
 2:30.70 /home/worker/workspace/gecko/dom/base/nsINode.h:314:3: internal compiler error: in XIL_GetFunctionFields, at type.c:359
 2:30.70    }
 2:30.70    ^

Comment 16

[Benoit Girard (:BenWa)]

sfink looks like we're getting an ICE (See Comment 15) during the root analysis build from xgill. The patch probably modified Unified_cpp_widget1.cpp. Any ideas?

Comment 17

[Steve Fink [:sfink] [:s:]]

Hm. For now, I'm going to cross my fingers and hope that bug 1259850 will fix this. I'm trying to land it now, but I can't promise it'll be quick -- I've been struggling with it for a while.

Comment 18

[Benoit Girard (:BenWa)]

Thanks for the update.

Perhaps ernest you should try to de-unify widget/nsBaseDragService.cpp and/or files that you've touched and do another try push. Unification is not that important.

Comment 19

[Ernest Yim [:ernest]]

Attachment: MozReview Request: edit

Review commit: https://reviewboard.mozilla.org/r/55800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55800/

Comment 20

[Ernest Yim [:ernest]]

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

Comment 21

[Ernest Yim [:ernest]]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55214/diff/3-4/

Comment 22

[Ernest Yim [:ernest]]

took nsBaseDragService.cpp out of unified build, and try seems to be good now
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0591c757a96c

Comment 23

[Ernest Yim [:ernest]]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55214/diff/4-5/

Comment 24

[Benoit Girard (:BenWa)]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

https://reviewboard.mozilla.org/r/55214/#review52628

Let's update the comment and then we can land this.

::: widget/moz.build:183
(Diff revisions 4 - 5)
>          'nsPrintOptionsImpl.cpp',
>          'nsPrintSession.cpp',
>      ]
>  
>  # nsBaseWidget.cpp needs to be built separately because of name clashes in the OS X headers
>  # nsBaseDragService.cpp moved out of UNIFIED to fix compiler issues after moving widget/ContentHelper -> apz/util/TouchActionHelper

Let's be more specific here. It's a crash from xgill and not the compiler. And include bug 1259850 so that someone looking to fix this later will know where to look.

Comment 25

[Ernest Yim [:ernest]]

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

Comment 26

[Ernest Yim [:ernest]]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55214/diff/5-6/

Comment 27

[Benoit Girard (:BenWa)]

Comment on attachment 8756539 [details]
MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.

https://reviewboard.mozilla.org/r/55214/#review52644

Comment 28

[Benoit Girard (:BenWa)]

Alright we've got a work around, we can land this Monday after we have the try results. ni?self as a reminder.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab467b4554b

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ab467b4554b