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.
Created attachment 230355 [details] Patch
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.
Created attachment 230413 [details] Revised to only call JS methods, not reprocess script each time.
<rdar://problem/16652874>
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 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.
Committed r167962: <http://trac.webkit.org/changeset/167962>
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 ]
It looks like it's not properly recognizing the ".hidden" class attribute added to the node. I'm testing a fix now.
Follow-up fix checked in as: Committed r167975: <http://trac.webkit.org/changeset/167975>.
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.
Re-opened since this is blocked by bug 132376
> 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.
Created attachment 230503 [details] Patch
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.
Committed r168036: <http://trac.webkit.org/changeset/168036>