Closed
Bug 1259666
Opened 7 years ago
Closed 6 years ago
Clean up WidgetGestureNotifyEvent
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: masayuki, Assigned: kshitijakarkar, Mentored)
References
Details
(Whiteboard: [good first bug])
Attachments
(3 files, 3 obsolete files)
See bug 1259654 comment 0 for the detail.
I would like to work on this bug. Can you help me with it?
Reporter | ||
Comment 2•6 years ago
|
||
kshitija: Thank you. I'd like you to do here are: 1. Rename ePanDirection to PanDirection (remove the "e" prefix from the enum name) and explicitly define the type of enum as int8_t. So, the definition should be: typedef int8_t PanDirectionType; enum PanDirection : PanDirectionType { ... }; 2. Rename panDirection to mPanDirection. 3. Rename displayPanFeedback to mDisplayPanFeedback. So, you should create 3 separated patches. Each commit message should start with "Bug 1259666 part n " and explain what you do in the changeset. Finally, add "r?masayuki" and use |hg push -r firstchangeset:lastchangeset review| to request review. Then, I'll post the patches to tryserver for testing your patch doesn't cause bustage on any platforms.
Assignee: nobody → kshitijakarkar
Reporter | ||
Updated•6 years ago
|
Whiteboard: [good first bug][for Gecko Inside #7 in Mozilla Japan] → [good first bug]
The first part of patch
Attachment #8744755 -
Flags: review-
Please review the attachments. Sorry I could not push it to repository as the permission was denied for me . Capella helped me out n IRC #introduction and suggested to post it on bugzilla. Let me know if I still need to work on it or please resolve the bug Thank you
Updated•6 years ago
|
Attachment #8744757 -
Flags: review?(masayuki)
Updated•6 years ago
|
Attachment #8744755 -
Flags: review- → review?(masayuki)
Updated•6 years ago
|
Attachment #8744758 -
Flags: review?(masayuki)
Reporter | ||
Comment 7•6 years ago
|
||
(In reply to kshitija from comment #6) > Please review the attachments. Sorry I could not push it to repository as > the permission was denied for me . Capella helped me out n IRC #introduction > and suggested to post it on bugzilla. > Let me know if I still need to work on it or please resolve the bug We need to confirm that your patches don't cause build failure in any platforms. So, if you can, I still want you to push the patches to review repository. Cannot you fix the error with referring this comment? https://bugzilla.mozilla.org/show_bug.cgi?id=1259669#c9
Flags: needinfo?(kshitijakarkar)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7) > (In reply to kshitija from comment #6) > > Please review the attachments. Sorry I could not push it to repository as > > the permission was denied for me . Capella helped me out n IRC #introduction > > and suggested to post it on bugzilla. > > Let me know if I still need to work on it or please resolve the bug > > We need to confirm that your patches don't cause build failure in any > platforms. So, if you can, I still want you to push the patches to review > repository. > > Cannot you fix the error with referring this comment? > https://bugzilla.mozilla.org/show_bug.cgi?id=1259669#c9 It build onto my system perfectly. I will try pushing on the repository today.
Review commit: https://reviewboard.mozilla.org/r/48841/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48841/
Attachment #8745099 -
Flags: review?(masayuki)
Attachment #8745100 -
Flags: review?(masayuki)
Attachment #8745101 -
Flags: review?(masayuki)
Assignee | ||
Comment 10•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48843/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48843/
Assignee | ||
Comment 11•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48845/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48845/
Attachment #8744758 -
Attachment is obsolete: true
Attachment #8744758 -
Flags: review?(masayuki)
Attachment #8744757 -
Attachment is obsolete: true
Attachment #8744757 -
Flags: review?(masayuki)
Attachment #8744755 -
Attachment is obsolete: true
Attachment #8744755 -
Flags: review?(masayuki)
Assignee | ||
Comment 12•6 years ago
|
||
Pushed changes onto review repository finally. Please check
Reporter | ||
Comment 13•6 years ago
|
||
Thank you very much. I posted your patches to tryserver for build test on all OSes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=87aede2035cd I'll review them after confirming all green.
Flags: needinfo?(kshitijakarkar)
Reporter | ||
Updated•6 years ago
|
Attachment #8745099 -
Flags: review?(masayuki) → review+
Reporter | ||
Comment 14•6 years ago
|
||
Comment on attachment 8745099 [details] MozReview Request: Bug 1259666 part 1 - rename variable and definition change - r?masayuki https://reviewboard.mozilla.org/r/48841/#review45765 ::: widget/windows/nsWinGesture.h:201 (Diff revision 1) > { > public: > nsWinGesture(); > > public: > - bool SetWinGestureSupport(HWND hWnd, mozilla::WidgetGestureNotifyEvent::ePanDirection aDirection); > + bool SetWinGestureSupport(HWND hWnd, mozilla::WidgetGestureNotifyEvent::PanDirection aDirection); In our coding rules, one line should be 80 characters or less. So, this line is too long but it's impossible to wrap this line keeping easier to read... So, it's okay for now...
Reporter | ||
Comment 15•6 years ago
|
||
Comment on attachment 8745100 [details] MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki https://reviewboard.mozilla.org/r/48843/#review45767 You can modify "not granted" patch with following setps: 1. hg up <bad-revision> 2. Change something 3. Build it 4. hg commit -m "fix" (now this is "another" child of the bad-revision) 5. hg rebase -s <following-revision-of-the-bad-revision> -d <revision-of-created-by-step-4> 6. hg up tip 7. hg histedit <revision-of-created-by-step-4> 8. Modify "pick" to "r" of the line for <revision-of-created-by-stip-4> in the editor launched by step 7 and save and exit. Then, <revision-of-created-by-step-4> is merged to the <bad-revision>. And push it again to the review repogitory. ::: dom/events/EventStateManager.cpp:2706 (Diff revision 1) > * - For XUL: if it's an scrollable element that can currently scroll in some > * direction. > * > * Note: we'll have to one-off various cases to ensure a good usable behavior > */ > - WidgetGestureNotifyEvent::PanDirection panDirection = > + WidgetGestureNotifyEvent::PanDirection mPanDirection = No, this is local variable of this method. Don't rename this.
Attachment #8745100 -
Flags: review?(masayuki)
Reporter | ||
Updated•6 years ago
|
Attachment #8745101 -
Flags: review?(masayuki)
Reporter | ||
Comment 16•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki https://reviewboard.mozilla.org/r/48845/#review45769 ::: dom/events/EventStateManager.cpp:2708 (Diff revision 1) > * > * Note: we'll have to one-off various cases to ensure a good usable behavior > */ > WidgetGestureNotifyEvent::PanDirection mPanDirection = > WidgetGestureNotifyEvent::ePanNone; > - bool displayPanFeedback = false; > + bool mDisplayPanFeedback = false; This is local variable of the method, don't rename it.
Assignee | ||
Comment 17•6 years ago
|
||
I have to keep panDirection and displayPanFeedback from EventStateManager.cpp because they are local. Is that what you mean?
Reporter | ||
Comment 18•6 years ago
|
||
Exactly. "m" prefix is used only when the variable is a member of the class or struct.
Assignee | ||
Comment 19•6 years ago
|
||
Comment on attachment 8745100 [details] MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48843/diff/1-2/
Attachment #8745100 -
Attachment description: MozReview Request: Bug 1259666 - Part 2 - change to panDirection to mPanDirection - r?masayuki → MozReview Request: Bug 1259666 - Part 2 -fix change to panDirection to mPanDirection - r?masayuki
Attachment #8745101 -
Attachment description: MozReview Request: Bug 1259666 - Part 3 - change displayPanFeedback to mDisplayPanFeedback - r?masayuki → MozReview Request: Bug 1259666 Part 3 fix r?masayuki
Attachment #8745100 -
Flags: review?(masayuki)
Attachment #8745101 -
Flags: review?(masayuki)
Assignee | ||
Comment 20•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48845/diff/1-2/
Assignee | ||
Comment 21•6 years ago
|
||
I did the necessary changes as suggested by you. Hope this time its correct. I messed up with the steps given by you that's why it took time. Sorry the commit messages are not as you gave in the comment but they do describe which patch is fixed.
Assignee | ||
Comment 22•6 years ago
|
||
Can you please review patch 2 and patch 3 ?
Attachment #8745100 -
Flags: review?(masayuki) → review+
Attachment #8745101 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 23•6 years ago
|
||
masayuki : Can you please review the fixed attachment?
Reporter | ||
Comment 24•6 years ago
|
||
Hi, sorry for the delay to review due to my vacation. I'm back now.
Reporter | ||
Comment 25•6 years ago
|
||
Comment on attachment 8745100 [details] MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki You shouldn't mark patches r? to r+ by yourself.
Attachment #8745100 -
Flags: review+ → review?(masayuki)
Reporter | ||
Updated•6 years ago
|
Attachment #8745101 -
Flags: review+ → review?(masayuki)
Reporter | ||
Comment 26•6 years ago
|
||
Comment on attachment 8745100 [details] MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki https://reviewboard.mozilla.org/r/48843/#review47977 > Summary: > - Bug 1259666 - Part 2 - change to panDirection to mPanDirection - r?masayuki > + Bug 1259666 - Part 2 -fix change to panDirection to mPanDirection - r?masayuki > Description: > ~ Bug 1259666 - Part 2 - change to panDirection to mPanDirection - r?masayuki > ~ Bug 1259666 - Part 2 -fix change to panDirection to mPanDirection - r?masayuki Don't modify the commit summary with unnecessary "fix". Please revert the message with using |hg histedit| (rewrite "pick" to "m" for modifying the commit message). Then, please do |hg push review| again. Then, I can land them with using the autoland tool of ReviewBoard.
Attachment #8745100 -
Flags: review?(masayuki) → review+
Reporter | ||
Comment 27•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki https://reviewboard.mozilla.org/r/48845/#review47979 And don't modify the commit message to "fix". The commit message is important for the other developers who investigate the change history of each line. ::: dom/events/EventStateManager.cpp:2767 (Diff revision 2) > + > panDirection = WidgetGestureNotifyEvent::ePanHorizontal; > + > displayPanFeedback = false; > + Don't insert unnecessary lines here. ::: dom/events/EventStateManager.cpp:2778 (Diff revision 2) > + > panDirection = WidgetGestureNotifyEvent::ePanVertical; > + ditto. ::: dom/events/EventStateManager.cpp:2787 (Diff revision 2) > break; > } > > if (scrollbarVisibility & nsIScrollableFrame::HORIZONTAL) { > - panDirection = WidgetGestureNotifyEvent::ePanHorizontal; > + panDirection = WidgetGestureNotifyEvent::ePanHorizontal; > + ditto. ::: dom/events/EventStateManager.cpp:2796 (Diff revision 2) > + > aEvent->mPanDirection = panDirection; > + > + ditto.
Attachment #8745101 -
Flags: review?(masayuki)
Reporter | ||
Updated•6 years ago
|
Attachment #8745101 -
Flags: review-
Assignee | ||
Comment 28•6 years ago
|
||
for patch 2 do I have to just change commit message?
Assignee | ||
Comment 29•6 years ago
|
||
for patch 2 do I have to just change commit message?
Reporter | ||
Comment 30•6 years ago
|
||
Yes. Commit messages are important information for the other developers when they investigate the change history of the file. Therefore, we need to make commit message better as far as possible.
Assignee | ||
Comment 31•6 years ago
|
||
Comment on attachment 8745100 [details] MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48843/diff/2-3/
Attachment #8745100 -
Attachment description: MozReview Request: Bug 1259666 - Part 2 -fix change to panDirection to mPanDirection - r?masayuki → MozReview Request: fix
Attachment #8745101 -
Flags: review- → review?(masayuki)
Assignee | ||
Comment 32•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48845/diff/2-3/
Assignee | ||
Comment 33•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48845/diff/3-4/
Attachment #8745101 -
Attachment description: MozReview Request: Bug 1259666 Part 3 fix r?masayuki → MozReview Request: fix
Attachment #8745101 -
Attachment is obsolete: true
Attachment #8745101 -
Flags: review?(masayuki)
Attachment #8745101 -
Attachment is obsolete: false
Assignee | ||
Comment 34•6 years ago
|
||
Can please you review the patches. I have changed the commit message to "fix" for both patches and deleted the extra spaces
Reporter | ||
Comment 35•6 years ago
|
||
(In reply to kshitija from comment #34) > Can please you review the patches. I have changed the commit message to > "fix" for both patches and deleted the extra spaces No, no, "fix" isn't clear what you did. The first commit message when you requested the first review is the best (e.g., like the part 1). # I'll check the patches later.
Assignee | ||
Comment 36•6 years ago
|
||
Comment on attachment 8745100 [details] MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48843/diff/3-4/
Attachment #8745100 -
Attachment description: MozReview Request: fix → MozReview Request: Bug 1259666 - Part 2 - change to panDirection to mPanDirection - r?masayuki
Attachment #8745101 -
Attachment description: MozReview Request: fix → MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki
Attachment #8745101 -
Flags: review?(masayuki)
Assignee | ||
Comment 37•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48845/diff/4-5/
Assignee | ||
Comment 38•6 years ago
|
||
Changed the commit messages of both patches
Reporter | ||
Comment 39•6 years ago
|
||
https://reviewboard.mozilla.org/r/48843/#review49648 Thanks, but I think |change to| should be |Change| or |Rename|.
Reporter | ||
Comment 40•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki https://reviewboard.mozilla.org/r/48845/#review49650 Thanks. Could you rechange the part2's commit message again?
Attachment #8745101 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 41•6 years ago
|
||
Comment on attachment 8745100 [details] MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48843/diff/4-5/
Attachment #8745100 -
Attachment description: MozReview Request: Bug 1259666 - Part 2 - change to panDirection to mPanDirection - r?masayuki → MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki
Assignee | ||
Comment 42•6 years ago
|
||
Comment on attachment 8745101 [details] MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48845/diff/5-6/
Assignee | ||
Comment 43•6 years ago
|
||
Changed it
Comment 44•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/11116e242958 https://hg.mozilla.org/integration/mozilla-inbound/rev/f42bfe716797 https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6d43968b34
Comment 45•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11116e242958 https://hg.mozilla.org/mozilla-central/rev/f42bfe716797 https://hg.mozilla.org/mozilla-central/rev/1f6d43968b34
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•