[webkit-reviews] review denied: [Bug 177355] [GTK] Initial webm support in MSE : [Attachment 321540] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Alicia Boya García
<aboya at igalia.com>'s request for review:
Bug 177355: [GTK] Initial webm support in MSE
https://bugs.webkit.org/show_bug.cgi?id=177355

Attachment 321540: Patch

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




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 321540
  --> https://bugs.webkit.org/attachment.cgi?id=321540
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:143
>> +	    ASSERT_NOT_REACHED();
> 
> This is the only concern I have. I suppose AppendPipeline wouldn't get
created if the type doesn't match one of the two conditions?

A test here would be interesting if there is none. Feel free to submit it to
the spec tests if there is none about this yet. If it is not accepted because
of whatever, I think it still pays off to keep it as WebKit test. I guess the
test would consist of creating a SourceBuffer with crap as type, try appending
some allowed format and try to play it, expecting an error.

If the test fails already without calling this constructor, then we are good to
go with the ASSERT_NOT_REACHED. If not, I agree with Žan here. I think the
proper thing to do here would be to setting the AppendPipeline to Invalid
state.

Then I see another problem coming and it is that apparently all state
transitions from Invalid are "ok" and I think it shouldn't be like that. Only
Invalid -> Invalid should be allowed. Everything else should be ok = false. I
guess then we would have some ASSERTs in Debug mode that we'd have to fix in
cases where somebody appends something to a pipeline in Invalid state. Am I
missing anything?