1259666 - Clean up WidgetGestureNotifyEvent

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See bug 1259654 comment 0 for the detail.

Comment 1

[kshitija]

I would like to work on this bug. Can you help me with it?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 3

[kshitija]

Attachment: chgSet_1.diff - Part 1 to rename ePanDirection and change definition

The first part of patch

Comment 4

[kshitija]

Attachment: chgSet_2.diff - renamed variable

Second part of patch

Comment 5

[kshitija]

Attachment: chgSet_3.diff - renamed displayPanFeedback

Third part of patch

Comment 6

[kshitija]

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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

Comment 8

[kshitija]

(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.

Comment 9

[kshitija]

Attachment: MozReview Request: Bug 1259666 part 1 - rename variable and definition change - r?masayuki

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

Comment 10

[kshitija]

Attachment: MozReview Request: Bug 1259666 - Part 2 - Change panDirection to mPanDirection - r?masayuki

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

Comment 11

[kshitija]

Attachment: MozReview Request: Bug 1259666 - Part 3- change displayPanFeedback to mDisplayPanFeedback - r?masayuki

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

Comment 12

[kshitija]

Pushed changes onto review repository finally. Please check

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 17

[kshitija]

I have to keep panDirection and displayPanFeedback from EventStateManager.cpp because they are local. Is that what you mean?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Exactly. "m" prefix is used only when the variable is a member of the class or struct.

Comment 19

[kshitija]

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/

Comment 20

[kshitija]

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/

Comment 21

[kshitija]

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.

Comment 22

[kshitija]

Can you please review patch 2 and patch 3 ?

Comment 23

[kshitija]

masayuki :

Can you please review the fixed attachment?

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hi, sorry for the delay to review due to my vacation. I'm back now.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 28

[kshitija]

for patch 2 do I have to just change commit message?

Comment 29

[kshitija]

for patch 2 do I have to just change commit message?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 31

[kshitija]

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/

Comment 32

[kshitija]

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 33

[kshitija]

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/

Comment 34

[kshitija]

Can please you review the patches. I have changed the commit message to "fix" for both patches and deleted the extra spaces

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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 36

[kshitija]

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/

Comment 37

[kshitija]

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/

Comment 38

[kshitija]

Changed the commit messages of both patches

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/48843/#review49648

Thanks, but I think |change to| should be |Change| or |Rename|.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 41

[kshitija]

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/

Comment 42

[kshitija]

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/

Comment 43

[kshitija]

Changed it

Comment 44

[Pulsebot]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/11116e242958
https://hg.mozilla.org/mozilla-central/rev/f42bfe716797
https://hg.mozilla.org/mozilla-central/rev/1f6d43968b34