[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]

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
crashing
https://bugs.webkit.org/show_bug.cgi?id=229462

Attachment 436652: Patch

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




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

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.