Bug 132320 – [Mac, iOS] Support caption activation via JS webkitHasClosedCaptions method

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

After recent changes to support using our JavaScript media controls implementation, we found that some sites that relied on activating/deactivating captions via the "webkitHasClosedCaptions" method no longer worked properly.

This patch corrects the logic for these cases, allowing them to use the new JS-based code path to enable/disable captions.

[Brent Fulgham]

Created attachment 230355 [details]
Patch

[Eric Carlson]

Comment on attachment 230355 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:3724
> +bool HTMLMediaElement::callMediaControlsJSFunction(const String& functionName)
> +{
> +    LOG(Media, "HTMLMediaElement::callMediaControlsJSFunction");
> +    Page* page = document().page();
> +    if (!page)
> +        return false;
> +    
> +    String mediaControlsScript = RenderTheme::themeForPage(page)->mediaControlsScript();
> +    if (!mediaControlsScript.length())
> +        return false;
> +    
> +    DOMWrapperWorld& world = ensureIsolatedWorld();

Won't this recreates the script each time it is called? Instead, I think you want to do this the same way we update the script every time the page scale changes. See the static function setPageScaleFactorProperty in HTMLMediaElement.

[Brent Fulgham]

Created attachment 230413 [details]
Revised to only call JS methods, not reprocess script each time.

[Brent Fulgham]

<rdar://problem/16652874>

[Eric Carlson]

Comment on attachment 230413 [details]
Revised to only call JS methods, not reprocess script each time.

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

> Source/WebCore/ChangeLog:3
> +        [Mac, iOS] Support caption activation via JS webkitHasClosedCaptions methoNeed a short description (OOPS!).

Oops - "... methoNeed a short description (OOPS!)."

> Source/WebCore/html/HTMLMediaElement.cpp:3719
> +    if (!m_mediaControlsHost)
> +        m_mediaControlsHost = MediaControlsHost::create(this);

It is created in didAddUserAgentShadowRoot, so I think we are in deep trouble if we get here and m_mediaControlsHost is NULL, so an ASSERT might be more appropriate.

[Brent Fulgham]

Comment on attachment 230413 [details]
Revised to only call JS methods, not reprocess script each time.

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

>> Source/WebCore/ChangeLog:3
>> +        [Mac, iOS] Support caption activation via JS webkitHasClosedCaptions methoNeed a short description (OOPS!).
> 
> Oops - "... methoNeed a short description (OOPS!)."

Whoops!

>> Source/WebCore/html/HTMLMediaElement.cpp:3719
>> +        m_mediaControlsHost = MediaControlsHost::create(this);
> 
> It is created in didAddUserAgentShadowRoot, so I think we are in deep trouble if we get here and m_mediaControlsHost is NULL, so an ASSERT might be more appropriate.

Good point. I'll change that.

[Brent Fulgham]

Committed r167962: <http://trac.webkit.org/changeset/167962>

[Andreas Kling]

Regressions: Unexpected text-only failures (8)
  fast/hidpi/video-controls-in-hidpi.html [ Failure ]
  fast/layers/video-layer.html [ Failure ]
  media/audio-controls-rendering.html [ Failure ]
  media/controls-after-reload.html [ Failure ]
  media/controls-strict.html [ Failure ]
  media/controls-without-preload.html [ Failure ]
  media/media-controls-clone.html [ Failure ]
  media/video-no-audio.html [ Failure ]

[Brent Fulgham]

It looks like it's not properly recognizing the ".hidden" class attribute added to the node. I'm testing a fix now.

[Brent Fulgham]

Follow-up fix checked in as:

Committed r167975: <http://trac.webkit.org/changeset/167975>.

[Alexey Proskuryakov]

It looks almost certain that the follow-up broke a number of other tests: http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r167977%20(5615)/results.html

I don't know how this is possible, but bot data appears definitive. I'm going to roll out both changes, and if that doesn't help, I'll roll them back in.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 132376

[Alexey Proskuryakov]

> I don't know how this is possible

Actually, I do. The follow-up change affected all elements with class="hidden", not just media controls.

[Brent Fulgham]

Created attachment 230503 [details]
Patch

[Brent Fulgham]

All tests that failed with earlier versions of this patch now pass:

1. run-webkit-tests media

	Found 693 tests; running 573, skipping 120.
	Running 8 DumpRenderTrees in parallel.

	[328/573] media/media-controls-invalid-url.html passed unexpectedly             
                                                                                
	572 tests ran as expected, 1 didn't:


	Expected to fail, but passed: (1)
	  media/media-controls-invalid-url.html

2. run-webkit-tests fast/layers

	Found 42 tests; running 42, skipping 0.
	Running 1 DumpRenderTree.       

	All 42 tests ran as expected.                                                   

3. run-webkit-tests fast/hidpi (flake appears to be expected)

	Found 57 tests; running 57, skipping 0.
	Running 1 DumpRenderTree.       

	[35/57] fast/hidpi/ima...ng-canvas.html failed unexpectedly (reference mismatch)
                                                                                
	Retrying 1 unexpected failure(s) ...

	Running 1 DumpRenderTree.

                                             
	56 tests ran as expected, 1 didn't:


	Unexpected flakiness: image-only failures (1)
	  fast/hidpi/image-srcset-png-canvas.html [ ImageOnlyFailure Pass ]
  
4. run-webkit-tests compositing/visibility

	Found 12 tests; running 12, skipping 0.
	Running 1 DumpRenderTree.       

	All 12 tests ran as expected.                                                   

5. run-webkit-tests fast/layers

	Found 42 tests; running 42, skipping 0.
	Running 1 DumpRenderTree.       

	All 42 tests ran as expected.                                                   

6. run-webkit-tests fast/regions

	Found 545 tests; running 542, skipping 3.
	Running 8 DumpRenderTrees in parallel.

	All 542 tests ran as expected.                                                  

7. run-webkit-tests platform/mac/accessibility

	Found 193 tests; running 191, skipping 2.
	Running 1 DumpRenderTree.       

	All 191 tests ran as expected.

[Brent Fulgham]

Committed r168036: <http://trac.webkit.org/changeset/168036>