All users were logged out of Bugzilla on October 13th, 2018

Clean up WidgetMouseEventBase

NEW
Unassigned

Status

()

P4
normal
3 years ago
2 months ago

People

(Reporter: masayuki, Unassigned, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: [good first bug][tpi:-][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

See bug 1259654 comment 0 for the detail.
Mentor: masayuki

Comment 1

3 years ago
I want to do the required , please assign this bug to me.
Hi, I'd like you to do following things in separated changesets (patches):

1. Rename WidgetMouseEventBase::buttonType to WidgetMouseEventBase::ButtonType and explicitly define its type as int16_t with names the type as WidgetMouseEventBase::ButtonTypeType. I.e., the definition should be:

typedef int16_t ButtonTypeType;
enum ButtonType : ButtonTypeType
{
  ...
};
ButtonType button;

2. Rename WidgetMouseEventBase::buttonsFlag to WidgetMouseEventBase::ButtonsFlag and make its type int16_t named as WidgetMouseEventBase::ButtonsFlagType. However, this defines the bit flags of buttons member. So, the definition should be:

typedef int16_t ButtonFlagType;
enum ButtonFlag : ButtonFlagType
{
  ...
};
int16_t buttons;

3. Rename WidgetMouseEventBase::relatedTarget to WidgetMouseEventBase::mRelatedTarget;
4. Rename WidgetMouseEventBase::buttons to WidgetMouseEventBase::mButtons
5. Rename WidgetMouseEventBase::button to WidgetMouseEventBase::mButton
6. Rename WidgetMouseEventBase::pressure to WidgetMouseEventBase::mPressure
7. Rename WidgetMouseEventBase::hitCluster to WidgetMouseEventBase::mHitCluster
8. Rename WidgetMouseEventBase::inputSource to WidgetMouseEventBase::mInputSource
9. Rename WidgetMouseEventBase::region to WidgetMouseEventBase::mRegion
10. Reorder the member definition for reducing the instance size:
  1. mRelatedTarget
  2. mRegion
  3. mPressure
  4. mButton
  5. mButtons
  6. mHitCluster
  Please don't forget to reorder the initialization list of the constructors.


However, unfortunately, your changes will conflict with the changes of bug 1259661. So, please wait to start on this bug until bug 1259661 is fixed.
Assignee: nobody → deepeshiitb15
Depends on: 1259661
Whiteboard: [good first bug][for Gecko Inside #7 in Mozilla Japan] → [good first bug]

Updated

3 years ago
No longer depends on: 1259661
deepesh00:

Do you still want to try to fix this bug? Bug 1259661 is now fixed. So, you can refer the patches of bug 1259661 when you try to fix this.
Flags: needinfo?(deepeshiitb15)

Comment 4

2 years ago
masayuki:
I would like to get started on this. Do your comments at the beginning still apply. What exactly do I have to refer in bug 1259661 ?

Comment 5

2 years ago
masayuki: needinfo?

Comment 6

2 years ago
Hey masayuki:
needinfo?
Flags: needinfo?(masayuki)
See comment 2.

However, now, we can use |enum class| because of coding rule changes.

Therefore, now, |enum ButtonType : ButtonTypeType| should be defined as enum class outside WidgetMouseEventBase (I think that ButtonTypeType should be defined in EventForwards.h and enum class ButtonType should be forward in it, but the definition should be in MouseEvents.h, though). Unfortunately, we cannot make |ButtonFlag| as enum class because its items are used as constant values of interger.
Assignee: deepeshiitb15 → nobody
Flags: needinfo?(masayuki)
Flags: needinfo?(deepeshiitb15)

Updated

2 years ago
Priority: -- → P4
Whiteboard: [good first bug] → [good first bug][tpi:-]
Depends on: 1295098
I separated the WidgetMouseEventBase::buttonType fix to bug 1295098.

So, this bug should treat only renaming and reordering the members.

Comment 9

2 years ago
Hi.
I am interested in working on this bug. As far as I see, I only need to treat the renaming and reordering the members, as you recommended in the comment 2?
Right. #2 ~ #10.6 should be fixed in this bug.

Comment 11

2 years ago
Okay. I already renamed the members and reordered the members of WidgetMouseEventBase class, inside of MouseEvents.h, according to your description made in the #2 message. The next step will be refactoring the sources by using these new names, as necessary. How can I share with you the changes I made to MouseEvents.h? Thanks!
Did you commit each of them separately? If not, it's too difficult to review them.

If you commit them separately, you can push the commits to MozReview after you set up SSH connection to reviewboard-hg.mozilla.org: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install-mercurial.html#configuring-the-auto-review-repository

Then, you can push the changesets with |hg push -r <first-changeset-revision>/<last-changeset-revision> review|. (If you saw some errors at pushing, you'd need to set up something more, though. Perhaps, |./mach mercurial-setup| helps you.)

If you cannot push them with some errors, let me know what message you see.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8801470 [details]
Rename WidgetMouseEventBase::buttonType to WidgetMouseEventBase::ButtonType and explicitly define its type as int16_t with names the type as WidgetMouseEventBase::ButtonTypeType

https://reviewboard.mozilla.org/r/86224/#review84874

Comment 16

2 years ago
mozreview-review
Comment on attachment 8801471 [details]
Bug 1259660 - Clean up WidgetMouseEventBase

https://reviewboard.mozilla.org/r/86226/#review84876

Updated

2 years ago
Attachment #8801470 - Flags: review?(masayuki)
Attachment #8801471 - Flags: review?(masayuki)
Comment hidden (mozreview-request)

Updated

2 years ago
Attachment #8801472 - Flags: review?(masayuki)

Comment 18

2 years ago
Ok, I finally managed to push my changes on Mozilla ReviewBoard. I added you as reviewer.Please let me know if everything is ok. Thanks!
(Reporter)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8801470 [details]
Rename WidgetMouseEventBase::buttonType to WidgetMouseEventBase::ButtonType and explicitly define its type as int16_t with names the type as WidgetMouseEventBase::ButtonTypeType

https://reviewboard.mozilla.org/r/86224/#review84934

::: widget/MouseEvents.h:107
(Diff revision 1)
> -  enum buttonType
> +  typedef int16_t ButtonTypeType;
> +  enum ButtonType : ButtonTypeType
>    {
>      eLeftButton   = 0,
>      eMiddleButton = 1,
>      eRightButton  = 2
>    };
>    // Pressed button ID of mousedown or mouseup event.
>    // This is set only when pressing a button causes the event.
> -  int16_t button;
> +  ButtonType button;

As I mentioned above, you shouldn't touch buttonType in this bug because it should be an enum class independent from WidgetMouseEventBase and making it needs a lot of code change.  So, please discard this change from your repository.
Attachment #8801470 - Flags: review?(masayuki) → review-
(Reporter)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8801471 [details]
Bug 1259660 - Clean up WidgetMouseEventBase

https://reviewboard.mozilla.org/r/86226/#review84936

::: widget/MouseEvents.h:107
(Diff revision 1)
>    }
>  
>    /// The possible related target
>    nsCOMPtr<nsISupports> relatedTarget;
>  
> +  // Defined ButtonTypeType as an int16_t.

I don't understand why didn't you merge this changeset to the previous changeset. But anyway, you should discard this changeset from the fix of this bug.

FYI: When you needs to modify a commit, you can merge new changes with an existing commit with following steps:

When you have following change sets and you want to modify changeset 2, then,

@ changeset 3
|
* changeset 2
|
* changeset 1

1. hg up 2
2. change something.
3. hg commit -m "fix"

Then, the tree becomes:

@changeset 4 "fix"
|
| * changeset 3
| |
|/
* changeset 2
|
* changeset 1

Then, you need to make change set 3 as a child of 4.

4. hg rebase -s 3 -d 4

* changeset 3
|
@ changeset 4 "fix"
|
* changeset 2
|
* changeset 1

Then, try to merge 4 with 2.
5. hg up tip && hg histedit 2
6. (then, you see changesets in your text editor)
7. Change "pick" to "r" of the line of "fix"
7. Save the file and close the text editor

Then, the new commit is merged with the first changeset which you specified at #1.

I have following aliases to do this easier in my .hgrc:

> [alias]
> modify = !hg bookmark -r "children(.)" fixchild -f && hg bookmark fixthis && hg commit -m "fix" && hg rebase -s "bookmark(fixchild)" -d "bookmark(fixthis)" && hg up tip && hg histedit "parents(bookmark(fixthis))" && hg up "bookmark(fixthis)" && hg bookmark -d fixchild && hg bookmark -d fixthis
> modifyleaf = !hg bookmark fixthis && hg commit -m "fix" && hg histedit "parents(bookmark(fixthis))" && hg bookmark -d fixthis

|hg modify| is usable with above case. |hg modifyleaf| is usable when you modify a leaf changeset (i.e., not necessary to rebase existing descendants before |hg histedit|).

If you see some errors at |hg histedit|, you should add following aliases and try to fix it. This could occur when you try to modify a changeset one of whose descendant has two or more children.

> retrymodify = !hg up tip && hg histedit "parents(bookmark(fixthis))" && hg up "bookmark(fixthis)" && hg bookmark -d fixchild && hg bookmark -d fixthis
Attachment #8801471 - Flags: review?(masayuki) → review-
(Reporter)

Comment 21

2 years ago
mozreview-review
Comment on attachment 8801472 [details]
Bug 1259660 - Clean up WidgetMouseEventBase

https://reviewboard.mozilla.org/r/86228/#review84946

Okay but you need to modify the changeset summary because it's not clear what the changeset does.

It should be "Bug 1259660 part.1 Rename WidgetMouseEventBase::buttonsFlag to WidgetMouseEventBase::ButtonsFlag and make its type to WidgetMouseEventBase::ButtonsFlagType r?masayuki".

You can modify the commit message with using |hg histedit| and modify "pick" to "m" when you see changeset list in your text editor.

I'd like to check new patch. So, temporarily marking this r-.

Then, could you remove the above two changes and add 8 changesets for #3 ~ #10? You can remove the two changes with following command:

(I assume that your tree is following tree now)

@ tip (this changeset)
|
* (changeset which only adds the comment)
|
* (changeset which touched WidgetMouseEventBase::ButtonType)
|
* central

1. hg rebase -s tip -d central
2. hg strip <revision of "changeset which touched WidgetMouseEventBase::ButtonType">

Then, the tree becomes:

@ tip (this changeset)
|
* central

If you removed necessary changesets unexpectedly, you can restore them with |hg unbundle|.  When you do |hg strip|, you see comment which creates backup as bundle file.
Attachment #8801472 - Flags: review?(masayuki) → review-
I'd like to work on this bug if its still open.
I am a beginner and just catching up to things...

Updated

5 months ago
Whiteboard: [good first bug][tpi:-] → [good first bug][tpi:-][lang=c++]

Comment 23

3 months ago
Is this a good first bug to work on. If it is then i'm interested in working on it. I may need help with it.

Comment 24

3 months ago
(In reply to Gowry Sugathan from comment #23)
> Is this a good first bug to work on. If it is then i'm interested in working
> on it. I may need help with it.

Yes it's available to work on. Comment 2 has details on what needs to be done, but there are already patches up that just need to be finished. Masayuki, can you help Gowry Sugathan get started?
Flags: needinfo?(masayuki)
Almost all of what should be fixed in this bug is explained at comment 2, but that, our coding rules have been changed. So, I added comment 7. So, this bug should fix #3 ~ #10 (And if there are new members which do not start with "m" prefix, they should be fixed too).

The old patches ignored my comment 7, and they don't include the fix of #3 ~ #10. So, you should ignore the patches and you need to write patches from zero.

I'd like new hackers learn "how to rename class members safely" here. So, each change should be an independent changeset. I.e., you'll request review for 8 patches finally.
Flags: needinfo?(masayuki)
(Reporter)

Updated

3 months ago
Attachment #8801470 - Attachment is obsolete: true
(Reporter)

Updated

3 months ago
Attachment #8801471 - Attachment is obsolete: true
(Reporter)

Updated

3 months ago
Attachment #8801472 - Attachment is obsolete: true
Unfortunately, Mozilla is changing review tools from MozReview and Sprinter to Phabricator. However, Phabricator still looks unstable especially for attaching multiple patches for a bug. So, I recommend to use MozReview for now:
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html
FYI: Each patch should become like patches for bug 1254755. The bug does same thing for WidgetKeyboardEvent.

Comment 28

2 months ago
i want to work on this bug but as i have seen the previous history of comments .it seems like most of the work listed in #2 comment is done.so please let me know the current status what work is still pending to work on.
Nothing has been done... And see comment 27, now, you'll need a lot of unnecessary work for attaching multiple patches to a bug.

Comment 31

2 months ago
so can you guide me from where to get started on this bug.

Comment 32

2 months ago
Sir i have created a patch using mercurial .But Mozreview is no longer taking review request .So can you guide me how to submit patch for review here at bugzilla .i am unable to find something relevant to submit patch here.

Comment 33

2 months ago
Created attachment 8999546 [details] [diff] [review]
I have done the first to parts of this bug 1259660

Sir ,i have done the first 2 parts in this patch.In case of any problem please let me know.
Flags: needinfo?(masayuki)
Attachment #8999546 - Flags: superreview?(masayuki)
Attachment #8999546 - Flags: review?(masayuki)
Comment on attachment 8999546 [details] [diff] [review]
I have done the first to parts of this bug 1259660

Sorry for the delay to reply.

In (really) most cases, we don't use superreview anymore. So, if you get r+ from a review who is module owner or a peer, it's enough to land unless your patch crosses module boundaries.

Reivew tool is, currently, MozReview is unavailable for new. Attaching file like this is available, but it's hard to review without review tools in this bug since inline diff is important for reviewer (checking unexpected changes in each change line). So, I'd like you to create an environemnt to use Phabricator. However, it's still difficult to create for now. Installation guide is here:
https://phabricator.services.mozilla.com/book/phabricator/
but it's big document, and there is no convenient tools for now. (I hope, somebody will support to create such environment with ./mach bootstrap)
If you use Windows, perhaps, you need to install WSL. I don't hear that somebody succeeded to install the tools without WSL.
Flags: needinfo?(masayuki)
Attachment #8999546 - Flags: superreview?(masayuki)
Comment on attachment 8999546 [details] [diff] [review]
I have done the first to parts of this bug 1259660

>diff --git a/widget/MouseEvents.h b/widget/MouseEvents.h
>--- a/widget/MouseEvents.h
>+++ b/widget/MouseEvents.h
>@@ -135,19 +135,22 @@ public:
>   {
>     MOZ_CRASH("WidgetMouseEventBase must not be most-subclass");
>   }
>-
>-  enum buttonType
>+  typedef int16_t ButtonTypeType;
>+  enum ButtonType : ButtonTypeType
>   {
>     eNoButton     = -1,
>     eLeftButton   = 0,
>     eMiddleButton = 1,
>     eRightButton  = 2
>   };
>+  ButtonType button;
>   // Pressed button ID of mousedown or mouseup event.
>-  // This is set only when pressing a button causes the event.
>+  // This is set only when pressing a button causes the event.  
>   int16_t button;

Please change the type of button to ButtonType. Then, you need to update nsGUIEventIPC.h too.
https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/widget/nsGUIEventIPC.h#141,148,159

>-  enum buttonsFlag {
>+  typedef int16_t ButtonFlagType;
>+  enum ButtonsFlag : ButtonFlagType
>+  {
>     eNoButtonFlag     = 0x00,
>     eLeftButtonFlag   = 0x01,
>     eRightButtonFlag  = 0x02,
>@@ -159,8 +162,8 @@ public:
>     // mice, see "buttons" attribute document of DOM3 Events.
>     e5thButtonFlag    = 0x10
>   };
>-
>-  // Flags of all pressed buttons at the event fired.
>+  
>+ // Flags of all pressed buttons at the event fired.
>   // This is set at any mouse event, don't be confused with |button|.
>   int16_t buttons;

But as I wrote in comment 2, this should be keep int16_t.

However, please separate the file for each change in comment 2 since change only one thing is better for making simpler patch.

And I have another idea now. It may be better to move these enums to EventForwards.h and make these names are MouseButton and MouseButtonsFlag. Then, we may be able to reduce the dependency of MouseEvents.h when it's required for another header file. e.g., https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/widget/windows/nsWindow.h#236-237

Could you do that?

Comment 36

2 months ago
so,for now my work is to move these two enums from ButtonType and ButtonsFlag ,from MouseEvents.h and EventForwards.h
and also mark changes in it's dependent file nsGUIEventIPC.h.

Is thee anything more you are expecting.

Comment 37

2 months ago
Also after comparing both files MouseEvents.h and EventForwards.h .both the enums are inside WidgetMouseEventBase class so do i need to re-implement the class in EventForwards.h for moving these enums

Comment 38

2 months ago
also for working on this can you provide me some documentation ,if available or not.
I meant that renaming mozilla::WidgetMosueEventBase::buttonType in MouseEvents.h to mozilla::MouseButton in EventForwards.h, and mozilla::WidgetMouseEventBase::buttonsFlag to mozilla::MouseButtonsFlag.

Then, any referrer in header files do not need to include MouseEvents.h only for referring them (instead, they need to include EventForwards.h). Then, when you change MouseEvents.h, the rebuild speed becomes faster. (FYI: The rebuild speed of *Events.h is really slow due to a lot of files refer the header files.)

And here is coding style guide:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

I think that there is no more documents for this bug.
You need to log in before you can comment on or make changes to this bug.