Closed Bug 874669 Opened 11 years ago Closed 11 years ago

Remove legacy event type constants

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: emk, Assigned: emk)

Details

Attachments

(1 file, 1 obsolete file)

These constants were defined only for captureEvents/releaseEvents. They have no useful purpose anymore.
Attached patch patch (obsolete) — Splinter Review
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).
Flags: needinfo?(VYV03354)
Attached patch patch v2Splinter Review
(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?
Trident or Presto doesn't support the property.
If you prefer to remove all of the constants, please r+ the v1 patch.
Attachment #753816 - Flags: review?(bugs)
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.
Probably bug 873996? (Technically the uuid bump shouldn't be needed because removing constants will not change the vtable layout.)
Didn't fail on local and try server. Strange...
https://tbpl.mozilla.org/?tree=Try&rev=f8fb00e437a3
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
releaseEvents have been restored as Bug 673919 has been backed out. How about this?
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.