Closed Bug 1259660 Opened 9 years ago Closed 6 years ago

Clean up WidgetMouseEventBase

Categories

(Core :: Widget, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox48 --- wontfix
firefox68 --- fixed

People

(Reporter: masayuki, Assigned: srujana121, Mentored)

References

(Depends on 1 open bug)

Details

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

Attachments

(9 files, 3 obsolete files)

1.45 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Mentor: masayuki
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]
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)
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 ?
masayuki: needinfo?
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)
Priority: -- → P4
Whiteboard: [good first bug] → [good first bug][tpi:-]
I separated the WidgetMouseEventBase::buttonType fix to bug 1295098. So, this bug should treat only renaming and reordering the members.
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.
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 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
Attachment #8801470 - Flags: review?(masayuki)
Attachment #8801471 - Flags: review?(masayuki)
Attachment #8801472 - Flags: review?(masayuki)
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!
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-
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-
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...
Whiteboard: [good first bug][tpi:-] → [good first bug][tpi:-][lang=c++]
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.
(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)
Attachment #8801470 - Attachment is obsolete: true
Attachment #8801471 - Attachment is obsolete: true
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.
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.
so can you guide me from where to get started on this bug.
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.
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?
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.
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
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.

Can I work on this?

Hi, I want to work on this bug. this is what I have understood.

  1. As of now 1 and 2 do not need to be solved. Only 3-10 should be solved.

  2. Part 3 is not valid any more because there was a code change and the variable(relatedTarget) now belongs to ancestor of the class and it was renamed too(to mrelatedTarget).

  3. Also we were asked to submit the bug in different changesets. Just to be clear, same patch but different changesets right?

I am applying for outreachy, I was assigned this bug. It was not allotted to me yet. Can someone allot it to me?

Also the comments said part 2 is done, but in the file MouseEvents.h , I dont see any changes. Should part 2 too is to be solved?

Also regarding part 10, am I correct in assuming that the order has to be in decreasing order of alignment size (alignof() value) to optimize the packing (but not cache because we will be working in multi processor systems) ?

Vaishnavi has agreed to give up the bug.

Assignee: nobody → Srujana.htt121

Renamed all class member instances from WidgetMouseEventBase::buttons to WidgetMouseEventBase::mButtons

To reduce the instance size, reordered the member definition in the class WidgetMouseEventBase, in the decreasing order of alignment size (alignof() value) to optimize the packing.

Renamed all class member instances from WidgetMouseEventBase::button to WidgetMouseEventBase::mButton.

Renamed all class member instances from WidgetMouseEventBase::region to WidgetMouseEventBase::mRegion

Renamed all class member instances from WidgetMouseEventBase::inputSource to WidgetMouseEventBase::mInputSource

Renamed all class member instances from WidgetMouseEventBase::hitCluster to WidgetMouseEventBase::mHitCluster

Renamed all class member instances from WidgetMouseEventBase::pressure to WidgetMouseEventBase::mPressure

Moved mozilla::WidgetMosueEventBase::buttonType in MouseEvents.h to mozilla::MouseButton in EventForwards.h, and mozilla::WidgetMouseEventBase::buttonsFlag to mozilla::MouseButtonsFlag so that any referer in header files do not need to include MouseEvents.h only for referring them. Instead, they just need to include EventForwards.h. Now when MouseEvents.h is changed, the rebuild speed becomes faster.

Keywords: checkin-needed

Wait a moment until trying to build on all platforms.

Keywords: checkin-needed

build failure on macOS:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238710570&repo=try&lineNumber=22810
build failure on Windows:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238710565&repo=try&lineNumber=24038
build failure on Android:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238710582&repo=try&lineNumber=24491

Looks like that you have not changed in widget/cocoa, widget/windows nor widget/android. You can look for them with:
https://searchfox.org/mozilla-central/search?q=symbol%3AF_%3CT_mozilla%3A%3AWidgetMouseEventBase%3E_button&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AF_%3CT_mozilla%3A%3AWidgetMouseEventBase%3E_buttons&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AF_%3CT_mozilla%3A%3AWidgetMouseEventBase%3E_pressure&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AF_%3CT_mozilla%3A%3AWidgetMouseEventBase%3E_hitCluster&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AF_%3CT_mozilla%3A%3AWidgetMouseEventBase%3E_inputSource&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonType%3E_eLeftButton&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonType%3E_eMiddleButton&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonType%3E_eRightButton&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonsFlag%3E_eNoButtonFlag&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonsFlag%3E_eLeftButtonFlag&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonsFlag%3E_eMiddleButtonFlag&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonsFlag%3E_eRightButtonFlag&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonsFlag%3E_e4thButtonFlag&case=false&regexp=false&path=widget%2F
https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWidgetMouseEventBase%3A%3AbuttonsFlag%3E_e5thButtonFlag&case=false&regexp=false&path=widget%2F

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/89c2646562d4 Cleaned up WidgetMouseEventBase by renaming WidgetMouseEventBase::buttons to WidgetMouseEventBase::mButtons r=masayuki https://hg.mozilla.org/integration/autoland/rev/912e85a1cbdf Cleaned up WidgetMouseEventBase by renaming WidgetMouseEventBase::button to WidgetMouseEventBase::mButton. r=masayuki https://hg.mozilla.org/integration/autoland/rev/cb6da071dc03 Cleaned up WidgetMouseEventBase by renaming WidgetMouseEventBase::pressure to WidgetMouseEventBase::mPressure r=masayuki https://hg.mozilla.org/integration/autoland/rev/d0d15e6fc468 Cleaned up WidgetMouseEventBase by renaming WidgetMouseEventBase::hitCluster to WidgetMouseEventBase::mHitCluster r=masayuki https://hg.mozilla.org/integration/autoland/rev/d4d915be6dcf Cleaned up WidgetMouseEventBase by renaming WidgetMouseEventBase::inputSource to WidgetMouseEventBase::mInputSource r=masayuki https://hg.mozilla.org/integration/autoland/rev/71f22c1e59a9 Cleaned up WidgetMouseEventBase by renaming WidgetMouseEventBase::region to WidgetMouseEventBase::mRegion r=masayuki https://hg.mozilla.org/integration/autoland/rev/8c32486b90c5 Reordered the member definition in the class WidgetMouseEventBase to reduce the instance size. r=masayuki https://hg.mozilla.org/integration/autoland/rev/b061de30553d Moved mozilla::WidgetMosueEventBase::buttonType in MouseEvents.h to mozilla::MouseButton in EventForwards.h, and mozilla::WidgetMouseEventBase::buttonsFlag to mozilla::MouseButtonsFlag r=masayuki
Attachment #8999546 - Flags: review?(masayuki)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: