Remove legacy event type constants

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
These constants were defined only for captureEvents/releaseEvents. They have no useful purpose anymore.
(Assignee)

Comment 1

5 years ago
Created attachment 752413 [details] [diff] [review]
patch
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #752413 - Flags: review?(bugs)
webkit does have those constants. I couldn't figure out why they were added.
Do you think you could check why they added those constants (long ago).
(Assignee)

Comment 4

5 years ago
Created attachment 753816 [details] [diff] [review]
patch v2

(In reply to Olli Pettay [:smaug] from comment #2)
> webkit does have those constants. I couldn't figure out why they were added.

Oh, I forgot WebKit wouldn't stick some properties until an actual instance is created, and I didn't realize that WebKit supported some of our constants.
Fortunately (?), their constants are unsigned short. So they don't support constants whose value is larger than 65535.

(In reply to Olli Pettay [:smaug] from comment #3)
> Do you think you could check why they added those constants (long ago).

Dunno, it backs to the KHTML-era. For now, I'm leaving constants smaller than 65536 to be safe.
Attachment #752413 - Attachment is obsolete: true
Attachment #752413 - Flags: review?(bugs)
Attachment #753816 - Flags: review?(bugs)
Flags: needinfo?(VYV03354)
It is not clear to me what the patch helps. Would be good to get rid of all the constants, but that is
a bit risky.

Does Trident or Presto have the constants?
(Assignee)

Comment 6

5 years ago
Trident or Presto doesn't support the property.
If you prefer to remove all of the constants, please r+ the v1 patch.
Comment on attachment 752413 [details] [diff] [review]
patch

Maybe we should try this, but needs to land very early in a cycle, so perhaps
FF25.
Also, I think FF24 will be the next ESR, so would be good to not land this in
that release.
Attachment #752413 - Flags: review+
Sorry, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ea60e04312 - I suspect that in a few hours it'll look like the mochitest-1 failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=24541486&tree=Mozilla-Inbound were on non-clobbered builds and the passes were on clobbers, and you can repush it with /CLOBBER modified (or find and fix the reason it needs a clobber), but right now we need a less-broken tree.
(Assignee)

Comment 10

5 years ago
Probably bug 873996? (Technically the uuid bump shouldn't be needed because removing constants will not change the vtable layout.)
(Assignee)

Comment 14

5 years ago
Didn't fail on local and try server. Strange...
https://tbpl.mozilla.org/?tree=Try&rev=f8fb00e437a3
(Assignee)

Comment 15

5 years ago
Landed with touching the CLOBBER file. I hope it works...
https://hg.mozilla.org/integration/mozilla-inbound/rev/61be0e4a8df6
https://hg.mozilla.org/mozilla-central/rev/61be0e4a8df6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
releaseEvents have been restored as Bug 673919 has been backed out. How about this?
(Assignee)

Comment 18

5 years ago
If the releaseEvents property is absent, |window.releaseEvent();| will throw. This is the reason the removal broke the compatibility despite that the method is no-op.
However, |window.releaseEvent(Event.MOUSEDOWN);| will not throw even if it is undefined. Also, IE 11 didn't add event type constants.
So I believe legacy event type constants are not needed for Web compat.
Got it.
You need to log in before you can comment on or make changes to this bug.