Add top Japanese sites to unprefixing service whitelist (part 1)

RESOLVED FIXED in Firefox 40

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: miketaylr, Assigned: miketaylr)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(p11+, firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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

2 years ago
Assignee: nobody → miket

Comment 1

2 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

2 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.
(Assignee)

Updated

2 years ago
See Also: → bug 1162245
(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

2 years ago
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.)
(Assignee)

Comment 6

2 years ago
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 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?)
(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

2 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.
(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).
Attachment #8602366 - Flags: review?(dholbert)
(Assignee)

Comment 11

2 years ago
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/.
Attachment #8602366 - Attachment is obsolete: true
Attachment #8602473 - Flags: review?(dholbert) → review+
(Assignee)

Comment 12

2 years ago
Thank you Daniel.
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a63a3b1bd61
Keywords: checkin-needed
(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
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

2 years ago
tracking-p11: --- → +
(Assignee)

Updated

2 years ago
See Also: → bug 1163826
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

2 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.
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

2 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!
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!

Updated

2 years ago
Blocks: 1162245

Comment 21

2 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

2 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.