[Webkit-unassigned] [Bug 139075] [MSE] Unset timestamps of trackbuffers during Reset Parser State algorithm

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=139075

Jer Noble <jer.noble at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #242312|                            |review+
              Flags|                            |

--- Comment #6 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 242312
  --> https://bugs.webkit.org/attachment.cgi?id=242312
Patch with LayoutTest as suggested by Jer Noble

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

r=me, with nits in the new layout test. Please upload a new version, and I'll cq+ it.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:34
> +        nextRequest = 0;
> +

No need to re-initialize this variable here.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:40
> +    function finishTest() {

It would be better if this function came after `sourceUpdated()`.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:44
> +        /*
> +            Only first sample was sync, after abort there were only non sync samples, so they should be dropped,
> +            as trackBuffer will have needsRandomAccessPoint set.
> +        */

This comment should go in the ChangeLog, instead of in the test.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:47
> +        var droppedAmount = sampleCount - abortAfter - 1; // 1 sample will be canceled

Since this variable is just subtracting constant values from one another, you can just define this up at the top of the file with the other const values.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:61
> +        if (nextRequest < sampleCount) {
> +            sourceBuffer.appendBuffer(
> +                makeASample(nextRequest, nextRequest, 1, 1,
> +                (nextRequest == 0) ? SAMPLE_FLAG.SYNC : SAMPLE_FLAG.NONE
> +                ));
> +        }
> +        else {
> +            finishTest();
> +        }

WebKit style has you invert the "if()" logic, call finishTest(), and return early.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:65
> +        if (nextRequest == abortAfter) {
> +            sourceBuffer.abort(); // need to do this after appendBuffer, before `updateend`
> +        }

WebKit style also has you remove the braces from single-line if clauses. I don't think the comment is necessary.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:72
> +    <video controls width=960 height=510></video>

No need to specify a width and height here.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141201/b12515e0/attachment-0002.html>