Closed
Bug 1162106
Opened 9 years ago
Closed 9 years ago
Add top Japanese sites to unprefixing service whitelist (part 1)
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: miketaylr, Assigned: miketaylr)
References
Details
Attachments
(1 file, 1 obsolete file)
2.55 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → miket
Comment 1•9 years ago
|
||
(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.
Assignee | ||
Comment 2•9 years ago
|
||
(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.
Comment 3•9 years ago
|
||
(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.)
Assignee | ||
Comment 4•9 years ago
|
||
Thanks for the clarification, Daniel.
Comment 5•9 years ago
|
||
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.)
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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 <miket@mozilla.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?)
Comment 8•9 years ago
|
||
(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.)
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Comment 10•9 years ago
|
||
(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).
Updated•9 years ago
|
Attachment #8602366 -
Flags: review?(dholbert)
Assignee | ||
Comment 11•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8602473 -
Flags: review?(dholbert) → review+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a63a3b1bd61
Keywords: checkin-needed
Comment 14•9 years ago
|
||
(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: ^
https://hg.mozilla.org/mozilla-central/rev/7a63a3b1bd61
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
tracking-p11:
--- → +
Comment 16•9 years ago
|
||
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.
Assignee | ||
Comment 17•9 years ago
|
||
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.
Comment 18•9 years ago
|
||
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?
Assignee | ||
Comment 19•9 years ago
|
||
(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!
Comment 20•9 years ago
|
||
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!
Comment 21•9 years ago
|
||
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
Assignee | ||
Comment 22•9 years ago
|
||
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.
Description
•