Closed
Bug 1276581
Opened 9 years ago
Closed 7 years ago
Rename *FromJS and *FromStyle in Animation.h
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
INVALID
People
(Reporter: mantaroh, Unassigned)
Details
(Whiteboard: [good first bug])
We can rename some of the *FromJS / *FromStyle methods to use the *NoUpdate naming.
In bug 1178662, we changed Do* method, So we should rename other name.
Reporter | ||
Comment 1•8 years ago
|
||
Before work on this, our Animation class have 2 subclass. CSSAnimation and CSSTransition.
These class is override some function from parent(Animation). In these case, we will need to rename to subclass.
e.g.
We will call PlayStateFromJS function when call setter of playState from JS code.
(https://dxr.mozilla.org/mozilla-central/search?q=PlayStateFromJS&redirect=false)
Currently, Animation have three **FromJS function.
(playStateFromJS / playFromJS / pauseFromJS)
We will need to rename these function as first step.
Whiteboard: [good first bug]
Comment 2•8 years ago
|
||
I think we should probably leave this code as-is for now.
Later we can try to do the refactoring Boris attempted in bug 1049975 and remove some of the NoUpdate() methods.
Comment 3•7 years ago
|
||
Since bug 1049975 is resolved does anyone still want to update the *FromJS / *FromStyle methods to use the *NoUpdate() call? Is this still a good first bug to fix?
Comment 4•7 years ago
|
||
For PlayStateFromJS, is it a name change to PlayStateNoUpdate()? could you explain the reason for *NoUpdate naming convention?
PauseFromJS, there is already a protected method call PauseNoUpdate(), is this just another wrapper? On a related note:
In the animation.h:
* PauseFromJS is currently only here for symmetry with PlayFromJS but
* in future we will likely have to flush style in
* CSSAnimation::PauseFromJS so we leave it for now.
What's the status of that?
CancelFromStyle, there is a protected CancelNoUpdate(), is this another change the wrapper function name?
Generally, are these methods stable enough to rename Animation.h and its subclass? any background behind some of these naming convection and history would be much appreciated. I'm trying to wrap my head around why changes are needed and then figure out how it can be done.
thanks!
Comment 5•7 years ago
|
||
Hi Chewie, sorry for not responding to your previous question. I was looking for an another good first bug instead but I haven't found one yet.
I think we should not change the naming of these method and we should resolve this bug as invalid.
I don't remember all the details behind this bug but originally we had variants of the different methods with the following qualities:
* Some flush style while others don't.
Typically we want to avoid flushing style for internal callers -- e.g. when we are updating style -- but
for *some* methods we need to flush style when called from JS.
* Some post an additional restyle and also update the layer generation, while others don't. For the Gecko code
path, if we are generating/destroying animations as part of a regular restyle, we also update animation
styles at the same time so we don't need to post a second restyle. For the Servo code path, the post
traversal task where we generation the animations will ensure we do a subsequent animation restyle. As
a result, when updating CSS animations and transitions we don't need to post an additional restyle.
* Some do extra style-specific handling (e.g. the CSS animation pause handling)
Last time I looked through this code, I recall discovering that renaming the methods here was going to be confused by some of the subtle differences behind the ways the methods work and it was better just to leave this as-is.
Unfortunately, I haven't yet found a better "good first bug"!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Comment 6•7 years ago
|
||
no worry Brian, thank you for the explanation. I was confused by the method, I noticed the differences but obviously no idea why it is.
You need to log in
before you can comment on or make changes to this bug.
Description
•