Bug 223277 – Many media/media-fragments/ tests are crashing due to other tests that run before them.

• Component: Media
• Status: RESOLVED FIXED

[Truitt Savell]

Bugs like: https://bugs.webkit.org/show_bug.cgi?id=222277

are making this directory very difficult to work around. These tests are causing issues on EWS and CQ. Jer proposed we skip all these tests while he works on a workaround.

[Radar WebKit Bug Importer]

<rdar://problem/75492360>

[Truitt Savell]

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.

[Truitt Savell]

Committed: https://trac.webkit.org/changeset/274511/webkit

[Jer Noble]

Created attachment 423407 [details]
Patch

[Eric Carlson]

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;

[Darin Adler]

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!

[Jer Noble]

(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.

[Jer Noble]

(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.

[Darin Adler]

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.

[Jer Noble]

Created attachment 423634 [details]
Patch for landing

[Jer Noble]

Created attachment 423645 [details]
Patch for landing

[EWS]

contributors.json is malformed: Expecting ',' delimiter: line 626 column 16 (char 13043)

[EWS]

commit-queue failed to commit attachment 423645 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

commit-queue failed to commit attachment 423645 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

commit-queue failed to commit attachment 423645 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r274734: <https://commits.webkit.org/r274734>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423645 [details].