Bug 185680 - Improve NowPlaying "title"
Summary: Improve NowPlaying "title"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on: 185799
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-16 09:44 PDT by Eric Carlson
Modified: 2018-05-21 14:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (25.80 KB, patch)
2018-05-16 11:20 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Respond to post-review comments (3.63 KB, patch)
2018-05-18 17:43 PDT, Eric Carlson
dino: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.14 MB, application/zip)
2018-05-18 18:46 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.83 MB, application/zip)
2018-05-18 18:54 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.58 MB, application/zip)
2018-05-18 19:25 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (3.06 MB, application/zip)
2018-05-18 19:32 PDT, EWS Watchlist
no flags Details
Respond to post-review comments (3.58 KB, patch)
2018-05-18 19:40 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Respond to post-review comments (3.65 KB, patch)
2018-05-19 13:41 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Fix flaky test. (3.27 KB, patch)
2018-05-21 13:44 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2018-05-16 09:44:51 PDT
Improve NowPlaying title
Comment 1 Radar WebKit Bug Importer 2018-05-16 09:45:36 PDT
<rdar://problem/40296700>
Comment 2 Eric Carlson 2018-05-16 11:20:27 PDT
Created attachment 340504 [details]
Patch
Comment 3 WebKit Commit Bot 2018-05-16 13:36:31 PDT
Comment on attachment 340504 [details]
Patch

Clearing flags on attachment: 340504

Committed r231866: <https://trac.webkit.org/changeset/231866>
Comment 4 WebKit Commit Bot 2018-05-16 13:36:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2018-05-16 18:25:34 PDT
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.
Comment 6 Darin Adler 2018-05-16 18:26:27 PDT
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.
Comment 7 Eric Carlson 2018-05-18 17:27:12 PDT
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.
Comment 8 Eric Carlson 2018-05-18 17:43:34 PDT
Reopening to attach new patch.
Comment 9 Eric Carlson 2018-05-18 17:43:35 PDT
Created attachment 340762 [details]
Respond to post-review comments
Comment 10 EWS Watchlist 2018-05-18 18:46:22 PDT
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
Comment 11 EWS Watchlist 2018-05-18 18:46:23 PDT
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
Comment 12 EWS Watchlist 2018-05-18 18:54:45 PDT
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
Comment 13 EWS Watchlist 2018-05-18 18:54:46 PDT
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
Comment 14 EWS Watchlist 2018-05-18 19:25:56 PDT
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
Comment 15 EWS Watchlist 2018-05-18 19:25:58 PDT
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
Comment 16 EWS Watchlist 2018-05-18 19:32:47 PDT
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
Comment 17 EWS Watchlist 2018-05-18 19:32:48 PDT
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
Comment 18 Eric Carlson 2018-05-18 19:40:25 PDT
Created attachment 340769 [details]
Respond to post-review comments
Comment 19 WebKit Commit Bot 2018-05-18 21:22:09 PDT
Comment on attachment 340769 [details]
Respond to post-review comments

Clearing flags on attachment: 340769

Committed r231996: <https://trac.webkit.org/changeset/231996>
Comment 20 WebKit Commit Bot 2018-05-18 21:22:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Commit Bot 2018-05-19 08:10:36 PDT
Re-opened since this is blocked by bug 185799
Comment 22 Eric Carlson 2018-05-19 13:41:59 PDT
Created attachment 340786 [details]
Respond to post-review comments
Comment 23 WebKit Commit Bot 2018-05-19 16:07:43 PDT
Comment on attachment 340786 [details]
Respond to post-review comments

Clearing flags on attachment: 340786

Committed r232001: <https://trac.webkit.org/changeset/232001>
Comment 24 WebKit Commit Bot 2018-05-19 16:07:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Dawei Fenton (:realdawei) 2018-05-21 13:06:26 PDT
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 = "")
Comment 26 Eric Carlson 2018-05-21 13:44:54 PDT
Created attachment 340880 [details]
Fix flaky test.
Comment 27 Ryan Haddad 2018-05-21 13:49:02 PDT
Reopening so the test fix patch can be landed.
Comment 28 WebKit Commit Bot 2018-05-21 14:28:21 PDT
Comment on attachment 340880 [details]
Fix flaky test.

Clearing flags on attachment: 340880

Committed r232026: <https://trac.webkit.org/changeset/232026>
Comment 29 WebKit Commit Bot 2018-05-21 14:28:23 PDT
All reviewed patches have been landed.  Closing bug.