Last Comment Bug 1162106 - Add top Japanese sites to unprefixing service whitelist (part 1)
: Add top Japanese sites to unprefixing service whitelist (part 1)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla40
Assigned To: Mike Taylor [:miketaylr]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 1162245
  Show dependency treegraph
 
Reported: 2015-05-06 09:10 PDT by Mike Taylor [:miketaylr]
Modified: 2015-09-18 11:03 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Add-top-jp-sites-to-whitelist.patch (2.75 KB, patch)
2015-05-06 15:43 PDT, Mike Taylor [:miketaylr]
no flags Details | Diff | Splinter Review
Add-top-jp-sites-to-CSS-unprefixing-2.patch (2.55 KB, patch)
2015-05-06 19:58 PDT, Mike Taylor [:miketaylr]
dholbert: review+
Details | Diff | Splinter Review

Description User image Mike Taylor [:miketaylr] 2015-05-06 09:10:39 PDT
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).
Comment 1 User image Jet Villegas (:jet) 2015-05-06 13:26:54 PDT
(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.
Comment 2 User image Mike Taylor [:miketaylr] 2015-05-06 13:35:30 PDT
(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 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2015-05-06 14:29:49 PDT
(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.)
Comment 4 User image Mike Taylor [:miketaylr] 2015-05-06 14:31:19 PDT
Thanks for the clarification, Daniel.
Comment 5 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2015-05-06 14:35:14 PDT
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.)
Comment 6 User image Mike Taylor [:miketaylr] 2015-05-06 15:43:29 PDT
Created attachment 8602366 [details] [diff] [review]
Add-top-jp-sites-to-whitelist.patch

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 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2015-05-06 15:49:36 PDT
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 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2015-05-06 15:51:57 PDT
(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.)
Comment 9 User image Mike Taylor [:miketaylr] 2015-05-06 16:00:09 PDT
(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 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2015-05-06 17:54:52 PDT
(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).
Comment 11 User image Mike Taylor [:miketaylr] 2015-05-06 19:58:38 PDT
Created attachment 8602473 [details] [diff] [review]
Add-top-jp-sites-to-CSS-unprefixing-2.patch

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/.
Comment 12 User image Mike Taylor [:miketaylr] 2015-05-07 06:59:11 PDT
Thank you Daniel.
Comment 14 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2015-05-07 09:09:15 PDT
(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: ^
Comment 15 User image Wes Kocher (:KWierso) 2015-05-08 10:16:13 PDT
https://hg.mozilla.org/mozilla-central/rev/7a63a3b1bd61
Comment 16 User image Hallvord R. M. Steen [:hallvors] 2015-05-15 07:01:10 PDT
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.
Comment 17 User image Mike Taylor [:miketaylr] 2015-05-15 08:34:00 PDT
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 User image dynamis (Tomoya ASAI) 2015-05-25 23:08:48 PDT
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?
Comment 19 User image Mike Taylor [:miketaylr] 2015-05-26 12:29:35 PDT
(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 User image dynamis (Tomoya ASAI) 2015-05-27 00:13:37 PDT
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 User image Yuki Kokubun 2015-09-17 00:00:59 PDT
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
Comment 22 User image Mike Taylor [:miketaylr] 2015-09-18 11:03:23 PDT
Thank you Yuki! I've filed Bug 1206184 to do this.

Note You need to log in before you can comment on or make changes to this bug.