Bug 172881

Summary: Modify Netflix controlsManager quirk to prevent only scrubbing
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, buildbot, eric.carlson, jer.noble, thorton, wenson_hsieh
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch aestes: review+

Description Beth Dakin 2017-06-02 16:37:19 PDT
Modify Netflix controlsManager quirk to prevent only scrubbing

rdar://problem/32228660
Comment 1 Beth Dakin 2017-06-02 16:57:14 PDT
Created attachment 311887 [details]
Patch
Comment 2 Build Bot 2017-06-02 17:00:15 PDT
Attachment 311887 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:518:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:522:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:526:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:529:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:529:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:530:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:59:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andy Estes 2017-06-02 17:15:53 PDT
Comment on attachment 311887 [details]
Patch

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

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:533
> +    String host = document->url().host();
> +    return !(equalLettersIgnoringASCIICase(host, "netflix.com") || host.endsWithIgnoringASCIICase(".netflix.com"));

Not new to this patch, but the double string comparison makes me a little sad. In the past I've done something like this:

    String netflixDomain = ASCIILiteral("netflix.com");
    if (!host.endsWithIgnoringASCIICase(netflixDomain))
        return true;

    unsigned suffixOffset = host.length() - netflixDomain.length();
    return suffixOffset && host[suffixOffset - 1] != '.';
Comment 4 Wenson Hsieh 2017-06-02 19:24:43 PDT
Does this mean that Now Playing scrubber controls will now be enabled again for Netflix?
Comment 5 Eric Carlson 2017-06-03 08:26:14 PDT
(In reply to Andy Estes from comment #3)
> Comment on attachment 311887 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311887&action=review
> 
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:533
> > +    String host = document->url().host();
> > +    return !(equalLettersIgnoringASCIICase(host, "netflix.com") || host.endsWithIgnoringASCIICase(".netflix.com"));
> 
> Not new to this patch, but the double string comparison makes me a little
> sad. In the past I've done something like this:
> 
>     String netflixDomain = ASCIILiteral("netflix.com");
>     if (!host.endsWithIgnoringASCIICase(netflixDomain))
>         return true;
> 
>     unsigned suffixOffset = host.length() - netflixDomain.length();
>     return suffixOffset && host[suffixOffset - 1] != '.';

Isn't it possible for host.length to be smaller than netflixDomain.length?
Comment 6 Andy Estes 2017-06-03 11:45:41 PDT
(In reply to Eric Carlson from comment #5)
> (In reply to Andy Estes from comment #3)
> > Comment on attachment 311887 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=311887&action=review
> > 
> > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:533
> > > +    String host = document->url().host();
> > > +    return !(equalLettersIgnoringASCIICase(host, "netflix.com") || host.endsWithIgnoringASCIICase(".netflix.com"));
> > 
> > Not new to this patch, but the double string comparison makes me a little
> > sad. In the past I've done something like this:
> > 
> >     String netflixDomain = ASCIILiteral("netflix.com");
> >     if (!host.endsWithIgnoringASCIICase(netflixDomain))
> >         return true;
> > 
> >     unsigned suffixOffset = host.length() - netflixDomain.length();
> >     return suffixOffset && host[suffixOffset - 1] != '.';
> 
> Isn't it possible for host.length to be smaller than netflixDomain.length?

We already know that host ends with netflixDomain at this point, so it must be at least as long as netflixDomain.
Comment 7 Beth Dakin 2017-06-03 12:24:43 PDT
(In reply to Andy Estes from comment #3)
> Comment on attachment 311887 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311887&action=review
> 
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:533
> > +    String host = document->url().host();
> > +    return !(equalLettersIgnoringASCIICase(host, "netflix.com") || host.endsWithIgnoringASCIICase(".netflix.com"));
> 
> Not new to this patch, but the double string comparison makes me a little
> sad. In the past I've done something like this:
> 
>     String netflixDomain = ASCIILiteral("netflix.com");
>     if (!host.endsWithIgnoringASCIICase(netflixDomain))
>         return true;
> 
>     unsigned suffixOffset = host.length() - netflixDomain.length();
>     return suffixOffset && host[suffixOffset - 1] != '.';

Cool, I'll try this! Thanks Andy!
Comment 8 Beth Dakin 2017-06-03 12:25:17 PDT
(In reply to Wenson Hsieh from comment #4)
> Does this mean that Now Playing scrubber controls will now be enabled again
> for Netflix?

It means that Netflix will have all of the controls, but if a user tries to use the timeline to scrub, nothing will happen.
Comment 9 Beth Dakin 2017-06-05 13:30:46 PDT
https://trac.webkit.org/changeset/217791/webkit