Closed Bug 1193019 Opened 4 years ago Closed 4 years ago

Rename CSSFontFaceLoadEvent to FontFaceSetLoadEvent

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: heycam)

References

()

Details

Attachments

(1 file)

When the font loading stuff was enabled on release builds in bug 1149381 and merged to beta it broke a bunch of test_interfaces.html tests.  While verifying all the newly exposed globals were correct I noticed that CSSFontFaceLoadEvent is misnamed.  Its now called FontFaceSetLoadEvent in the spec.  This was changed over a year ago:

  https://github.com/w3c/csswg-drafts/commit/48353a72f7bc4bb9cd91102f3a2ce25f7e73b669
Can we fix this before shipping in release?
Flags: needinfo?(jdaggett)
Flags: needinfo?(cam)
Thanks for spotting this, Ben.
Flags: needinfo?(jdaggett)
Flags: needinfo?(cam)
Attached patch patchSplinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8646177 - Flags: review?(khuey)
Comment on attachment 8646177 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1028497
[User impact if declined]: script that attempts to use the actual event interface name could break
[Describe test coverage new/current, TreeHerder]: relevant tests updated in this patch
[Risks and why]: low; we are just changing an IDL interface name
[String/UUID change made/needed]: N/A
Attachment #8646177 - Flags: approval-mozilla-beta?
Attachment #8646177 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/82b0246cf577
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Jorge, this patch has IDL changes and I wanted to get your comment on whether it has a UUID change impact. Is it ok to approve the uplift to Beta on this one? Thanks.
Flags: needinfo?(jorge)
Changing UUIDs is for .idl (xpidl) files, not .webidl (WebIDL) files.
(That said, it is a Web facing feature change, and I'm not sure what our policy is about uplifting them to beta.)
Yes, as far as add-on compat is concerned, this one isn't an issue.
Flags: needinfo?(jorge)
(In reply to David Baron [:dbaron] (busy Aug. 8-Aug. 30) from comment #11)
> (That said, it is a Web facing feature change, and I'm not sure what our
> policy is about uplifting them to beta.)

We don't have a strict policy. The approach that we should follow is to be informed about the ramifications of these changes. Are we likely to break a lot of web content? Are we likely to break high profile web content? In what way would content break with this change? More generally, what fallout do we expect from this change?
It's very unlikely anyone is relying on the old name.  For one thing, the CSS Font Loading API isn't yet available on release (but will be when 41 is released).  For another, this just affects people who are either (a) manually creating an event object using |new CSSFontFaceLoadEvent(...)|, or (b) checking event object instances with |evt instanceof CSSFontFaceLoadEvent|.  I think it's pretty safe to uplift this.
Comment on attachment 8646177 [details] [diff] [review]
patch

Taking it into beta, this is early in the cycle and we want to release a feature matching the spec.
Attachment #8646177 - Flags: approval-mozilla-beta?
Attachment #8646177 - Flags: approval-mozilla-beta+
Attachment #8646177 - Flags: approval-mozilla-aurora?
Attachment #8646177 - Flags: approval-mozilla-aurora+
Sherriffs who land this: please update the commit message from r=smaug to r=khuey, thanks. :-)
Removing dev-doc-needed. CSSFontFaceLoadEvent didn't reached a release nor was documented as such in MDN (FontFaceLoadEvent is not yet documented)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.