web fonts runtime setting broken

RESOLVED FIXED in Firefox 65

Status

defect
P1
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: snorp, Assigned: m_kato)

Tracking

59 Branch
mozilla65
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(geckoview62 wontfix, geckoview63 wontfix, geckoview64 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

Attachments

(2 attachments)

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 :)
Makoto would you like to take this bug?
Flags: needinfo?(m_kato)
Priority: -- → P1
Ah, it mean browser.display.use_document_fonts.  I thought that this is gfx.downloadable_fonts.enabled for downloadable font.
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/50484e0a51c8
browser.display.use_document_fonts is integer, not boolean. r=snorp
https://hg.mozilla.org/mozilla-central/rev/50484e0a51c8
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
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.
Flags: needinfo?(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
Flags: needinfo?(m_kato)
Attachment #9022539 - Flags: approval-mozilla-beta?
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
Attachment #9022539 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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? :)
Flags: in-testsuite?
Filed bug 1506624 for the test.
See Also: → 1506624
Makoto any thoughts on cause for backout in comment 12? We probably want this in 64...
(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...
Posted patch for betaSplinter Review
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.
Flags: needinfo?(m_kato)
Attachment #9028656 - Flags: review?(snorp)
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`
Attachment #9028656 - Flags: review?(snorp) → review+
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.
status-geckoview64=wontfix
Product: Firefox for Android → GeckoView
Version: Firefox 59 → 59 Branch
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.