Closed
Bug 1158425
Opened 9 years ago
Closed 9 years ago
Rename _SYNTH event names
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: enndeakin, Assigned: darkdh, Mentored)
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 4 obsolete files)
59.00 KB,
patch
|
Details | Diff | Splinter Review |
The *_SYNTH event constants are a bit confusing. We should rename them to be clearer. NS_MOUSE_ENTER_SYNTH -> NS_MOUSE_OVER NS_MOUSE_EXIT_SYNTH -> NS_MOUSE_OUT NS_DRAGDROP_LEAVE_SYNTH -> NS_DRAGDROP_LEAVE And maybe rename NS_MOUSE_ENTER to NS_MOUSE_ENTER_WIDGET and NS_MOUSE_EXIT to NS_MOUSE_EXIT_WIDGET. Remove redundant NS_DRAGDROP_OVER_SYNTH and NS_DRAGDROP_EXIT_SYNTH
Reporter | ||
Comment 1•9 years ago
|
||
We should also audit uses of these to ensure the right constant is being used in the right place.
Assignee | ||
Comment 2•9 years ago
|
||
Hi Neil, I would like to work on this bug. So I should rename these macro in widget/BasicEvents.h and all the .h/.cpp which use these macro, am I correct?
Reporter | ||
Comment 3•9 years ago
|
||
Hi Anthony. Thanks for taking this bug. You are correct. Rename those macros in each .h and .cpp file where they occur. Let me know if you need any help.
Assignee | ||
Comment 4•9 years ago
|
||
Hi Neil, How do I test all the changes I made is no mistakes before submitting a patch? Should I clone the entire source tree and build? Or is there any document for the beginner like me who tries to contribute? Thank you.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 5•9 years ago
|
||
I'd recommend building the source, if only as an exercise in learning how to do it and if you'd like to contribute to other bugs. For this bug though, since this mostly involves replacing constant names, as long as you're confident in your editing C++ skill, you might be able to get by without it. You'll need to modify a couple of files that are specific to certain platforms though (Windows, Mac and Linux). We use an automated testing system called 'Try' that we can submit patches to and various machines run our test suites automatically. Results are posted at https://treeherder.mozilla.org/#/jobs?repo=try A key value of this to test patches on different platforms, operating system versions and configurations that you don't normally use. I assume you don't have access to our Try system so you can post a patch to this bug and I or someone else can do so for you. You can then look at the results and fix and issues that arise. We have documentation about building and testing at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 7•9 years ago
|
||
OK great. Are you ready to have it tested? When I applied it I got a merge conflict. Perhaps someone checked in some code to the same file today.
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Neil Deakin from comment #7) > OK great. Are you ready to have it tested? When I applied it I got a merge > conflict. Perhaps someone checked in some code to the same file today. I don't have the permission to perform 'Try' test. I just make sure it won't build fail. I will pull the newest code and update the patch.
Assignee | ||
Comment 9•9 years ago
|
||
After pulling from the repository, I made this patch. However, I'm afraid that there will be more conflicts if someone commits new change in this period.
Attachment #8598052 -
Attachment is obsolete: true
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8598576 -
Attachment is obsolete: true
Reporter | ||
Comment 11•9 years ago
|
||
I submitted this to the try server. Results will appear at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=412deaf94ccc
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 12•9 years ago
|
||
According to the failure summary: c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\widget\windows\nsWindow.cpp(3784) : error C2065: 'NS_MOUSE_EXIT' : undeclared identifier c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\widget\windows\nsWindow.cpp(4912) : error C2065: 'NS_MOUSE_EXIT' : undeclared identifier I found I didn't resolve the conflict completely. Here is the updated version. Sorry for the missing part.
Attachment #8598578 -
Attachment is obsolete: true
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 13•9 years ago
|
||
Here is an updated try run with the new patch, just for Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e2b3b9061c6
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Neil Deakin from comment #13) > Here is an updated try run with the new patch, just for Windows: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e2b3b9061c6 Hi Neil, How should I deal with these unclassified cases?
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 15•9 years ago
|
||
The one failure there looks like bug 1159150, so you should be ok. If you are happy with the patch you can mark it for review. I can review it if you like but you might want to ask for review and/or feedback from bugs@pettay.fi
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Neil Deakin from comment #15) > The one failure there looks like bug 1159150, so you should be ok. If you > are happy with the patch you can mark it for review. I can review it if you > like but you might want to ask for review and/or feedback from bugs@pettay.fi Thank you for your suggestion.
Assignee | ||
Updated•9 years ago
|
Attachment #8598678 -
Flags: review?(bugs)
Comment 17•9 years ago
|
||
Comment on attachment 8598678 [details] [diff] [review] _SYNTH.patch.v2 > #define NS_MOUSE_MESSAGE_START 300 > #define NS_MOUSE_MOVE (NS_MOUSE_MESSAGE_START) > #define NS_MOUSE_BUTTON_UP (NS_MOUSE_MESSAGE_START + 1) > #define NS_MOUSE_BUTTON_DOWN (NS_MOUSE_MESSAGE_START + 2) >-#define NS_MOUSE_ENTER (NS_MOUSE_MESSAGE_START + 22) >-#define NS_MOUSE_EXIT (NS_MOUSE_MESSAGE_START + 23) >+#define NS_MOUSE_ENTER_WIDGET (NS_MOUSE_MESSAGE_START + 22) >+#define NS_MOUSE_EXIT_WIDGET (NS_MOUSE_MESSAGE_START + 23) Please align (NS_MOUSE_MESSAGE_START + 22) and (NS_MOUSE_MESSAGE_START + 23) under the other similar stuff > #define NS_MOUSE_DOUBLECLICK (NS_MOUSE_MESSAGE_START + 24) > #define NS_MOUSE_CLICK (NS_MOUSE_MESSAGE_START + 27) > #define NS_MOUSE_ACTIVATE (NS_MOUSE_MESSAGE_START + 30) >-#define NS_MOUSE_ENTER_SYNTH (NS_MOUSE_MESSAGE_START + 31) >-#define NS_MOUSE_EXIT_SYNTH (NS_MOUSE_MESSAGE_START + 32) >+#define NS_MOUSE_OVER (NS_MOUSE_MESSAGE_START + 31) >+#define NS_MOUSE_OUT (NS_MOUSE_MESSAGE_START + 32) ditto >-#define NS_DRAGDROP_OVER_SYNTH (NS_DRAGDROP_EVENT_START + 1) >-#define NS_DRAGDROP_EXIT_SYNTH (NS_DRAGDROP_EVENT_START + 2) >-#define NS_DRAGDROP_LEAVE_SYNTH (NS_DRAGDROP_EVENT_START + 9) >+#define NS_DRAGDROP_LEAVE (NS_DRAGDROP_EVENT_START + 9) ditto
Attachment #8598678 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8598678 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Hi Neil, I update the patch as Olli recommended. What is the next step to push my patch into the repository? By the way, can I take this bug and become assignee? And what is the permission I need to take bugs by myself?
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 20•9 years ago
|
||
You can add the checkin-needed keyword to the 'Keywords' field and someone will check the patch in. You can assign bugs to yourself by setting the Assigned field. (Click the 'take' link). Or maybe you don't have permission? I'll set the assigned field to you for this bug. Generally to get more permissions (for bugzilla, to check in, etc), you need someone or several people to vouch for you. See https://www.mozilla.org/en-US/about/governance/policies/commit/ for more information. You will likely need to have worked on a couple of bugs with other people to get higher privileges. It looks like you could get access to the try server already at least.
Assignee: nobody → darkdh
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 21•9 years ago
|
||
(In reply to Neil Deakin from comment #20) > You can add the checkin-needed keyword to the 'Keywords' field and someone > will check the patch in. > > You can assign bugs to yourself by setting the Assigned field. (Click the > 'take' link). Or maybe you don't have permission? I'll set the assigned > field to you for this bug. > > Generally to get more permissions (for bugzilla, to check in, etc), you need > someone or several people to vouch for you. See > https://www.mozilla.org/en-US/about/governance/policies/commit/ for more > information. You will likely need to have worked on a couple of bugs with > other people to get higher privileges. It looks like you could get access to > the try server already at least. I got it. Thank you.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/94eb248b77b0
Keywords: checkin-needed
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/94eb248b77b0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Reporter | ||
Comment 24•9 years ago
|
||
Anthony, good work on your first patch!
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Neil Deakin from comment #24) > Anthony, good work on your first patch! Thank you Neil. I really appreciate your mentoring.
You need to log in
before you can comment on or make changes to this bug.
Description
•