Clean up WidgetGestureNotifyEvent

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: masayuki, Assigned: kshitijakarkar, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox49 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(3 attachments, 3 obsolete attachments)

I would like to work on this bug. Can you help me with it?
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
Whiteboard: [good first bug][for Gecko Inside #7 in Mozilla Japan] → [good first bug]
The first part of patch
Attachment #8744755 - Flags: review-
Second part of patch
Third part of patch
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
Attachment #8744757 - Flags: review?(masayuki)
Attachment #8744755 - Flags: review- → review?(masayuki)
Attachment #8744758 - Flags: review?(masayuki)
(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.
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)
Pushed changes onto review repository finally. Please check
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)
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...
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)
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.
I have to keep panDirection and displayPanFeedback from EventStateManager.cpp because they are local. Is that what you mean?
Exactly. "m" prefix is used only when the variable is a member of the class or struct.
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)
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/
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.
Can you please review patch 2 and patch 3 ?
Attachment #8745100 - Flags: review?(masayuki) → review+
Attachment #8745101 - Flags: review?(masayuki) → review+
masayuki :

Can you please review the fixed attachment?
Hi, sorry for the delay to review due to my vacation. I'm back now.
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)
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+
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)
for patch 2 do I have to just change commit message?
for patch 2 do I have to just change commit message?
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.
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)
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/
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
Can please you review the patches. I have changed the commit message to "fix" for both patches and deleted the extra spaces
(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.
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)
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/
Changed the commit messages of both patches
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+
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
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/
Changed it
You need to log in before you can comment on or make changes to this bug.