1501804 - web fonts runtime setting broken

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

At some point the pref for this changed from a boolean to an integer, so it broke our runtime setting. We should have a test :)

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Makoto would you like to take this bug?

Comment 2

[Makoto Kato [:m_kato]]

Ah, it mean browser.display.use_document_fonts.  I thought that this is gfx.downloadable_fonts.enabled for downloadable font.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1501804 - browser.display.use_document_fonts is integer, not boolean. r?snorp

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50484e0a51c8
browser.display.use_document_fonts is integer, not boolean. r=snorp

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/50484e0a51c8

Comment 6

[Chris Peterson [:cpeterson]]

Makoto, does this web fonts setting affect GV 64? Focus will use GV 64 Beta in another beta test so blocking web fonts needs to work in 64.

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 9022539 [details]
Bug 1501804 - browser.display.use_document_fonts is integer, not boolean. r?snorp

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Firefox Focus can blow web font setting.  But it doesn't work well when using GeckoView.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Preference type is mistake.  it is int type, not boolean

String changes made/needed: No

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 9022539 [details]
Bug 1501804 - browser.display.use_document_fonts is integer, not boolean. r?snorp

fix for web fonts in geckoview, approved for 64.0b9

Comment 9

[Julien Cristau [:jcristau]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #0)
> At some point the pref for this changed from a boolean to an integer, so it
> broke our runtime setting. We should have a test :)

Any update on that test? :)

Comment 10

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/a691b474af0c

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Filed bug 1506624 for the test.

Comment 12

[Razvan Maries]

Backed out from beta for failing Android at org.mozilla.geckoview.test.crash.CrashTest.crashParent

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=a4175191b29b274d59ed117567ec03df9d6b8cf5&selectedJob=211226792

Failure log: https://taskcluster-artifacts.net/PPYQfJl-RwuzaK3-0bkWLg/0/public/logs/live_backing.log

Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/d74a918ff930da31a1ef3ad019ae27fdc75b0d44

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Makoto any thoughts on cause for backout in comment 12? We probably want this in 64...

Comment 14

[Makoto Kato [:m_kato]]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #13)
> Makoto any thoughts on cause for backout in comment 12? We probably want
> this in 64...

Since bug 1502053 isn't landed on 65, this seems to occurs.  I am looking for workaround...

Comment 15

[Makoto Kato [:m_kato]]

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

Comment 16

[Makoto Kato [:m_kato]]

Attachment: for beta

Since bug 1502053 isn't landed on 65, we have to fix PrefsHelper too.  This prefs is into INT_TO_BOOL_PREFS, "(Boolean) value" causes exception.

After landing bug 1502053, PrefsHelper.setPref isn't used.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9028656 [details] [diff] [review]
for beta

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +498,5 @@
>       *
>       * @return Whether web fonts support is enabled.
>       */
>      public boolean getWebFontsEnabled() {
> +        return mWebFonts.get() != 0 ? true : false;

You can just do `return mWebFonts.get() != 0`

Comment 18

[Chris Peterson [:cpeterson]]

In anticipation of the creation of the GECKOVIEW_64_RELBRANCH next week, I am moving this bug from status-firefox64=affected to status-geckoview64=affected.

Comment 19

[Chris Peterson [:cpeterson]]

status-geckoview64=wontfix