Closed Bug 1145085 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: kats, Assigned: ernest, Mentored)

Details

(Keywords: arch, Whiteboard: [gfx-noted])

Attachments

(1 file, 6 obsolete files)

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.
Whiteboard: [gfx-noted]
Mentor: bugmail.mozilla
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)
Assignee: nobody → eyim
Yes, still relevant. Thanks! I'm happy to mentor/answer questions.
Flags: needinfo?(bugmail.mozilla)
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)
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.
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/
Attachment #8756540 - Attachment is obsolete: true
Attachment #8756540 - Flags: review?(bugmail.mozilla)
Attachment #8756540 - Flags: review?(bgirard)
Attachment #8756541 - Attachment is obsolete: true
Attachment #8756541 - Flags: review?(bugmail.mozilla)
Attachment #8756541 - Flags: review?(bgirard)
Attachment #8756542 - Attachment is obsolete: true
Attachment #8756542 - Flags: review?(bugmail.mozilla)
Attachment #8756542 - Flags: review?(bgirard)
Attachment #8756543 - Attachment is obsolete: true
Attachment #8756543 - Flags: review?(bugmail.mozilla)
Attachment #8756543 - Flags: review?(bgirard)
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.
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.
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/
Attachment #8757317 - Attachment is obsolete: true
took nsBaseDragService.cpp out of unified build, and try seems to be good now
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0591c757a96c
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+
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)
https://hg.mozilla.org/mozilla-central/rev/8ab467b4554b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.