Bug 165104 - Blacklist Netflix for TouchBar support
Summary: Blacklist Netflix for TouchBar support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-28 12:31 PST by Beth Dakin
Modified: 2016-12-01 14:14 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.19 KB, patch)
2016-11-28 12:33 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (3.87 KB, patch)
2016-11-28 16:13 PST, Beth Dakin
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2016-11-28 12:31:44 PST
Blacklist Netflix for TouchBar support

rdar://problem/29404778
Comment 1 Beth Dakin 2016-11-28 12:33:51 PST
Created attachment 295516 [details]
Patch
Comment 2 Darin Adler 2016-11-28 12:37:59 PST
Comment on attachment 295516 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:7142
> +static bool needsPlaybackControlsManagerQuirk(bool needsQuirks, const URL& url)

I think I would have suggested having this take a Page& argument rather than having the caller dig out the boolean and the URL.

> Source/WebCore/html/HTMLMediaElement.cpp:7148
> +    if (!needsQuirks)
> +        return false;
> +
> +    String host = url.host();
> +    return equalLettersIgnoringASCIICase(host, "www.netflix.com");

I think this would read better without the local variable. Maybe even just a one-liner:

    return needsQuirks && equalLettersIgnoringASCIICase(url.host(), "www.netflix.com");
Comment 3 Tim Horton 2016-11-28 12:38:54 PST
Comment on attachment 295516 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:7148
> +    return equalLettersIgnoringASCIICase(host, "www.netflix.com");

This could be a (host isEqualIgnoringCase "netflix.com" || host hasPrefixIgnoringCase ".netflix.com") if you don't want to depend on Netflix's redirect.
Comment 4 WebKit Commit Bot 2016-11-28 13:43:15 PST
Comment on attachment 295516 [details]
Patch

Clearing flags on attachment: 295516

Committed r209013: <http://trac.webkit.org/changeset/209013>
Comment 5 WebKit Commit Bot 2016-11-28 13:43:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Beth Dakin 2016-11-28 16:11:46 PST
Re-opening since the blacklisting failed to cover Now Playing.
Comment 7 Beth Dakin 2016-11-28 16:13:20 PST
Created attachment 295548 [details]
Patch
Comment 8 Beth Dakin 2016-11-28 16:18:34 PST
Thanks Tim https://trac.webkit.org/changeset/209045
Comment 9 Darin Adler 2016-12-01 14:06:33 PST
Hi Beth. What we ended up landing didn’t match Tim’s suggestion, and will return false if Netflix ever uses "netflix.com"; might want to return and clean that up either by removing the "www.netflix.com" check (which is redundant) or changing it to "netflix.com" (taking Tim’s suggestion more literally).
Comment 10 Beth Dakin 2016-12-01 14:09:24 PST
(In reply to comment #9)
> Hi Beth. What we ended up landing didn’t match Tim’s suggestion, and will
> return false if Netflix ever uses "netflix.com"; might want to return and
> clean that up either by removing the "www.netflix.com" check (which is
> redundant) or changing it to "netflix.com" (taking Tim’s suggestion more
> literally).

Oops!
Comment 11 Beth Dakin 2016-12-01 14:14:50 PST
Committed follow-up: https://trac.webkit.org/changeset/209206