163481 2016-10-14 20:38:49 -0700 [GTK] Restore user agent quirk for Yahoo 2016-10-16 13:37:15 -0700 1 1 1 Unclassified WebKit WebKitGTK Other PC Linux RESOLVED FIXED P2 Normal --- 1 mcatanzaro mcatanzaro bugs-noreply cgarcia commit-queue mcatanzaro oldest_to_newest 1240551 0 mcatanzaro 2016-10-14 20:38:49 -0700 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. 1240552 1 mcatanzaro 2016-10-14 20:40:21 -0700 Ooops, copypaste error. "Do this" means "display a mobile site without checking window dimensions." 1240554 2 291703 mcatanzaro 2016-10-14 20:43:39 -0700 Created attachment 291703 Patch 1240578 3 cgarcia 2016-10-15 01:37:18 -0700 (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. 1240579 4 291703 cgarcia 2016-10-15 01:38:44 -0700 Comment on attachment 291703 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? 1240605 5 mcatanzaro 2016-10-15 07:43:44 -0700 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. 1240608 6 291703 commit-queue 2016-10-15 08:06:41 -0700 Comment on attachment 291703 Patch Clearing flags on attachment: 291703 Committed r207376: <http://trac.webkit.org/changeset/207376> 1240609 7 commit-queue 2016-10-15 08:06:45 -0700 All reviewed patches have been landed. Closing bug. 1240700 8 cgarcia 2016-10-16 00:59:35 -0700 (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. 1240739 9 mcatanzaro 2016-10-16 11:12:43 -0700 (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. 1240785 10 mcatanzaro 2016-10-16 13:37:15 -0700 (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. 291703 2016-10-14 20:43:39 -0700 2016-10-15 08:06:41 -0700 Patch bug-163481-20161014224011.patch text/plain 1522 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjA3MzAyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZmQzZTlmZDQyMGE3Yjlh YWUzMjI4ZWRlMTA1MDUzMmQ2MDRmODcyNC4uYTEyZDNjZmFiOGJhNTg1NmRjMGM4MWFjMzA5YTc3 YzZhZjFhYWI5YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE2LTEwLTE0ICBNaWNo YWVsIENhdGFuemFybyAgPG1jYXRhbnphcm9AaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXSBS ZXN0b3JlIHVzZXIgYWdlbnQgcXVpcmsgZm9yIFlhaG9vCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjM0ODEKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBmaW5hbmNlLnlhaG9vLmNvbSBpcyBzZW5kaW5nIGEg bW9iaWxlIHZlcnNpb24gaW4gcmVzcG9uc2UgdG8gb3VyIHN0YW5kYXJkIHVzZXIgYWdlbnQuCisK KyAgICAgICAgKiBwbGF0Zm9ybS9ndGsvVXNlckFnZW50R3RrLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OnVybFJlcXVpcmVzTWFjaW50b3NoUGxhdGZvcm0pOgorCiAyMDE2LTEwLTEzICBDaHJpcyBE dW1leiAgPGNkdW1lekBhcHBsZS5jb20+CiAKICAgICAgICAgUmVuYW1lIFtDb25zdHJ1Y3RvclRl bXBsYXRlPSpdIHRvIFtMZWdhY3lDb25zdHJ1Y3RvclRlbXBsYXRlPSpdCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvVXNlckFnZW50R3RrLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2d0ay9Vc2VyQWdlbnRHdGsuY3BwCmluZGV4IDZlOGM0MDNhNzQ0MWNhM2Jh N2ZlYjQ2MDViNmYwN2ZlOWJhMmVkMmIuLjM4ZDRhZTZiYWFhMmVhY2ZiYjU2YWIxMTA4MjU5MDIw YWVkYmY2ZTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9Vc2VyQWdl bnRHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9Vc2VyQWdlbnRHdGsu Y3BwCkBAIC0xOTcsNiArMTk3LDEwIEBAIHN0YXRpYyBib29sIHVybFJlcXVpcmVzTWFjaW50b3No UGxhdGZvcm0oY29uc3QgVVJMJiB1cmwpCiB7CiAgICAgU3RyaW5nIGJhc2VEb21haW4gPSB0b3BQ cml2YXRlbHlDb250cm9sbGVkRG9tYWluKHVybC5ob3N0KCkpOwogCisgICAgLy8gQXQgbGVhc3Qg ZmluYW5jZS55YWhvby5jb20gZGlzcGxheXMgYSBtb2JpbGUgdmVyc2lvbiB3aXRoIG91ciBzdGFu ZGFyZCB1c2VyIGFnZW50LgorICAgIGlmIChiYXNlRG9tYWluID09ICJ5YWhvby5jb20iKQorICAg ICAgICByZXR1cm4gdHJ1ZTsKKwogICAgIC8vIHRhb2Jhby5jb20gZGlzcGxheXMgYSBtb2JpbGUg dmVyc2lvbiB3aXRoIG91ciBzdGFuZGFyZCB1c2VyIGFnZW50LgogICAgIGlmIChiYXNlRG9tYWlu ID09ICJ0YW9iYW8uY29tIikKICAgICAgICAgcmV0dXJuIHRydWU7Cg==