Bug 185680 – Improve NowPlaying "title"

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Improve NowPlaying title

[Radar WebKit Bug Importer]

<rdar://problem/40296700>

[Eric Carlson]

Created attachment 340504 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 340504 [details]
Patch

Clearing flags on attachment: 340504

Committed r231866: <https://trac.webkit.org/changeset/231866>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 340504 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:7459
> +    if (hasAttributeWithoutSynchronization(titleAttr)) {
> +        auto title = attributeWithoutSynchronization(titleAttr);
> +        if (!title.isEmpty())
> +            return title;
> +    }

This does an unnecessary double hash table lookup. To make it a single hash table lookup, just leave out the call to hasAttributeWithoutSynchronization since we will get a null string in that case, which returns true for "isEmpty".

It it also *slightly* strange that we special case the empty string without stripping whitespace or anything like that, but I guess that’s OK.

> Source/WebCore/html/HTMLMediaElement.cpp:7465
> +    return m_currentSrc.host();

I think we want to do IDN decoding here. So we need a call to decodeHostName.

[Darin Adler]

Comment on attachment 340504 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:7465
>> +    return m_currentSrc.host();
> 
> I think we want to do IDN decoding here. So we need a call to decodeHostName.

We also want the smart host name formatting that removes "www." -- not sure where that logic lives and if it’s even available in WebKit.

[Eric Carlson]

Comment on attachment 340504 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:7459
>> +    }
> 
> This does an unnecessary double hash table lookup. To make it a single hash table lookup, just leave out the call to hasAttributeWithoutSynchronization since we will get a null string in that case, which returns true for "isEmpty".
> 
> It it also *slightly* strange that we special case the empty string without stripping whitespace or anything like that, but I guess that’s OK.

Good points, thanks for the suggestions.

>>> Source/WebCore/html/HTMLMediaElement.cpp:7465
>>> +    return m_currentSrc.host();
>> 
>> I think we want to do IDN decoding here. So we need a call to decodeHostName.
> 
> We also want the smart host name formatting that removes "www." -- not sure where that logic lives and if it’s even available in WebKit.

decodeHostName is in WebCoreNSURLExtras so we can on macOS and iOS. 

It looks like topPrivatelyControlledDomain removes the "www." etc.

[Eric Carlson]

Reopening to attach new patch.

[Eric Carlson]

Created attachment 340762 [details]
Respond to post-review comments

[EWS Watchlist]

Comment on attachment 340762 [details]
Respond to post-review comments

Attachment 340762 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7728106

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-aux-frame-navigation.sub.html
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
imported/w3c/web-platform-tests/html/browsers/origin/relaxing-the-same-origin-restriction/document_domain_setter.html
fast/dom/beforeload/image-before-load-innerHTML.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/cross_origin_parentage.html
media/unsupported-rtsp.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/Document.currentScript.html

[EWS Watchlist]

Created attachment 340763 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

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

[EWS Watchlist]

Comment on attachment 340762 [details]
Respond to post-review comments

Attachment 340762 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7728166

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-aux-frame-navigation.sub.html
fast/dom/beforeload/frame-before-load.html
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
imported/w3c/web-platform-tests/html/browsers/origin/relaxing-the-same-origin-restriction/document_domain_setter.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/cross_origin_parentage.html
security/contentSecurityPolicy/plugins-types-allows-youtube-plugin-replacement.html
media/unsupported-rtsp.html
security/contentSecurityPolicy/plugins-types-blocks-quicktime-plugin-replacement-without-mime-type.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/Document.currentScript.html

[EWS Watchlist]

Created attachment 340764 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 340762 [details]
Respond to post-review comments

Attachment 340762 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7728198

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-aux-frame-navigation.sub.html
fast/dom/beforeload/frame-before-load.html
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
imported/w3c/web-platform-tests/html/browsers/origin/relaxing-the-same-origin-restriction/document_domain_setter.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/cross_origin_parentage.html
security/contentSecurityPolicy/plugins-types-allows-youtube-plugin-replacement.html
media/unsupported-rtsp.html
security/contentSecurityPolicy/plugins-types-blocks-quicktime-plugin-replacement-without-mime-type.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/Document.currentScript.html

[EWS Watchlist]

Created attachment 340766 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340762 [details]
Respond to post-review comments

Attachment 340762 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7728210

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-aux-frame-navigation.sub.html
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
platform/ios/ios/plugin/youtube-flash-plugin-iframe-no-height-or-width.html
imported/w3c/web-platform-tests/html/browsers/origin/relaxing-the-same-origin-restriction/document_domain_setter.html
fast/dom/beforeload/image-before-load-innerHTML.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/cross_origin_parentage.html
security/contentSecurityPolicy/plugins-types-allows-youtube-plugin-replacement.html
media/unsupported-rtsp.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/Document.currentScript.html

[EWS Watchlist]

Created attachment 340767 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Eric Carlson]

Created attachment 340769 [details]
Respond to post-review comments

[WebKit Commit Bot]

Comment on attachment 340769 [details]
Respond to post-review comments

Clearing flags on attachment: 340769

Committed r231996: <https://trac.webkit.org/changeset/231996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185799

[Eric Carlson]

Created attachment 340786 [details]
Respond to post-review comments

[WebKit Commit Bot]

Comment on attachment 340786 [details]
Respond to post-review comments

Clearing flags on attachment: 340786

Committed r232001: <https://trac.webkit.org/changeset/232001>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

The test added with this change is a flaky failure on the bot, 
( it has been flaky since it was added)

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fmedia%2Fnow-playing-info.html

--- /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/http/tests/media/now-playing-info-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/http/tests/media/now-playing-info-actual.txt
@@ -25,6 +25,7 @@
 EXPECTED (nowPlayingState.registeredAsNowPlayingApplication == 'true') OK
 EXPECTED (nowPlayingState.title == 'Video Title') OK
 EXPECTED (nowPlayingState.duration > '0') OK
+EXPECTED (nowPlayingState.elapsedTime == '0'), OBSERVED '0.000986996' FAIL
 
 * Clear video and page titles, page domain should be used.
 RUN(video.title = "")

[Eric Carlson]

Created attachment 340880 [details]
Fix flaky test.

[Ryan Haddad]

Reopening so the test fix patch can be landed.

[WebKit Commit Bot]

Comment on attachment 340880 [details]
Fix flaky test.

Clearing flags on attachment: 340880

Committed r232026: <https://trac.webkit.org/changeset/232026>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.