Bug 215947 – AudioParam.cancelAndHoldAtTime() is missing

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

AudioParam.cancelAndHoldAtTime() is missing:
- https://www.w3.org/TR/webaudio/#dom-audioparam-cancelandholdattime

[Radar WebKit Bug Importer]

<rdar://problem/68362061>

[Chris Dumez]

Corresponding Blink patch: https://codereview.chromium.org/1533103002

[Chris Dumez]

Created attachment 408161 [details]
WIP Patch

[Chris Dumez]

Created attachment 408162 [details]
WIP Patch

[Chris Dumez]

Created attachment 408189 [details]
Patch

[Darin Adler]

Comment on attachment 408189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408189&action=review

r=me assuming tests pass

> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:196
> +    size_t i;
> +    // Find the first event at or just past cancelTime.
> +    for (i = 0; i < m_events.size(); ++i) {
> +        if (m_events[i]->time() > cancelTime)
> +            break;
> +    }

Vector::findMatching does this kind of thing, although it returns notFound instead of size() when not found.

> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:217
> +    Optional<UniqueRef<ParamEvent>> newEvent;
> +    Optional<UniqueRef<ParamEvent>> newSetValueEvent;

Normally we’d use unique_ptr instead of Optional<UniqueRef>.

> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:376
> +        ParamEvent& event = m_events[i].get();
> +        ParamEvent* nextEvent = i < n - 1 ? &m_events[i + 1].get() : nullptr;

auto&, auto or auto*

> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:382
> +        ParamEvent::Type nextEventType = nextEvent ? static_cast<ParamEvent::Type>(nextEvent->type()) : ParamEvent::LastType /* unknown */;

auto

> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:451
> +                {

This is brace style from another project, I think. But not just the new code.

> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:605
> +inline float AudioParamTimeline::linearRampAtTime(Seconds t, float value1, Seconds time1, float value2, Seconds time2)

Seems unlikely we have to write "inline" to get inlining here.

> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:610
> +inline float AudioParamTimeline::exponentialRampAtTime(Seconds t, float value1, Seconds time1, float value2, Seconds time2)

Ditto.

> Source/WebCore/Modules/webaudio/AudioParamTimeline.h:84
> +        static UniqueRef<ParamEvent> createSetValueEvent(float value, Seconds time)
> +        {
> +            return makeUniqueRef<ParamEvent>(ParamEvent::SetValue, value, time, 0, Seconds { }, Vector<float> { }, 0, 0, nullptr);
> +        }

Can we put most of the function bodies into the .cpp file instead of the header?

[Chris Dumez]

Comment on attachment 408189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408189&action=review

>> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:217
>> +    Optional<UniqueRef<ParamEvent>> newSetValueEvent;
> 
> Normally we’d use unique_ptr instead of Optional<UniqueRef>.

Yes, I wanted that. I could not find a nice way to convert a unique_ptr to a UniqueRef though. How do we do that? Or should I add a function to UniqueRef to construct from a unique_ptr?

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 408189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408189&action=review
> 
> >> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:217
> >> +    Optional<UniqueRef<ParamEvent>> newSetValueEvent;
> > 
> > Normally we’d use unique_ptr instead of Optional<UniqueRef>.
> 
> Yes, I wanted that. I could not find a nice way to convert a unique_ptr to a
> UniqueRef though. How do we do that? Or should I add a function to UniqueRef
> to construct from a unique_ptr?

I might be missing something but it seems we can easily go from a UniqueRef to unique_ptr but not the other way around. Seems like a potential hole in our current API?

My proposal would be to add a new static function to UniqueRef<>, e.g.

static UniqueRef fromUniquePtr(unique_ptr p);
With an assertion in there to make sure p is non-null.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Chris Dumez from comment #7)
> > Comment on attachment 408189 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=408189&action=review
> > 
> > >> Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:217
> > >> +    Optional<UniqueRef<ParamEvent>> newSetValueEvent;
> > > 
> > > Normally we’d use unique_ptr instead of Optional<UniqueRef>.
> > 
> > Yes, I wanted that. I could not find a nice way to convert a unique_ptr to a
> > UniqueRef though. How do we do that? Or should I add a function to UniqueRef
> > to construct from a unique_ptr?
> 
> I might be missing something but it seems we can easily go from a UniqueRef
> to unique_ptr but not the other way around. Seems like a potential hole in
> our current API?
> 
> My proposal would be to add a new static function to UniqueRef<>, e.g.
> 
> static UniqueRef fromUniquePtr(unique_ptr p);
> With an assertion in there to make sure p is non-null.

template<typename T>
UniqueRef<T> makeUniqueRefFromNonNullUniquePtr(std::unique_ptr<T> ptr)
{
    static_assert(std::is_same<typename T::webkitFastMalloced, int>::value, "T is FastMalloced");
    return UniqueRef<T>(*ptr.release());
}

Thoughts?

Although the Optional<UniqueRef<T>> solution in my patch was kind of nice because I did not need moveToUniquePtr() or makeUniqueRefFromNonNullUniquePtr().

[Chris Dumez]

Created attachment 408197 [details]
Patch

[EWS]

Committed r266712: <https://trac.webkit.org/changeset/266712>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408197 [details].