Closed Bug 993137 Opened 10 years ago Closed 10 years ago

Use Firefox OS font prefs on B2G Desktop

Categories

(Core Graveyard :: Widget: Gonk, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: janx, Assigned: janx)

References

Details

Attachments

(4 files, 34 obsolete files)

2.49 KB, patch
janx
: review+
janx
: feedback+
cbook
: checkin+
Details | Diff | Splinter Review
27.25 KB, patch
janx
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.02 KB, patch
janx
: review+
Details | Diff | Splinter Review
15.66 KB, patch
janx
: review+
Details | Diff | Splinter Review
Currently in http://dxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#3187-3188 Firefox OS font prefs are used only on MOZ_WIDGET_GONK and FXOS_SIMULATOR. To use them on B2G Desktop as well, this test should be `#ifdef MOZ_B2G`.

Also, a lot of these prefs are duplicated multiple times and could use some cleaning up.
Comment on attachment 8402995 [details] [diff] [review]
Clean up font prefs and use Firefox OS fonts on B2G Desktop. r=ochameau

Review of attachment 8402995 [details] [diff] [review]:
-----------------------------------------------------------------

Alex could you please take a look? This patch exposes the Firefox OS font prefs using MOZ_B2G instead of MOZ_WIDGET_GONK||FXOS_SIMULATOR like we discussed IRL. It also factors duplicate font prefs out, making it clearer what each platform does differently from the default. Also fixes trailing spaces.

If assume correctly that MOZ_B2G||ANDROID||XP_WIN||XP_MACOSX||XP_UNIX is always true, then my refactor doesn't change font preferences expect for B2G Desktop.
Attachment #8402995 - Flags: review?(poirot.alex)
Jan, looks like your editor trimmed whitespace...  Typically what you want, but I'd say it may be best to at least trim whitespace as a separate patch, or just don't do it in this case.  For this patch, it adds lots of noise since it appears there are quite a few such lines in all.js.
Comment on attachment 8402995 [details] [diff] [review]
Clean up font prefs and use Firefox OS fonts on B2G Desktop. r=ochameau

Review of attachment 8402995 [details] [diff] [review]:
-----------------------------------------------------------------

