[webkit-reviews] review granted: [Bug 215947] AudioParam.cancelAndHoldAtTime() is missing : [Attachment 408189] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 7 11:57:26 PDT 2020


Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 215947: AudioParam.cancelAndHoldAtTime() is missing
https://bugs.webkit.org/show_bug.cgi?id=215947

Attachment 408189: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 408189
  --> https://bugs.webkit.org/attachment.cgi?id=408189
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?


More information about the webkit-reviews mailing list