Closed Bug 1259878 Opened 4 years ago Closed 4 years ago

Animation code uses isSome() when it doesn't need to

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: birtles, Assigned: hiro)

Details

Attachments

(1 file)

In dom/animation we use isSome() when we could just use operator bool to make the code easier to read:

For example:

  https://dxr.mozilla.org/mozilla-central/search?q=path%3Adom%2Fanimation%2F+isSome()&redirect=false&case=true
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Comment on attachment 8735042 [details]
MozReview Request: Bug 1259878 - remove unnecessary isSome(). r?birtles

https://reviewboard.mozilla.org/r/42525/#review39045

::: dom/animation/KeyframeEffect.cpp:1075
(Diff revision 1)
>  
>      // Create a JS object with the BaseComputedKeyframe dictionary members.
>      BaseComputedKeyframe keyframeDict;
>      keyframeDict.mOffset.SetValue(entry->mOffset);
>      keyframeDict.mComputedOffset.Construct(entry->mOffset);
> -    if (entry->mTimingFunction && entry->mTimingFunction->isSome()) {
> +    if (entry->mTimingFunction && *(entry->mTimingFunction)) {

I think the existing code is actually easier to read in this case.
Attachment #8735042 - Flags: review?(bbirtles)
https://reviewboard.mozilla.org/r/42525/#review39045

> I think the existing code is actually easier to read in this case.

Thanks!
Comment on attachment 8735042 [details]
MozReview Request: Bug 1259878 - remove unnecessary isSome(). r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42525/diff/1-2/
Attachment #8735042 - Flags: review?(bbirtles)
Comment on attachment 8735042 [details]
MozReview Request: Bug 1259878 - remove unnecessary isSome(). r?birtles

https://reviewboard.mozilla.org/r/42525/#review39049

Thanks you, looks good!
Attachment #8735042 - Flags: review?(bbirtles) → review+
https://hg.mozilla.org/mozilla-central/rev/3a90ca63a3f0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.