Doesn't work when the simulator is built on linux, because XP_UNIX font prefs overwrite the MOZ_B2G ones.
Attachment #8402995 - Flags: review?(poirot.alex) → review-
(In reply to J. Ryan Stinnett [:jryans] from comment #3)
> Jan, looks like your editor trimmed whitespace...  Typically what you want,
> but I'd say it may be best to at least trim whitespace as a separate patch,
> or just don't do it in this case.  For this patch, it adds lots of noise
> since it appears there are quite a few such lines in all.js.

Actually the trimming was on purpose out of pure OCD, but I agree about the noise. I guess I'll have to add these whitespaces back...
Comment on attachment 8403946 [details] [diff] [review]
Clean up font prefs and use Firefox OS fonts on B2G Desktop. r=ochameau

Alex could you please review? It now works on Linux B2G Desktop, because I moved the XP_UNIX font prefs above the MOZ_B2G overwrites. If the whitespace noise bothers you, please complain about it, I have a patch locally that adds all the trailing spaces back (*sigh*...)
Attachment #8403946 - Flags: review?(poirot.alex)
Comment on attachment 8403946 [details] [diff] [review]
Clean up font prefs and use Firefox OS fonts on B2G Desktop. r=ochameau

Review of attachment 8403946 [details] [diff] [review]:
-----------------------------------------------------------------

I challenge you to read this patch!

It is almost impossible to track the code moves and you modify too many things at once.
1/ I'm not sure there is much value in fixing whitespaces. In any case, if you want to fix whitespaces,
you have to do it in its own bug/patch so that, we ease merge conflicts and do not force to pull the font patches when you modify any of the whitespace line!!
2/ There is some font.* pref moves, but also some other that have nothing to do with pref. It makes it even harder to review, especially since we have to be carefull about prefs order.
Attachment #8403946 - Flags: review?(poirot.alex) → review-
Thanks a lot for taking the time to review my patch!

(In reply to Alexandre Poirot (:ochameau) from comment #9)
> 1/ I'm not sure there is much value in fixing whitespaces. In any case, if
> you want to fix whitespaces,
> you have to do it in its own bug/patch so that, we ease merge conflicts and
> do not force to pull the font patches when you modify any of the whitespace
> line!!

I'll put the whitespace fixes into a separate patch.

> 2/ There is some font.* pref moves, but also some other that have nothing to
> do with pref. It makes it even harder to review, especially since we have to
> be carefull about prefs order.

I think the diff engine is making the wrong assumptions about what changed and what stayed in place, making it confusing to review. I understand your pain, and will split the changes into a commit queue, that should make things easier to review.
My original patch in bug 925833 was uplifted to Aurora (so it could be included in the 1.4 simulator).  You may want to uplift this as well, if it makes sense to do so.
FYI - pref changes alone aren't enough to change all fonts to Fira Sans. At the moment, text which doesn't have any font preference get set to the platform default font. This is determined by the platform specific implementation of GetDefaultFont in gfx/thebes/.
Attached patch Trivial fixes. r=ochameau (obsolete) — Splinter Review
Attachment #8403946 - Attachment is obsolete: true
Attachment #8405081 - Flags: review?(poirot.alex)
Attachment #8405082 - Flags: review?(poirot.alex)
Attachment #8405083 - Flags: review?(poirot.alex)
Attachment #8405084 - Flags: review?(poirot.alex)
Attachment #8405085 - Flags: review?(poirot.alex)
Attachment #8405086 - Flags: review?(poirot.alex)
Attachment #8405087 - Flags: review?(poirot.alex)
Attachment #8405088 - Flags: review?(poirot.alex)
Comment on attachment 8405089 [details] [diff] [review]
Move B2G font prefs after XP_UNIX font prefs. r=ochameau

Review of attachment 8405089 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/src/init/all.js
@@ +3055,5 @@
>  // print_extra_margin enables platforms to specify an extra gap or margin
>  // around the content of the page for Print Preview only
>  pref("print.print_extra_margin", 0); // twips
>  
> +/* PostScript print module prefs */

Even though it still looks like I've changed these lines, I haven't. I've just moved the B2G fonts down, and since that puts to "#ifdef ANDROID ... #endif" blocks together, I merged them.
Attachment #8405089 - Flags: review?(poirot.alex)
Patch successfully split up, should be easier to review now :) Maybe the splitting was a bit extreme, please let me know which patches you feel comfortable to land as one.
Comment on attachment 8405081 [details] [diff] [review]
Trivial fixes. r=ochameau

Review of attachment 8405081 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ -8885,5 @@
>  dnl ========================================================
>  
> -if test "$OS_ARCH" = "Darwin"; then
> -  AC_DEFINE(XP_UNIX)
> -elif test "$OS_ARCH" != "WINNT"; then

Nothing is trivial when speaking about ifdefs!
I would prefer having a build system peer to review this,
as that's not my field.
Attachment #8405081 - Flags: review?(poirot.alex)
Attachment #8405081 - Flags: review?(mh+mozilla)
Attachment #8405081 - Flags: feedback+
Comment on attachment 8405082 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau

Review of attachment 8405082 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and should be merged with all "Clean up ..." patches.
Attachment #8405082 - Flags: review?(poirot.alex) → review+
Comment on attachment 8405083 [details] [diff] [review]
Trim whitespaces in all.js. r=ochameau

Review of attachment 8405083 [details] [diff] [review]:
-----------------------------------------------------------------

Would be really great to be the first patch of the queue.
Attachment #8405083 - Flags: review?(poirot.alex) → review+
Attachment #8405084 - Flags: review?(poirot.alex) → review+
Attachment #8405085 - Flags: review?(poirot.alex) → review+
Attachment #8405086 - Flags: review?(poirot.alex) → review+
Attachment #8405088 - Flags: review?(poirot.alex) → review+
Comment on attachment 8405089 [details] [diff] [review]
Move B2G font prefs after XP_UNIX font prefs. r=ochameau

Review of attachment 8405089 [details] [diff] [review]:
-----------------------------------------------------------------

Your work highlights the fact that all these font prefs are entangled with various other prefs!
That forces to read the whole content of all.js to understand what is going to be the final value of each font pref.
Attachment #8405089 - Flags: review?(poirot.alex) → review+
Comment on attachment 8405087 [details] [diff] [review]
Clean up B2G font prefs. r=ochameau

Review of attachment 8405087 [details] [diff] [review]:
-----------------------------------------------------------------

Pike, Do you manage fonts per locale?
In these patches, Jan mainly tries to clean up and simplify related prefs in all.js.
Mostly because it becomes hairy to understand what is really set for each platform.
(the concrete goal of this bug is to fix fonts on b2g desktop.)

I wish that someone managing these prefs could bless these patches.
I took great care rewieving these patches to ensure we do not change all.js behavior,
except for b2g desktop, which we want to fix in order to behave like devices.

::: modules/libpref/src/init/all.js
@@ -3142,5 @@
>  pref("font.name.sans-serif.el", "Clear Sans");
>  pref("font.name.monospace.el", "Droid Sans Mono");
>  pref("font.name-list.sans-serif.el", "Clear Sans, Roboto, Droid Sans");
>  
>  pref("font.name.serif.he", "Droid Serif");

This one `he` is Charis on b2g and Droid on Android.
It may be an error, but I'm not the one to make that call.
With your patch applied, it looks like it will be Charis on both.
Attachment #8405087 - Flags: review?(poirot.alex) → review?(l10n)
Comment on attachment 8405087 [details] [diff] [review]
Clean up B2G font prefs. r=ochameau

Review of attachment 8405087 [details] [diff] [review]:
-----------------------------------------------------------------

Johnathan is the best guy to comment on these.

I didn't pick up if we're making any effort to actually get the fonts to people using the desktop builds. Are we?
Attachment #8405087 - Flags: review?(l10n) → review?(jfkthame)
Alex, thanks a lot for the awesome reviews!

(In reply to Alexandre Poirot (:ochameau) from comment #28)
> Comment on attachment 8405089 [details] [diff] [review]
> Move B2G font prefs after XP_UNIX font prefs. r=ochameau
> 
> Review of attachment 8405089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your work highlights the fact that all these font prefs are entangled with
> various other prefs!
> That forces to read the whole content of all.js to understand what is going
> to be the final value of each font pref.

True. Maybe my changes even entangle them a bit more because they remove duplicates and instead rely on pref overwrite order.

(In reply to Alexandre Poirot (:ochameau) from comment #29)
> Comment on attachment 8405087 [details] [diff] [review]
> Clean up B2G font prefs. r=ochameau
> 
> Review of attachment 8405087 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: modules/libpref/src/init/all.js
> @@ -3142,5 @@
> >  pref("font.name.sans-serif.el", "Clear Sans");
> >  pref("font.name.monospace.el", "Droid Sans Mono");
> >  pref("font.name-list.sans-serif.el", "Clear Sans, Roboto, Droid Sans");
> >  
> >  pref("font.name.serif.he", "Droid Serif");
> 
> This one `he` is Charis on b2g and Droid on Android.
> It may be an error, but I'm not the one to make that call.
> With your patch applied, it looks like it will be Charis on both.

Actually, the pref "font.name.serif.he" is set to "Charis SIL Compact" if ANDROID || MOZ_B2G on line 3235, but is later overwritten to "Droid Serif" if !MOZ_B2G && ANDROID on line 3335.

So the previous behavior is not changed, but if this font difference between android and b2g is indeed an error, it should be fixed.

Factoring the common prefs out also serves the purpose of stressing the slight differences between platform prefs, which might well be errors in some cases. Maybe there is a need for follow-up patches to address these inconsistencies.

(In reply to Axel Hecht [:Pike] from comment #30)
> Comment on attachment 8405087 [details] [diff] [review]
> Clean up B2G font prefs. r=ochameau
> 
> I didn't pick up if we're making any effort to actually get the fonts to
> people using the desktop builds. Are we?

Yes, the meta-bug for this is 992210, and it should soon start discussing how to bring fonts to desktop builds. They could for example be packaged inside a gaia profile, and regarding how to register these fonts in gecko, Jonathan mentioned in bug 951593 comment 36 that it's probably better to use @font-face CSS rules than to implement a new way for specifying additional font folders.
I played around @font-face a bit for a cross-locale testing project, and didn't come up with good resolutions. Mostly because the font-face contradicts the font prefs, depending on language. Or at least I couldn't figure out how to do it.

I'd be happy to actually see a example that works across all our upcoming languages before we ditch the idea of having to mock around with font loading.

Also, if we did sneak in font-face in a way that works, we wouldn't need to change the prefs at all, wouldn't we?
(In reply to Axel Hecht [:Pike] from comment #32)
> I played around @font-face a bit for a cross-locale testing project, and
> didn't come up with good resolutions. Mostly because the font-face
> contradicts the font prefs, depending on language. Or at least I couldn't
> figure out how to do it.
> 
> I'd be happy to actually see a example that works across all our upcoming
> languages before we ditch the idea of having to mock around with font
> loading.
> 
> Also, if we did sneak in font-face in a way that works, we wouldn't need to
> change the prefs at all, wouldn't we?

I also found that @font-face was not sufficient.  I provided a more detailed comment on my findings in bug 951593 comment 37.  TL;DR: If @font-face can be made to work, great!  But I am not currently convinced that it can handle all cases that are needed here.
Attachment #8405081 - Flags: review?(mh+mozilla) → review+
Attachment #8405081 - Attachment is obsolete: true
Attachment #8405083 - Attachment is obsolete: true
Attachment #8405090 - Attachment is obsolete: true
Comment on attachment 8407626 [details] [diff] [review]
Trivial fixes. f=ochameau, r=glandium

Glandium's r+, ochameau's f+
Attachment #8407626 - Flags: review+
Attachment #8407626 - Flags: feedback+
Attachment #8407628 - Attachment is obsolete: true
Jonathan, could you please address Alex's concern? (First part of comment 29).

These patches are already reviewed, but Alex would feel better if someone managing fonts could double-check my changes. The "Clean up" patches were separated to make their review easier, but I'll merge all "Clean up" patches and the "Introduce cross-platform font pref defaults" patch into a single one.
Flags: needinfo?(jfkthame)
(In reply to Jan Keromnes [:janx] from comment #37)
> Jonathan, could you please address Alex's concern? (First part of comment
> 29).
> 
> These patches are already reviewed, but Alex would feel better if someone
> managing fonts could double-check my changes. The "Clean up" patches were
> separated to make their review easier, but I'll merge all "Clean up" patches
> and the "Introduce cross-platform font pref defaults" patch into a single
> one.

I'm OK with factoring out the font size prefs, which are almost all shared across platforms, and then just overriding a few selected ones within the platform-specific blocks.

However, for font names I'm less comfortable with the rearrangement here. I think it'll be easier to follow if each platform-specific block of font.name(-list).* prefs is complete and independent, with a clear #if-condition that specifies which platform it's for, and can be seen and understood in a single place.

This means there will be some duplication between Android and B2G prefs; but I think it'll be less confusing overall. Otherwise, especially with lengthy, nested #if blocks, it gets really hard to tell which collection of settings belong to which platform(s).
Flags: needinfo?(jfkthame)
Attachment #8407626 - Attachment is obsolete: true
Attachment #8405082 - Attachment is obsolete: true
Attachment #8405084 - Attachment is obsolete: true
Attachment #8405085 - Attachment is obsolete: true
Attachment #8405086 - Attachment is obsolete: true
Attachment #8405087 - Attachment is obsolete: true
Attachment #8405087 - Flags: review?(jfkthame)
Attachment #8405088 - Attachment is obsolete: true
Attachment #8405089 - Attachment is obsolete: true
Comment on attachment 8411189 [details] [diff] [review]
Trivial fixes. f=ochameau, r=glandium

rebased patch, ochameau's f+, glandium's r+
Attachment #8411189 - Flags: review+
Attachment #8411189 - Flags: feedback+
Comment on attachment 8411191 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame

Merged all "Clean up [...] font prefs" and the "Introduce cross-platform font pref defaults" patch into this one, rebased, and addressed Jonathan's concern (see below).

Alex, you already r+'d these changes, and Jonathan blessed them (modulo his concern). I'm only asking for your r+ again to be safe.

(In reply to Jonathan Kew (:jfkthame) from comment #38)
> (In reply to Jan Keromnes [:janx] from comment #37)
> > Jonathan, could you please address Alex's concern? (First part of comment
> > 29).
> > 
> > These patches are already reviewed, but Alex would feel better if someone
> > managing fonts could double-check my changes. The "Clean up" patches were
> > separated to make their review easier, but I'll merge all "Clean up" patches
> > and the "Introduce cross-platform font pref defaults" patch into a single
> > one.
> 
> I'm OK with factoring out the font size prefs, which are almost all shared
> across platforms, and then just overriding a few selected ones within the
> platform-specific blocks.

Great! Thank you, you forgot to edit the review flag but I'll consider this an r+ with nits.

> However, for font names I'm less comfortable with the rearrangement here. I
> think it'll be easier to follow if each platform-specific block of
> font.name(-list).* prefs is complete and independent, with a clear
> #if-condition that specifies which platform it's for, and can be seen and
> understood in a single place.

The font.name(-list).* prefs have very little in common accross platforms, so I had already left them out of the default cross-platform pref list.

The only factoring I did, which probably troubled you, were common font names between Android and Firefox OS, hence their location in a common #ifdef block. To makes things simpler, I redistributed these common prefs into their original #ifdef blocks.

> This means there will be some duplication between Android and B2G prefs; but
> I think it'll be less confusing overall. Otherwise, especially with lengthy,
> nested #if blocks, it gets really hard to tell which collection of settings
> belong to which platform(s).

I agree that those nested blocks were confusing, and I believe my patches make them saner: I removed the nesting by taking out the FirefoxOS-specific and Android-specific blocks of their common pref block. The 3 #ifdef blocks are now isolated, and I shrinked the common Android/FxOS block to only 18 prefs, which are their common difference to the default cross-platform font-size and font-type prefs.
Attachment #8411191 - Flags: review?(poirot.alex)
Attachment #8411194 - Attachment is obsolete: true
Comment on attachment 8411256 [details] [diff] [review]
Use Firefox OS fonts on B2G Desktop. r=ochameau

This is a rebased squash of "Use Firefox OS fonts on B2G Desktop" and "Move B2G font prefs after XP_UNIX font prefs" which Alex already r+'d.

Note: Since git and hg were completely failing at generating a sensible diff, I edited the changeset manually to replace the diff part with a `diff -u8` of the file before and after my commit.
Attachment #8411256 - Flags: review+
Alex, for your convenience and mental sanity, I generated a diff of all.js:

1. with the previous patch queue applied (which you reviewed), and
2. with the current patch queue applied.

The few changes are either addressing Jonathan's comment, or leaving comments in the source that were already there before my patches.
Attachment #8411265 - Flags: review-
Comment on attachment 8411189 [details] [diff] [review]
Trivial fixes. f=ochameau, r=glandium

Ryan, could you land this for me please? No point for this patch to wait on the others.
Attachment #8411189 - Flags: checkin?(ryanvm)
Attachment #8411189 - Flags: checkin?(ryanvm) → checkin+
Comment on attachment 8411191 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame

Review of attachment 8411191 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM thanks for the interdiff!
Attachment #8411191 - Flags: review?(poirot.alex) → review+
Attachment #8411189 - Attachment is obsolete: true
Attachment #8411265 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/30138717a075
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Attachment #8411191 - Attachment is obsolete: true
Actually not resolved fixed, since only 1/3 patches landed!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
;-)
Keywords: leave-open
Attachment #8411256 - Attachment is obsolete: true
ochameau's r+
Attachment #8412116 - Flags: review+
Comment on attachment 8412109 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame

ochameau's and jfkthames' r+
Attachment #8412109 - Flags: review+
"checkin-needed" for the 2 remaining patches.

Please first apply "Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame",
and after that "Use Firefox OS fonts on B2G Desktop. r=ochameau".

Thanks!
Keywords: checkin-needed
Attachment #8411189 - Attachment is obsolete: false
Attachment #8412109 - Flags: checkin+
Attachment #8412116 - Flags: checkin+
Comment on attachment 8412109 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame

backed out
Attachment #8412109 - Flags: checkin+
Flags: needinfo?(janx)
Comment on attachment 8412116 [details] [diff] [review]
Use Firefox OS fonts on B2G Desktop. r=ochameau

backed out
Attachment #8412116 - Flags: checkin+
Font comparison reftests on linux (here ubuntu32) for b2g desktop are expected to fail, because b2g desktop wasn't using the correct fonts before, and these patches fix that.

Jonathan, how can I rebaseline these reftests for my changes?

Also, I'm skeptical that only two reftests should fail on this change. And some of the reftest logs look worrying:

> SKIPPED; boolean preference 'font.size.variable.x-western'
> SKIPPED; string preference 'font.size.variable.x-western'

This pref if neither boolean nor string.
Flags: needinfo?(jfkthame)
(In reply to Jan Keromnes [:janx] from comment #61)
> Font comparison reftests on linux (here ubuntu32) for b2g desktop are
> expected to fail, because b2g desktop wasn't using the correct fonts before,
> and these patches fix that.
> 
> Jonathan, how can I rebaseline these reftests for my changes?
> 

The tests themselves look OK to me. They simply compare the rendering of text with font-family:serif or font-family:sans-serif to the browser's default with no font-family specified. So one of those comparisons should match, and not the other; which one will depend on the setting of font.default.x-western, so we test with the pref set to both "serif" and "sans-serif". (See the reftest manifest.)

From the images in the log, it certainly appears something isn't working right: the tests show that elements with
  font-family: serif
  font-family: sans-serif
both end up rendering identically, which should not be the case.

I don't know what the "B2G Desktop Linux" test environment is like; does it even have the fonts you're trying to specify in prefs?

> Also, I'm skeptical that only two reftests should fail on this change. 

It sounds like this needs to be run through tryserver before (re-)landing, to check that any impact on existing tests is properly accounted for. Fixing the problem above may well lead to failures elsewhere...

> And
> some of the reftest logs look worrying:
> 
> > SKIPPED; boolean preference 'font.size.variable.x-western'
> > SKIPPED; string preference 'font.size.variable.x-western'
> 
> This pref if neither boolean nor string.

Don't worry about that. There are a couple of tests in there to verify that the reftest framework will FAIL to set a pref if the wrong type of value is provided; that's where these messages come from.
Flags: needinfo?(jfkthame)
Attachment #8412116 - Attachment is obsolete: true
Comment on attachment 8414323 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame

Review of attachment 8414323 [details] [diff] [review]:
-----------------------------------------------------------------

r=ochameau, r=jfkthame
Attachment #8414323 - Flags: review+
r=ochameau
Attachment #8414324 - Attachment is obsolete: true
Attachment #8414330 - Flags: review+
Comment on attachment 8414330 [details] [diff] [review]
Use Firefox OS fonts on B2G Desktop. r=ochameau

(hand-crafted but still a patch)
Attachment #8414330 - Attachment is patch: true
Comment on attachment 8414323 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame

The reftest failures were unrelated to this patch : https://tbpl.mozilla.org/?tree=Try&rev=ae9c2db45891

Please check it back in.
Attachment #8414323 - Flags: checkin?
(one patch left after that)
Keywords: leave-open
Comment on attachment 8414323 [details] [diff] [review]
Introduce cross-platform font pref defaults. r=ochameau, r=jfkthame

https://hg.mozilla.org/integration/b2g-inbound/rev/157f3235f360
Attachment #8414323 - Flags: checkin? → checkin+
Target Milestone: mozilla31 → ---
(In reply to Jonathan Kew (:jfkthame) from comment #62)
> The tests themselves look OK to me. They simply compare the rendering of
> text with font-family:serif or font-family:sans-serif to the browser's
> default with no font-family specified. So one of those comparisons should
> match, and not the other; which one will depend on the setting of
> font.default.x-western, so we test with the pref set to both "serif" and
> "sans-serif". (See the reftest manifest.)
> 
> From the images in the log, it certainly appears something isn't working
> right: the tests show that elements with
>   font-family: serif
>   font-family: sans-serif
> both end up rendering identically, which should not be the case.
> 
> I don't know what the "B2G Desktop Linux" test environment is like; does it
> even have the fonts you're trying to specify in prefs?

Thanks for the detailed explanation, Jonathan! The different test environments probably don't have the right fonts, because they're unique to Firefox OS (Fira Sans, Fira Mono, etc). However, the reftest failure only happens on Linux. I think that the other platforms properly fall back to serif or sans-serif fonts respectively, so they render differently and the reftest is happy, but on Linux both seem to fall back to the same font, which makes the comparison test fail.

How should I proceed to get these fonts installed on the test environments? Or should we fix the Linux fall-back problem?
Flags: needinfo?(jfkthame)
(In reply to Jan Keromnes [:janx] from comment #73)
> How should I proceed to get these fonts installed on the test environments?

I don't know anything about that - presumably, you'd need to work with RelEng to get the appropriate fonts installed on the machines (or in the OS images, or whatever) that are being used. But see the note below; I'm not entirely sure this is a good idea.

> Or should we fix the Linux fall-back problem?

It depends what you care about these B2G desktop builds testing. I think the reftest fails because the various prefs you're setting for both font.name.sans-serif and font.name.serif specify fonts that aren't present, and so we end up falling back to a common "last resort" font in both cases. So you can either fix this by making the Fira and Charis fonts available, so that the prefs work as intended, or by adjusting the font prefs so that they list fonts that are actually present.

I think there's something of a tension here between the desire to have things look "as close as possible to a real device" in the B2G desktop builds, so that designers can see how screens look, etc., and the fact that the collection of available fonts is a function of the underlying platform (Ubuntu or Fedora or Gonk or Windows or MacOSX, etc), and NOT of the application layer (Firefox, Fennec, B2G Desktop, etc) that's running on top of it.

Note that if you do negotiate with RelEng about getting the B2G fonts installed on the desktop test environments, this will introduce an added maintenance burden, as those fonts may then need to be updated when we make changes to the fonts installed on the actual devices - otherwise there will again be a mismatch, and probably test breakage.

So I wonder if it might be better to accept that the B2G Desktop unit tests are *not* testing the exact same font configuration as you'll get on B2G devices, and adjusting their prefs accordingly. ISTM that B2G Desktop tests can't be a complete replacement for testing on the emulator and on real devices anyhow, as it's inherently a different environment. So these tests can test lots of functionality, internal consistency, etc., but they wouldn't be testing precise details of layout/appearance.
Flags: needinfo?(jfkthame)
Attachment #8414330 - Attachment is obsolete: true
Comment on attachment 8444147 [details] [diff] [review]
Use Firefox OS fonts on B2G Desktop.

Rebased, carried over.
Attachment #8444147 - Flags: review+
Now that bug 998844 is done, I think the other thing you want here is a patch to include the appropriate FxOS fonts in desktop B2G builds, installed into a new ${GRE}/fonts/ directory.

I'd suggest putting the required fonts into a source directory such as ${M-C}/b2g/fonts/, so they can be part of the desktop B2G build; then you'll need some Makefile and/or moz.build magic to get them installed.
Depends on bug 1011562, which once landed should be used to ship the fonts in test environments.

Jonathan, the solution you suggest makes sense. Bug 1011562 tries to do something similar, except the fonts are downloaded at compile time (or the reference to a local fonts folder is given) instead of importing the 60MB of moztt fonts into M-C (or even the compiled 16MB).
Depends on: 1011562
Attachment #8444147 - Attachment is obsolete: true
Depends on: 1083544
(this can be closed after the last patch lands)
Keywords: leave-open
Rebased, carried over. Let's see again which releng machines need fixing by bug 1011562:

https://tbpl.mozilla.org/?tree=Try&rev=1197438b9ca1
Attachment #8482381 - Attachment is obsolete: true
Attachment #8506472 - Flags: review+
(Note to self: The only failing font tests are "reftest-sanity/font-sans-serif.html" and "reftest-sanity/font-serif.html", and only in "B2G Desktop Linux Opt" and "B2G Desktop Linux x64 Opt".)
Rebased, carried over.

Trying it together with patches from bug 1011562: https://tbpl.mozilla.org/?tree=Try&rev=8fc6b8f85e2f
Attachment #8509154 - Attachment is obsolete: true
Attachment #8509167 - Flags: review+
chicken-needed for last patch

http://cdn.iwastesomuchtime.com/5282012203012sdfsg.jpeg
Keywords: checkin-needed
Sure! No worries, let's have a look.

Rebased, carried over: https://tbpl.mozilla.org/?tree=Try&rev=415841c72cc8
Attachment #8509167 - Attachment is obsolete: true
Attachment #8510392 - Flags: review+
It does look like my patch is causing this. Not sure what font prefs have to do with color picker tests though...
Comment on attachment 8510777 [details] [diff] [review]
Make color picker popup test less fragile.

Mounir, it seems like my font change makes your test time out on just the "label-3" label with "foo<input type='color'>". Changing the order of the labels fixes the problem, but the failure seems to be because "foo" and the input break onto two lines, and somehow the "synthesizeMouseAtCenter" fails to do its job (or maybe it's a screen edge problem). This is very fragile, and breaking labels onto different lines seems to harden it enough.

Please let me know what you think.
Attachment #8510777 - Flags: review?(mounir)
Comment on attachment 8510777 [details] [diff] [review]
Make color picker popup test less fragile.

Review of attachment 8510777 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/forms/test_input_color_picker_popup.html
@@ +8,5 @@
>    <title>Test for Bug 1234567</title>
>    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>    <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <style> label { display: block } </style>

I would have expected element.focus() to move the element into view. Maybe B2G has a different behaviour there?

Anyway, this doesn't change the behaviour of the test so if that solves your problem, rs+
Attachment #8510777 - Flags: review?(mounir) → review+
Thanks Mounir!

On B2G, element.focus() doesn't seem to scroll horizontally to bring the element into full view when part if it is already showing. I suspect that with my font change, just enough of the label+input was pushed off-screen, such that synthesizeMouseAtCenter went outside as well and failed.

(Rebased, r+ carried over.)
Attachment #8510777 - Attachment is obsolete: true
Attachment #8511932 - Flags: review+
Keywords: checkin-needed
(Rebased, r+ carried over.)
Attachment #8510392 - Attachment is obsolete: true
Attachment #8511934 - Flags: review+
Mulet doesn't like this either. Your Try run actually shows the same failures. I'm leaving the first patch in (the test fix), but backing out the main patch.
https://hg.mozilla.org/integration/b2g-inbound/rev/098154530493

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=711067&repo=b2g-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=711675&repo=b2g-inbound
Whiteboard: [leave open]
Attachment #8511932 - Flags: checkin+
Oh no, these look like fixed size expectations in pixels for fonts and text, whereas I'm changing the default fonts for Mulet...
And clipboard fail... the backout's merged commit is https://hg.mozilla.org/mozilla-central/rev/098154530493

Sorry for the bug spam.
Backed out the test_input_color_picker_popup.html patch because it made the test more flaky than it already was (as evidenced by bug 1090174).
https://hg.mozilla.org/integration/mozilla-inbound/rev/440774d3604b
Comment on attachment 8511932 [details] [diff] [review]
Make color picker popup test less fragile.

And checked back out.
Attachment #8511932 - Flags: checkin+
Here is a summary of the failures preventing the last font patch ("Use Firefox OS fonts on B2G Desktop") from landing.

On B2G Desktop Linux x64 opt:

1) /tests/dom/html/test/forms/test_input_color_picker_popup.html times out.

I suspect this is related to screen size. When running `./mach mochitest-b2g-desktop dom/html/test/`, a small B2G window (about 200x200 pixels) opens, and a lot of tests time out. If (while tests are running) I manually resize the window to a size big enough to show all test contents (e.g. 800x800) then all tests pass.

When running all `mochitest-b2g-desktop` tests, the B2G window eventually takes on a device-like size after a few tests, and only the color_picker_popup test times out. As said in comment 92, I suspect the font change pushes a color picker just slightly too far off the screen.

2) /tests/dom/html/test/forms/test_input_color_picker_update.html sometimes throws an exception.

The exception is NS_ERROR_FACTORY_NOT_REGISTERED: Component returned failure code: 0x80040154, and I can't reproduce it locally.

(The Mulet issues will be addressed by bug 1011562's last patches.)
Making sure bug 1011562's last patches fix Mulet issues: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13ba9512b5d8
It does! \o/
Rebased on top of latest font pref changes, r+ carried over. Almost there.
Attachment #8511934 - Attachment is obsolete: true
Attachment #8522883 - Flags: review+
Slight change in patch removes the color picker failures on try, as shown in:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ad2b01e35032

r+ carried over.
Attachment #8511932 - Attachment is obsolete: true
Attachment #8522884 - Flags: review+
(I forgot this patch needs to be hand-crafted, or the diff blame falls on the wrong code.)
Attachment #8522883 - Attachment is obsolete: true
Attachment #8522889 - Flags: review+
Attachment #8522889 - Attachment is patch: true
"checkin-needed" for "Make color picker tests less fragile" and "Use Firefox OS fonts on B2G Desktop".

Please land these two patches together with the patch from bug 1011562.

Try for all three patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c66d38ae32b
Keywords: checkin-needed
Whiteboard: [leave open]
"Use Firefox OS fonts on B2G Desktop" needs rebasing.
Keywords: checkin-needed
Rebased, carried over.
Attachment #8522884 - Attachment is obsolete: true
Attachment #8523401 - Flags: review+
Rebased (just needed a recount), carried over.
Attachment #8522889 - Attachment is obsolete: true
Attachment #8523403 - Flags: review+
Please see comment 112.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d08b388c879a
https://hg.mozilla.org/mozilla-central/rev/d5153ab08ca4
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.