[webkit-reviews] review denied: [Bug 94395] [Chrome] Create a toggle button for closed captions : [Attachment 160846] change test to use testRunner and remove default attr

[bugzilla-daemon at webkit.org]

Anna Cavender <annacc at chromium.org> has denied Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 94395: [Chrome] Create a toggle button for closed captions
https://bugs.webkit.org/show_bug.cgi?id=94395

Attachment 160846: change test to use testRunner and remove default attr
https://bugs.webkit.org/attachment.cgi?id=160846&action=review

------- Additional Comments from Anna Cavender <annacc at chromium.org>
Thanks so much.  I'll try your suggestions and upload another patch.

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


>> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:168
>> +	if (document->page()->theme()->supportsClosedCaptioning()) {
> 
> I think you need to override the default return false in all the
RenderThemeChromium* that support this.
> However, I don't think it's enough - the button doesn't show on my machine,
but the test passes. Is there
> any other Chrome patch waiting to land? Or maybe some other places that need
these kind of updates.

Thank you, Victor!!  Looks like RenderThemeMac was already supporting closed
captioning, so I didn't even realize I needed to do this.

>> LayoutTests/media/video-controls-captions.html:10
>> +	    if (window.testRunner) {
> 
> I think you can remove this check - this is already done in video-test.js

Done.

>> LayoutTests/media/video-controls-captions.html:28
>> +			testRunner.notifyDone();
> 
> Nit: would be nice to log the exception that was catched, so we know what
happened.

Smart.	Thanks.

>> LayoutTests/media/video-controls-captions.html:49
>> +		    failTest();
> 
> *Optional*: I would fail early and remove the if {} else {}

Done.

>> LayoutTests/media/video-controls-captions.html:71
>> +		    checkCaptionsDisplay();
> 
> Nit: would you mind changing the indentation to be just 4 spaces?

Done.