| Summary: | [ Catalina EWS ] media/track/track-disabled-addcue.html is flaky crashing | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | ayumi_kojima | ||||||
| Component: | New Bugs | Assignee: | Eric Carlson <eric.carlson> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | alancoon, benjamin, calvaris, cdumez, changseok, cmarcelo, darin, eric.carlson, esprehn+autocc, ews-watchlist, gyuyoung.kim, mark.lam, philipj, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Mac (Intel) | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=229765 | ||||||||
| Attachments: |
|
||||||||
|
Description
ayumi_kojima
2021-08-24 11:57:16 PDT
Updated expectations: https://trac.webkit.org/changeset/281509/webkit Created attachment 436652 [details]
Patch
Comment on attachment 436652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436652&action=review > Source/WTF/wtf/SetForScope.h:64 > + , m_originalValue(restoreValue) Should this be std::forward<U>(restoreValue)? (In reply to Darin Adler from comment #4) > Comment on attachment 436652 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=436652&action=review > > > Source/WTF/wtf/SetForScope.h:64 > > + , m_originalValue(restoreValue) > > Should this be std::forward<U>(restoreValue)? Good point, thanks. Comment on attachment 436652 [details] 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. Comment on attachment 436652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436652&action=review >>>> 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) Great idea, changed. I also renamed `m_originalValue` to `m_valueToRestore` for clarity >> 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". Changed. >> 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? canLoadURL does return false for an empty url, so this check isn't necessary. >> Source/WebCore/html/HTMLTrackElement.cpp:-209 >> - track().scheduleLoad(url); > > Stray blank line here we could delete. Removed. Created attachment 436673 [details]
Patch for landing
Committed r281727 (241069@main): <https://commits.webkit.org/241069@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436673 [details]. |