Closed Bug 1158425 Opened 5 years ago Closed 5 years ago

Rename _SYNTH event names

Categories

(Core :: DOM: Events, defect)

defect
Not set

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)

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
We should also audit uses of these to ensure the right constant is being used in the right place.
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?
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.
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.
Flags: needinfo?(enndeakin)
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)
Attached patch _SYNTH.patch (obsolete) — Splinter Review
Here is my first patch.
Flags: needinfo?(enndeakin)
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)
(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.
Attached patch _SYNTH.patch (obsolete) — Splinter Review
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)
Attached patch _SYNTH.patch (obsolete) — Splinter Review
Attachment #8598576 - Attachment is obsolete: true
I submitted this to the try server. Results will appear at:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=412deaf94ccc
Flags: needinfo?(enndeakin)
Attached patch _SYNTH.patch.v2 (obsolete) — Splinter Review
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)
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)
(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)
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)
(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.
Attachment #8598678 - Flags: review?(bugs)
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+
Attached patch _SYNTH.patch.v3Splinter Review
Attachment #8598678 - Attachment is obsolete: true
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)
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)
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/94eb248b77b0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Anthony, good work on your first patch!
(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.