Bug 132329 – [GTK] Enable skipped http/tests/media/hls/video-controls-live-stream.html

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Lorenzo Tilve]

As support for Live streams has been introduced at http://trac.webkit.org/changeset/167828 this test could be fixed and unskipped on GTK.

It's currently failing.

33EXPECTED (video.duration == 'Infinity') OK
44EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-rewind-button") == 'null') OK
5 EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-play-button") != 'null') OK
6 EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-status-display") != 'null') OK
 5EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-play-button") != 'null'), OBSERVED 'null' FAIL
 6EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-status-display") != 'null'), OBSERVED 'null' FAIL
77EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-timeline") == 'null') OK
88EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-current-time-display") == 'null') OK
99EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-remaining-time-display") == 'null') OK

[Xabier Rodríguez Calvar]

Created attachment 230483 [details]
Patch

[Martin Robinson]

Comment on attachment 230483 [details]
Patch

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

> LayoutTests/platform/mac/http/tests/media/hls/video-controls-live-stream-expected.txt:11
> +
> +EVENT(play)
> +EXPECTED (video.duration == 'Infinity') OK
> +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-rewind-button") == 'null') OK
> +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-play-button") != 'null') OK
> +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-status-display") != 'null') OK
> +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-timeline") == 'null') OK
> +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-current-time-display") == 'null') OK
> +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-remaining-time-display") == 'null') OK
> +END OF TEST
> +

You removed all the testExpected lines so aren't these results invalid now?

[Build Bot]

Comment on attachment 230483 [details]
Patch

Attachment 230483 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6241129603268608

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 230489 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 230483 [details]
Patch

Attachment 230483 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5933866770497536

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 230494 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Philippe Normand]

Yep, you can manually rebase the results from the mac EWS. That'd be great to do before landing the patch.

[Build Bot]

Comment on attachment 230483 [details]
Patch

Attachment 230483 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5714767603826688

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 230504 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> (From update of attachment 230483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230483&action=review
> 
> > LayoutTests/platform/mac/http/tests/media/hls/video-controls-live-stream-expected.txt:11
> > +
> > +EVENT(play)
> > +EXPECTED (video.duration == 'Infinity') OK
> > +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-rewind-button") == 'null') OK
> > +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-play-button") != 'null') OK
> > +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-status-display") != 'null') OK
> > +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-timeline") == 'null') OK
> > +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-current-time-display") == 'null') OK
> > +EXPECTED (mediaControlsElement(controls, "-webkit-media-controls-remaining-time-display") == 'null') OK
> > +END OF TEST
> > +
> 
> You removed all the testExpected lines so aren't these results invalid now?

Regarding this there are two things: the first is that I moved this file and that's why all these lines are going away in one side and coming in the other; you can see them in the platform/mac directory. The second is that I do need to get the new baseline for the test as Phil suggests because I was waiting for the failures to update it.

[Xabier Rodríguez Calvar]

Created attachment 230689 [details]
Patch

[Martin Robinson]

Comment on attachment 230689 [details]
Patch

The GTK+ bits look fine to me, but I think someone else should review the changes to the tests.

[Xabier Rodríguez Calvar]

(In reply to comment #12)
> (From update of attachment 230689 [details])
> The GTK+ bits look fine to me, but I think someone else should review the changes to the tests.

Eric, Jer, would you have a moment for a review?

[Xabier Rodríguez Calvar]

Brent, you reviewed fix for bug 131390, would you have a minute for this?

[Eric Carlson]

Comment on attachment 230689 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:63
> +    configureControls: function() {
> +        if (!this.controls.configured) {
> +            this.configureInlineControls();
> +            this.controls.configured = true;
> +            this.addControls();
> +        }
> +    },

Nit: you should use an early return here:

    if (this.controls.configured) 
        return;

[Xabier Rodríguez Calvar]

Created attachment 230985 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to comment #15)
> Nit: you should use an early return here:
> 
>     if (this.controls.configured) 
>         return;

Done this.

I also rebased the test because of the long time labels. I think that requires a new review.

[Eric Carlson]

Comment on attachment 230985 [details]
Patch

Thank you!

[WebKit Commit Bot]

Comment on attachment 230985 [details]
Patch

Clearing flags on attachment: 230985

Committed r168467: <http://trac.webkit.org/changeset/168467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.