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

RESOLVED FIXED in Firefox 49

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kats, Assigned: ernest, Mentored)

Tracking

({arch})

unspecified
mozilla49
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 6 obsolete attachments)

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.

Updated

3 years ago
Whiteboard: [gfx-noted]
Mentor: bugmail.mozilla@staktrace.com
Keywords: arch
Tentatively assigning this bug to ernest as a first bug. IS this still relevant kats? I see 'widget/ContentHelper.cpp' is still there.
Flags: needinfo?(bugmail.mozilla)

Updated

2 years ago
Assignee: nobody → eyim
Yes, still relevant. Thanks! I'm happy to mentor/answer questions.
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Comment 3

2 years ago
Created attachment 8756539 [details]
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/
Attachment #8756539 - Flags: review?(bugmail.mozilla)
Attachment #8756539 - Flags: review?(bgirard)
Attachment #8756540 - Flags: review?(bugmail.mozilla)
Attachment #8756540 - Flags: review?(bgirard)
Attachment #8756541 - Flags: review?(bugmail.mozilla)
Attachment #8756541 - Flags: review?(bgirard)
Attachment #8756542 - Flags: review?(bugmail.mozilla)
Attachment #8756542 - Flags: review?(bgirard)
Attachment #8756543 - Flags: review?(bugmail.mozilla)
Attachment #8756543 - Flags: review?(bgirard)
Attachment #8756544 - Flags: review?(bugmail.mozilla)
Attachment #8756544 - Flags: review?(bgirard)
(Assignee)

Comment 4

2 years ago
Created attachment 8756540 [details]
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/
(Assignee)

Comment 5

2 years ago
Created attachment 8756541 [details]
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/
(Assignee)

Comment 6

2 years ago
Created attachment 8756542 [details]
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/
(Assignee)

Comment 7

2 years ago
Created attachment 8756543 [details]
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8756544 [details]
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/
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.
I agree with BenWa - the changes look good, but please squash them together into a single changeset and push that for review.
(Assignee)

Comment 11

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8756540 - Attachment is obsolete: true
Attachment #8756540 - Flags: review?(bugmail.mozilla)
Attachment #8756540 - Flags: review?(bgirard)
(Assignee)

Updated

2 years ago
Attachment #8756541 - Attachment is obsolete: true
Attachment #8756541 - Flags: review?(bugmail.mozilla)
Attachment #8756541 - Flags: review?(bgirard)
(Assignee)

Updated

2 years ago
Attachment #8756542 - Attachment is obsolete: true
Attachment #8756542 - Flags: review?(bugmail.mozilla)
Attachment #8756542 - Flags: review?(bgirard)
(Assignee)

Updated

2 years ago
Attachment #8756543 - Attachment is obsolete: true
Attachment #8756543 - Flags: review?(bugmail.mozilla)
Attachment #8756543 - Flags: review?(bgirard)
(Assignee)

Updated

2 years ago
Attachment #8756544 - Attachment is obsolete: true
Attachment #8756544 - Flags: review?(bugmail.mozilla)
Attachment #8756544 - Flags: review?(bgirard)
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
Attachment #8756539 - Flags: review?(bgirard) → review+
Attachment #8756539 - Flags: review?(bugmail.mozilla) → review+
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.
(Assignee)

Comment 14

2 years ago
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/
Attachment #8756539 - Attachment description: MozReview Request: Bug 1145085 - Move and consolidate widget/ContentHelper.cpp → MozReview Request: Bug 1145085 - Move widget/ContentHelper.* to gfx/layers/apz/util/TouchActionHelper.*.
 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    ^
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?
Flags: needinfo?(sphink)
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.
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.
(Assignee)

Comment 19

2 years ago
Created attachment 8757317 [details]
MozReview Request: edit

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

Comment 20

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b99e7fee7c8d
(Assignee)

Comment 21

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8757317 - Attachment is obsolete: true
(Assignee)

Comment 22

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

Comment 23

2 years ago
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 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.
Attachment #8756539 - Flags: review+
(Assignee)

Comment 25

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=992e083f0255
(Assignee)

Comment 26

2 years ago
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/
Attachment #8756539 - Flags: review?(bgirard)
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
Attachment #8756539 - Flags: review?(bgirard) → review+
Alright we've got a work around, we can land this Monday after we have the try results. ni?self as a reminder.
Flags: needinfo?(sphink) → needinfo?(bgirard)

Comment 29

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

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ab467b4554b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

2 years ago
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.