Bug 217060 – Fullscreen mode on mlb.com shows white letterboxing when video aspect ratio does not match screen

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Fullscreen mode on mlb.com shows white letterboxing when video aspect ratio does not match screen

[Jer Noble]

<rdar://problem/68936043>

[Jer Noble]

Created attachment 409908 [details]
Patch

[Darin Adler]

Comment on attachment 409908 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:1074
> +bool Quirks::needsBlackFullscreenBackgroundQuirk() const

Frustrating that unlike per-app quirks that at least have "linked on or after" rules, per-website quirks don’t come with an expiration date or any plan to eventually remove them. Do we need a comment explaining something about the context of why the quirk is added to help future people know when to remove a quirk?

> Source/WebCore/page/Quirks.cpp:1081
> +        m_needsBlackFullscreenBackgroundQuirk = equalLettersIgnoringASCIICase(host, "mlb.com") || host.endsWithIgnoringASCIICase(".mlb.com");

I wonder if this "equals or ends with (preceded by dot)" idiom should finally get its own helper function.

[Jer Noble]

(In reply to Darin Adler from comment #3)
> Comment on attachment 409908 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409908&action=review
> 
> > Source/WebCore/page/Quirks.cpp:1074
> > +bool Quirks::needsBlackFullscreenBackgroundQuirk() const
> 
> Frustrating that unlike per-app quirks that at least have "linked on or
> after" rules, per-website quirks don’t come with an expiration date or any
> plan to eventually remove them. Do we need a comment explaining something
> about the context of why the quirk is added to help future people know when
> to remove a quirk?

That's a good point; it looks like this specific problem would be mitigated if we supported the :background pseudo-element; I'll add a comment to that effect.

> > Source/WebCore/page/Quirks.cpp:1081
> > +        m_needsBlackFullscreenBackgroundQuirk = equalLettersIgnoringASCIICase(host, "mlb.com") || host.endsWithIgnoringASCIICase(".mlb.com");
> 
> I wonder if this "equals or ends with (preceded by dot)" idiom should
> finally get its own helper function.

It totally should.

[Jer Noble]

s/:background/:backdrop/g

[Jer Noble]

Created attachment 409916 [details]
Patch for landing

[Darin Adler]

Comment on attachment 409916 [details]
Patch for landing

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

> Source/WebCore/page/Quirks.cpp:77
> +static inline bool hostMatchesDomainOrSubdomains(const StringView& host, const StringView& domain)

Should just be StringView, not const StringView&.

> Source/WebCore/page/Quirks.cpp:739
> +    m_needsPreloadAutoQuirk = hostMatchesDomainOrSubdomains(m_document->url().host(), "vimeo.com");

Missing "." here. Don’t land this!

[Darin Adler]

Comment on attachment 409916 [details]
Patch for landing

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

> Source/WebCore/page/Quirks.cpp:77
> +static inline bool hostMatchesDomainOrSubdomains(const StringView& host, const StringView& domain)

Should just be StringView, not const StringView&.

No need to say inline here.

[Peng Liu]

Comment on attachment 409916 [details]
Patch for landing

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

> Source/WebCore/page/Quirks.cpp:192
> +    return hostMatchesDomainOrSubdomains(m_document->topDocument().url().host(), "youtube.com");

Missing "." as well? :-)

[Jer Noble]

(In reply to Darin Adler from comment #7)
> Comment on attachment 409916 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409916&action=review
> 
> > Source/WebCore/page/Quirks.cpp:77
> > +static inline bool hostMatchesDomainOrSubdomains(const StringView& host, const StringView& domain)
> 
> Should just be StringView, not const StringView&.
> 
> > Source/WebCore/page/Quirks.cpp:739
> > +    m_needsPreloadAutoQuirk = hostMatchesDomainOrSubdomains(m_document->url().host(), "vimeo.com");
> 
> Missing "." here. Don’t land this!

Ack! I'll re-author this to make this a static assert. This should be something that can be caught at compilation time.

[Jer Noble]

Created attachment 410020 [details]
Patch for landing

[Jer Noble]

I banged my head against C++17's support for constexpr string literals, static_assert's requirements, (lack of) support for strings as template arguments, and could not come up with a reasonable solution for doing compile-time checking of string contents.

This should get better in C++20, maybe. Until then, I rewrote the hostMatchesDomainOrSubdomains() function to not require a domain parameter starting with a period, which in addition to not imposing a weird requirement on the domain parameter, should also make the comparison faster by only doing a string equality check once.

[Darin Adler]

Comment on attachment 410020 [details]
Patch for landing

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

> Source/WebCore/page/Quirks.cpp:361
> -        auto host = url.host().convertToASCIILowercase();
> +        auto host = url.host();

Please do not land. This breaks a bunch of the code below, including the "trailers.apple.com" check and the ".naver.com" check.

[Darin Adler]

Can we separate this one new quirk from the refactoring, please?

[Jer Noble]

(In reply to Darin Adler from comment #14)
> Can we separate this one new quirk from the refactoring, please?

Sure, we can just land the original reviewed patch.

[Jer Noble]

Created attachment 410049 [details]
Patch for landing

[EWS]

Committed r267773: <https://trac.webkit.org/changeset/267773>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410049 [details].