Fullscreen mode on mlb.com shows white letterboxing when video aspect ratio does not match screen
<rdar://problem/68936043>
Created attachment 409908 [details] Patch
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.
(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.
s/:background/:backdrop/g
Created attachment 409916 [details] Patch for landing
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!
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.
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? :-)
(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.
Created attachment 410020 [details] Patch for landing
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.
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.
Can we separate this one new quirk from the refactoring, please?
(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.
Created attachment 410049 [details] Patch for landing
Committed r267773: <https://trac.webkit.org/changeset/267773> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410049 [details].