[webkit-reviews] review granted: [Bug 229462] [ Catalina EWS ] media/track/track-disabled-addcue.html is flaky crashing : [Attachment 436652] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 27 12:37:26 PDT 2021

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 229462: [ Catalina EWS ] media/track/track-disabled-addcue.html is flaky

Attachment 436652: Patch


--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 436652
  --> https://bugs.webkit.org/attachment.cgi?id=436652

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

Looks good assuming we have enough test coverage. I see various interesting
cases here in the code we are refactoring and altering subtly and not really
sure we have tests for all of them

>>> Source/WTF/wtf/SetForScope.h:64
>>> +	     , m_originalValue(restoreValue)
>> Should this be std::forward<U>(restoreValue)?
> Good point, thanks.

Was thinking further and, for flexibility, I also think that we also could use
two different types for newValue and restoreValue. The stored values will still
be of type T, but the arguments could be two different types convertible to T.

    template<typename U, typename V>
    SetForScope(T& scopedVariable, U&& newValue, V&& restoreValue)

> Source/WebCore/html/HTMLTrackElement.cpp:196
> +	   URL url = getNonEmptyURLAttribute(srcAttr);

I find the name "url" distasteful, and try to dodge it, so I would have named
this "trackURL".

> Source/WebCore/html/HTMLTrackElement.cpp:202
> +	   if (url.isEmpty() || !canLoadURL(url)) {

Do we need an empty check here? Maybe canLoadURL is guaranteed to return false
for empty?

> Source/WebCore/html/HTMLTrackElement.cpp:-209
> -    track().scheduleLoad(url);

Stray blank line here we could delete.

More information about the webkit-reviews mailing list