Closed Bug 1162106 Opened 5 years ago Closed 5 years ago
Add top Japanese sites to unprefixing service whitelist (part 1)
These are the target domains. I need to check where the CSS is served for each before writing a patch. * shopping.yahoo.co.jp * auctions.yahoo.co.jp * travel.yahoo.co.jp * m.sports.yahoo.co.jp * ameblo.jp * goo.ne.jp * s.tabelog.jp * mobile.gnavi.co.jp * sp.daily.co.jp * www.smbc-card.com * rakuten-card.co.jp * mixi.jp * girlschannel.net * www.fancl.co.jp * s.cosme.net * sapporobeer.jp * www.mapion.co.jp * touch.navitime.co.jp * sp.mbga.jp * www.ntv.co.jp * suntory.jp * www.aeonsquare.net * mw.nikkei.com * nhk.or.jp * tokyo-sports.co.jp (Note, there may be a few more domains added to the whitelist for top Japan sites, but they depend on other things ATM).
(In reply to Mike Taylor [:miketaylr] from comment #0) > These are the target domains. I need to check where the CSS is served for > each before writing a patch. > Is this still for fennec only? I'm a bit surprised that we can't get yahoo.co.jp to do the right thing server-side.
(In reply to Jet Villegas (:jet) from comment #1) > Is this still for fennec only? I'm a bit surprised that we can't get > yahoo.co.jp to do the right thing server-side. This should just be for Fennec, yes. As for yahoo.co.jp, we'll be working with the Japan team to get that sorted out so we can remove this. AIUI, yahoo.co.jp and yahoo.com are two different beasts--I admit I don't understand all the details.
(In reply to Mike Taylor [:miketaylr] from comment #2) > (In reply to Jet Villegas (:jet) from comment #1) > > Is this still for fennec only?[...] > > This should just be for Fennec, yes. Actually, there's no "fennec only" limitation to the unprefixing service's behavior. The *goal* is to fix mobile sites, but the unprefixing is applied in Gecko, regardless of the particular product (Desktop Firefox vs Fennec vs Firefox OS) -- constrained by the domain whitelist. This is intentional, because Gecko is Gecko, and cross-platform differences only cause confusion, and things are simpler/saner if FxDesktop-with-a-mobile-UA-and-small-viewport can be expected to behave just like Fennec. This means that if foo.com is on the whitelist, then foo.com's deskop *and* mobile sites will get unprefixing treatment. Generally, this shouldn't affect the desktop rendering of foo.com, since desktop sites are more likely to have interoperable CSS already. (And to the extent that they don't, unprefixing will only improve the experience.)
Thanks for the clarification, Daniel.
Sure. (Also, if we *did* want to restrict the unprefixing service to be fennec-only, we could easily do so, by customizing layout.css.unprefixing-service.enabled to be false by default (in all.js) & true in Fennec & B2G, for example. I tend to think we shouldn't do that, for reasons discussed in comment 3, but it's easy enough to change if we need to.)
I've manually tested these on a local build and it seems to do the trick. If anyone does the same, tabelog and girlschannel also have WebKit gradients so those will look much better when that code lands. Daniel, should I push this to try? Seems like we could get away without that, but I'm happy to do so.
Comment on attachment 8602366 [details] [diff] [review] Add-top-jp-sites-to-whitelist.patch Yeah, no need for Try server on these. If it builds & passes a local run of ./mach mochitest-plain layout/style/test/test_unprefixing_service.html then it shouldn't be able to cause any other form of bustage. (The mochitest won't exercise these new list entries, but it does touch this array & this code, so worth running it as a sanity-check.) ># HG changeset patch ># User Mike Taylor <email@example.com> > >Bug 1162106: Add top .jp sites to unprefixing service whitelist. r=dholbert s/unprefixing/CSS unprefixing/ >--- a/caps/nsPrincipal.cpp >+++ b/caps/nsPrincipal.cpp >@@ -650,16 +650,45 @@ IsOnFullDomainWhitelist(nsIURI* aURI) >+ NS_LITERAL_CSTRING("i.yimg.jp"), // for *.yahoo.co.jp >+ NS_LITERAL_CSTRING("ai.yimg.jp"), // for *.yahoo.co.jp [...] >+ NS_LITERAL_CSTRING("stat100.ameba.jp"), // for ameblo.jp >+ NS_LITERAL_CSTRING("stat.ameba.jp"), // for ameblo.jp >+ NS_LITERAL_CSTRING("user.ameba.jp"), // for ameblo.jp >+ NS_LITERAL_CSTRING("www.goo.ne.jp"), // for www.goo.ne.jp >+ NS_LITERAL_CSTRING("u.xgoo.jp"), // for www.goo.ne.jp >+ NS_LITERAL_CSTRING("img.news.goo.ne.jp"), // for www.goo.ne.jp For these multi-entry ones -- are you sure you need each of these domains? (Is there prefixed CSS being served w/out working fallbacks from *each* of these domains?)
(i.e. if ameblo.jp pulls in styles from 3 domains, but the relevant prefixed CSS that breaks the site only comes from one of those, then we should just whitelist that one.) (Just asking as a sanity-check, since I'm slightly surprised that there are so many new sites here (5 I think?) that need multi-domain whitelisting to function.)
(In reply to Daniel Holbert [:dholbert] from comment #7) > Comment on attachment 8602366 [details] [diff] [review] > Add-top-jp-sites-to-whitelist.patch > > Yeah, no need for Try server on these. If it builds & passes a local run of > ./mach mochitest-plain layout/style/test/test_unprefixing_service.html > then it shouldn't be able to cause any other form of bustage. Cool, we should be good then. > For these multi-entry ones -- are you sure you need each of these domains? > (Is there prefixed CSS being served w/out working fallbacks from *each* of > these domains?) Let me double check, I did some spot checking in DevTools style editor search but it's worth being a little more careful--we might be able to nuke 1 or 2 of these.
(In reply to Mike Taylor [:miketaylr] from comment #9) > Let me double check, I did some spot checking in DevTools style editor > search but it's worth being a little more careful--we might be able to nuke > 1 or 2 of these. Cool, canceling review for the moment then. Re-tag me when you've finished checking (& reuploaded if there are any changes that result).
5 years ago
Attachment #8602366 - Flags: review?(dholbert)
Alright, tests look good. 240 INFO TEST-OK | layout/style/test/test_unprefixing_service.html | took 3233ms 241 INFO TEST-START | Shutdown 242 INFO Passed: 232 243 INFO Failed: 0 244 INFO Todo: 0 Here's the updated patch, I was actually able to remove 4 domains \o/.
Attachment #8602366 - Attachment is obsolete: true
5 years ago
Attachment #8602473 - Flags: review?(dholbert) → review+
Thank you Daniel.
(Sheriffs generally require a Try run of some sort before they'll act on checkin-needed, to be on the safe side [since they push lots of patches at a time, and if tests break on a mega-push, it can be difficult to figure out which checkin-needed patch was responsible].) I just went ahead and included this in my push -- landed: ^
See https://webcompat.com/issues/1101 for a newly analysed flexbox issue with nikkei.com - I expect this patch to take care of it but since it's a high-profile site we might want to do an extra check.
Yes, the menu is fixed by this patch (you can test in Desktop Nightly, if you're not testing with the whitelist-disabled builds). There's some other issues, but we can discuss them in webcompat#1101.
I've contacted to nikkei.com and they'll fix the site as early as end of this month. If you continue to find issues and tell me (tell url of webcompat issue), we can avoid to add nikkei.com into this list. Need file another bug to remove nikkei.com from the list?
(In reply to dynamis (Tomoya ASAI) from comment #18) > If you continue to find issues and tell me (tell url of webcompat issue), we > can avoid to add nikkei.com into this list. Need file another bug to remove > nikkei.com from the list? Hi Tomoya, the webcompat issue is at https://webcompat.com/issues/948. To remove nikkei.com from the whitelist, please file a bug and make it a blocker of 1162245, and cc myself and dholbert. One of us can write the patch to remove it, thanks!
Hi Mike, I understand and I've shared this with our team about this. We'll file bug when we need to add/remove some sites to the list. Thanks!
Hi, Mark. I'm an employee of mixi. We have fixed this issue. Could you remove our domains (mixi.jp and img.mixi.net) from your list please? Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1057221
Thank you Yuki! I've filed Bug 1206184 to do this.
You need to log in before you can comment on or make changes to this bug.