Summary: | Many media/media-fragments/ tests are crashing due to other tests that run before them. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Truitt Savell <tsavell> | ||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aakash_jain, darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=222277 | ||||||||||
Attachments: |
|
Description
Truitt Savell
2021-03-16 13:41:13 PDT
I am marking the whole directory as [ Pass Failure ] for Mac which will skip these all on EWS but keep them running on the production bots. Created attachment 423407 [details]
Patch
Comment on attachment 423407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423407&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:846 > + size_t count = 0; > + auto results = fragmentParameter.splitAllowingEmptyEntries('='); > + for (auto iterator = results.begin(); iterator != results.end(); ++iterator) > + ++count; > + return count != 2; Couldn't this be just: return fragmentParameter.splitAllowingEmptyEntries('=').size != 2; Comment on attachment 423407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423407&action=review >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:846 >> + return count != 2; > > Couldn't this be just: > > return fragmentParameter.splitAllowingEmptyEntries('=').size != 2; This seems a roundabout way to check if to be sure there is exactly one "=". I think that’s what it means to split allowing empty and then count and then check that there are exactly 2. Unnecessary to count all the "=" signs to check if there are more than one; ideally we’d stop once the count hits 2. But I’ve never found we have a great idiom for that in cases like this. I think I would write this: auto hasInvalidNumberOfEqualCharacters = [] (StringView fragmentParameter) { unsigned count = 0; for (auto character : fragmentParameter.codeUnits()) { if (character == '=') { if (++count > 1) return true; } } return count != 1; } But it is sad that this function is so long! (In reply to Eric Carlson from comment #5) > Comment on attachment 423407 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423407&action=review > > Couldn't this be just: > > return fragmentParameter.splitAllowingEmptyEntries('=').size != 2; No, because a SplitResult is just an iterator pair; and the iterators aren't random access, so they can't be subtracted from one another. (In reply to Darin Adler from comment #6) > Comment on attachment 423407 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423407&action=review > > >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:846 > >> + return count != 2; > > > > Couldn't this be just: > > > > return fragmentParameter.splitAllowingEmptyEntries('=').size != 2; > > This seems a roundabout way to check if to be sure there is exactly one "=". > I think that’s what it means to split allowing empty and then count and then > check that there are exactly 2. > > Unnecessary to count all the "=" signs to check if there are more than one; > ideally we’d stop once the count hits 2. But I’ve never found we have a > great idiom for that in cases like this. > > I think I would write this: > > auto hasInvalidNumberOfEqualCharacters = [] (StringView > fragmentParameter) { > unsigned count = 0; > for (auto character : fragmentParameter.codeUnits()) { > if (character == '=') { > if (++count > 1) > return true; > } > } > return count != 1; > } > > But it is sad that this function is so long! So I avoided this solution exactly because it was too long. But because a StringResult iterator basically encapsulates that search algorithm above, I think you could get the same benefit just by re-writing the `hasInvalidNumberOfEqualCharacters` lambda: auto hasInvalidNumberOfEqualCharacters = [](StringView& fragmentParameter) { size_t count = 0; auto results = fragmentParameter.splitAllowingEmptyEntries('='); for (auto iterator = results.begin(); iterator != results.end(); ++iterator) { if (++count > 1) return false; } return count != 1; }; Actually, you may be able to do something like: auto hasInvalidNumberOfEqualCharacters = [](StringView& fragmentParameter) { auto results = fragmentParameter.splitAllowingEmptyEntries('='); auto iterator = results.begin(); return iterator == results.end() || ++iterator == results.end() || ++iterator != results.end(); } Not sure that the latter is better just because it's shorter though. Comment on attachment 423407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423407&action=review >>>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:846 >>>> + return count != 2; >>> >>> Couldn't this be just: >>> >>> return fragmentParameter.splitAllowingEmptyEntries('=').size != 2; >> >> This seems a roundabout way to check if to be sure there is exactly one "=". I think that’s what it means to split allowing empty and then count and then check that there are exactly 2. >> >> Unnecessary to count all the "=" signs to check if there are more than one; ideally we’d stop once the count hits 2. But I’ve never found we have a great idiom for that in cases like this. >> >> I think I would write this: >> >> auto hasInvalidNumberOfEqualCharacters = [] (StringView fragmentParameter) { >> unsigned count = 0; >> for (auto character : fragmentParameter.codeUnits()) { >> if (character == '=') { >> if (++count > 1) >> return true; >> } >> } >> return count != 1; >> } >> >> But it is sad that this function is so long! > > So I avoided this solution exactly because it was too long. But because a StringResult iterator basically encapsulates that search algorithm above, I think you could get the same benefit just by re-writing the `hasInvalidNumberOfEqualCharacters` lambda: > > auto hasInvalidNumberOfEqualCharacters = [](StringView& fragmentParameter) { > size_t count = 0; > auto results = fragmentParameter.splitAllowingEmptyEntries('='); > for (auto iterator = results.begin(); iterator != results.end(); ++iterator) { > if (++count > 1) > return false; > } > return count != 1; > }; > > Actually, you may be able to do something like: > > auto hasInvalidNumberOfEqualCharacters = [](StringView& fragmentParameter) { > auto results = fragmentParameter.splitAllowingEmptyEntries('='); > auto iterator = results.begin(); > return iterator == results.end() || ++iterator == results.end() || ++iterator != results.end(); > } > > Not sure that the latter is better just because it's shorter though. One peculiar thing is the & on the StringView&. I like that last one. Created attachment 423634 [details]
Patch for landing
Created attachment 423645 [details]
Patch for landing
contributors.json is malformed: Expecting ',' delimiter: line 626 column 16 (char 13043) commit-queue failed to commit attachment 423645 [details] to WebKit repository. To retry, please set cq+ flag again.
commit-queue failed to commit attachment 423645 [details] to WebKit repository. To retry, please set cq+ flag again.
commit-queue failed to commit attachment 423645 [details] to WebKit repository. To retry, please set cq+ flag again.
Committed r274734: <https://commits.webkit.org/r274734> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423645 [details]. |