Bug 132320

Summary: [Mac, iOS] Support caption activation via JS webkitHasClosedCaptions method
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: MediaAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kling, philipj, sergio
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 132376    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Revised to only call JS methods, not reprocess script each time.
none
Patch eric.carlson: review+

Description Brent Fulgham 2014-04-28 22:38:51 PDT
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.
Comment 1 Brent Fulgham 2014-04-28 22:48:43 PDT
Created attachment 230355 [details]
Patch
Comment 2 Eric Carlson 2014-04-29 08:34:54 PDT
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.
Comment 3 Brent Fulgham 2014-04-29 14:02:35 PDT
Created attachment 230413 [details]
Revised to only call JS methods, not reprocess script each time.
Comment 4 Brent Fulgham 2014-04-29 14:12:39 PDT
<rdar://problem/16652874>
Comment 5 Eric Carlson 2014-04-29 15:07:13 PDT
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.
Comment 6 Brent Fulgham 2014-04-29 15:18:12 PDT
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.
Comment 7 Brent Fulgham 2014-04-29 15:19:54 PDT
Committed r167962: <http://trac.webkit.org/changeset/167962>
Comment 8 Andreas Kling 2014-04-29 17:28:07 PDT
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 ]
Comment 9 Brent Fulgham 2014-04-29 17:55:14 PDT
It looks like it's not properly recognizing the ".hidden" class attribute added to the node. I'm testing a fix now.
Comment 10 Brent Fulgham 2014-04-29 18:10:52 PDT
Follow-up fix checked in as:

Committed r167975: <http://trac.webkit.org/changeset/167975>.
Comment 11 Alexey Proskuryakov 2014-04-29 22:37:03 PDT
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.
Comment 12 WebKit Commit Bot 2014-04-29 22:39:51 PDT
Re-opened since this is blocked by bug 132376
Comment 13 Alexey Proskuryakov 2014-04-29 22:47:49 PDT
> 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.
Comment 14 Brent Fulgham 2014-04-30 11:43:57 PDT
Created attachment 230503 [details]
Patch
Comment 15 Brent Fulgham 2014-04-30 11:46:36 PDT
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.
Comment 16 Brent Fulgham 2014-04-30 12:44:26 PDT
Committed r168036: <http://trac.webkit.org/changeset/168036>