[GTK] Restore user agent quirk for Yahoo

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

What happened here was:

 * Yahoo used to do this on yahoo.com
 * We decided Yahoo lost the right to receive an accurate user agent, and added a user agent quirk to pretend to be OS X on yahoo.com (and apparently also on subdomains).
 * At some point in the past, Yahoo stopped doing this on yahoo.com
 * I removed the user agent quirk in 2.14.1, because it was no longer needed for yahoo.com, and I assumed Yahoo had regained the right to receive an accurate user agent.

I assumed wrongly. I didn't think to check finance.yahoo.com, which is still sending a mobile version in response to our user agent. Our policy is to add user agent quirks for any websites dumb enough to do this, to the detriment of anyone using WebKit on mobile devices.

[Michael Catanzaro]

Ooops, copypaste error. "Do this" means "display a mobile site without checking window dimensions."

[Michael Catanzaro]

Created attachment 291703 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #0)
> What happened here was:
> 
>  * Yahoo used to do this on yahoo.com
>  * We decided Yahoo lost the right to receive an accurate user agent, and
> added a user agent quirk to pretend to be OS X on yahoo.com (and apparently
> also on subdomains).
>  * At some point in the past, Yahoo stopped doing this on yahoo.com
>  * I removed the user agent quirk in 2.14.1, because it was no longer needed
> for yahoo.com, and I assumed Yahoo had regained the right to receive an
> accurate user agent.
> 
> I assumed wrongly. I didn't think to check finance.yahoo.com, which is still
> sending a mobile version in response to our user agent. Our policy is to add
> user agent quirks for any websites dumb enough to do this, to the detriment
> of anyone using WebKit on mobile devices.

That's right, maybe we should try to detect if we are running on a mobile device and not apply these particular quirks.

[Carlos Garcia Campos]

Comment on attachment 291703 [details]
Patch

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

You also removed the code in the unit tests checking this quirk, please bring it back too.

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:202
> +    // At least finance.yahoo.com displays a mobile version with our standard user agent.
> +    if (baseDomain == "yahoo.com")
> +        return true;

Wouldn't it be possible to do this only for finance, since we now know that it doesn't happen with other yahoo pages?

[Michael Catanzaro]

I agree, it would be good to apply these quirks only on mobile devices. Bug #163487.

(In reply to comment #4)
> You also removed the code in the unit tests checking this quirk, please
> bring it back too.

I don't think it makes sense. It's not desirable to keep the same list of quirks in both the unit test and the quirks plugin. Right now we have one test for the OS X quirk, one test for the Chrome quirk, and two Google tests since that one is implemented a bit differently. We should add a WebCore API so that the unit test can add its own quirks, that way it can test that the quirks class works properly without requiring that we update it each time we add new quirks. Bug #163488.

> > Source/WebCore/platform/gtk/UserAgentGtk.cpp:202
> > +    // At least finance.yahoo.com displays a mobile version with our standard user agent.
> > +    if (baseDomain == "yahoo.com")
> > +        return true;
> 
> Wouldn't it be possible to do this only for finance, since we now know that
> it doesn't happen with other yahoo pages?

I think it's safer to apply the quirk to all of yahoo.com, since they probably have some internal toolkit with this bug and it would not be surprising to see this problem on other Yahoo subdomains. I have not checked each Yahoo domain to see which are broken. The OS X quirk should be much safer than the Chrome quirk; the worst breakage I would expect is to see Yahoo offer Mac downloads or something.

[WebKit Commit Bot]

Comment on attachment 291703 [details]
Patch

Clearing flags on attachment: 291703

Committed r207376: <http://trac.webkit.org/changeset/207376>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(In reply to comment #5)
> I agree, it would be good to apply these quirks only on mobile devices. Bug
> #163487.
> 
> (In reply to comment #4)
> > You also removed the code in the unit tests checking this quirk, please
> > bring it back too.
> 
> I don't think it makes sense. It's not desirable to keep the same list of
> quirks in both the unit test and the quirks plugin. Right now we have one
> test for the OS X quirk, one test for the Chrome quirk, and two Google tests
> since that one is implemented a bit differently. We should add a WebCore API
> so that the unit test can add its own quirks, that way it can test that the
> quirks class works properly without requiring that we update it each time we
> add new quirks. Bug #163488.

I disagree, checking every single quirk you not only check the class works as expected but also that the UA is going to be changed for the websites you want. The goal of that unit test, was also to check that the quirks worked for different urls pointing to the same domain for example.

> > > Source/WebCore/platform/gtk/UserAgentGtk.cpp:202
> > > +    // At least finance.yahoo.com displays a mobile version with our standard user agent.
> > > +    if (baseDomain == "yahoo.com")
> > > +        return true;
> > 
> > Wouldn't it be possible to do this only for finance, since we now know that
> > it doesn't happen with other yahoo pages?
> 
> I think it's safer to apply the quirk to all of yahoo.com, since they
> probably have some internal toolkit with this bug and it would not be
> surprising to see this problem on other Yahoo subdomains. I have not checked
> each Yahoo domain to see which are broken. The OS X quirk should be much
> safer than the Chrome quirk; the worst breakage I would expect is to see
> Yahoo offer Mac downloads or something.

Ok.

[Michael Catanzaro]

(In reply to comment #8)
> I disagree, checking every single quirk you not only check the class works
> as expected but also that the UA is going to be changed for the websites you
> want. The goal of that unit test, was also to check that the quirks worked
> for different urls pointing to the same domain for example.

I really think this is going to become unwieldy as the quantity of quirks increases. We can test this well enough using test quirks. Actually, I think we should be doing more unit testing like this. Our layout tests and API tests are so good that we can't keep up when they start failing, but we have almost no unit tests.

[Michael Catanzaro]

(In reply to comment #8) 
> I disagree, checking every single quirk you not only check the class works
> as expected but also that the UA is going to be changed for the websites you
> want. The goal of that unit test, was also to check that the quirks worked
> for different urls pointing to the same domain for example.

It's probably right... bug #163508.