Closed
Bug 1153734
Opened 10 years ago
Closed 10 years ago
Rename Animation to KeyframeEffect (and merge in AnimationEffect)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
Details
(Keywords: dev-doc-needed)
Attachments
(5 files, 2 obsolete files)
17.86 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
51.40 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
59.09 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
17.68 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
13.99 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Just WIP at the moment. Still need:
Part 3 - Add AnimationEffectReadonly as an abstract interface for KeyframeEffectReadonly
Part 4 - Rename 'source' member of AnimationPlayer to 'effect'
Assignee | ||
Comment 4•10 years ago
|
||
Most of this is fairly obvious. However, the addition of 'override' to
ElementPropertyTransition::Name() is not strictly necessary. It was simply added
because while making these changes I accidentally dropped the 'virtual' keyword
from the method in the superclass and the compiler didn't complain. Adding this
will hopefully make it harder to create the same bug in the future.
Attachment #8592105 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8591523 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
We define KeyframeEffectReadonly in KeyframeEffect.cpp since Web Animations also
defines KeyframeEffect and when we come to implement that I expect we'll define
it in the same class, maybe even using the same object.
This patch also adds a few missing includes in places where
KeyframeEffectReadonly is used so that we're not just cargo-culting it in.
Attachment #8592114 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8591525 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
There are still some other references to "source" in AnimationPlayer such as
HasInPlayerSource and UpdateSourceContent. These are renamed in a subsequent
patch (that doesn't require DOM peer review).
Attachment #8592117 -
Flags: review?(bugs)
Assignee | ||
Comment 7•10 years ago
|
||
This patch also tightens up a one or two references to 'target effect' replacing
them with just 'effect'. This is because 'target effect' is longer and easily
confused with 'target element'. 'effect' should be sufficient. 'target element'
is a term from the Web Animations specification and in that context, simply
referring to the 'effect' would sound a little odd.
Attachment #8592118 -
Flags: review?(jwatt)
Assignee | ||
Comment 8•10 years ago
|
||
This patch also replaces some tabs with spaces in KeyframeEffect.h because I was
in the area.
REVIEW: Please let me know if there's a more lightweight way of doing this.
Simply in order order to add this superinterface which does nothing I seem to
have to drop the native refcounting, inherit from nsISupports.
I ran into the same problem when adding AnimationTimeline as a superinterface of
DocumentTimeline.
Attachment #8592122 -
Flags: review?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
Updated•10 years ago
|
Attachment #8592118 -
Flags: review?(jwatt) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8592105 [details] [diff] [review]
part 1 - Remove AnimationEffect
>+ void GetName(nsString& aRetVal) const {
{ goes to its own line
>+ // Alternative to GetName that returns a reference to the member for
>+ // more efficient internal usage.
>+ virtual const nsString& Name() const {
ditto
Attachment #8592105 -
Flags: review?(bugs) → review+
Comment 11•10 years ago
|
||
Comment on attachment 8592114 [details] [diff] [review]
part 2 - Rename Animation to KeyframeEffectReadonly
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#HeaderFile
works too, and might be nicer than to add anything to Bindings.conf
Attachment #8592114 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8592117 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8592122 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfcda525dbd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e58cc9b9f92
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b6e873a6bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4694e8ae6dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/787fc1b170e3
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cfcda525dbd3
https://hg.mozilla.org/mozilla-central/rev/1e58cc9b9f92
https://hg.mozilla.org/mozilla-central/rev/d1b6e873a6bc
https://hg.mozilla.org/mozilla-central/rev/a4694e8ae6dd
https://hg.mozilla.org/mozilla-central/rev/787fc1b170e3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•10 years ago
|
Keywords: dev-doc-needed
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•