[reftest::B2G] All reftest cases in layout/reftests/font-inflation fail after enable OOP

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
5 years ago
2 years ago

People

(Reporter: alin, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 10 obsolete attachments)

660 bytes, patch
Details | Diff | Splinter Review
4.91 KB, patch
Details | Diff | Splinter Review
6.67 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
The bug is related to 922680 #comment32.
As for module font-inflation, it seems to affect not only reftest but also normal cases. According to test, a normal app installed wouldn't be affected by pref of font inflation enabled. So most of the failed cases should be disabled or we can fix the side effect from bug(940036).
(Reporter)

Updated

5 years ago
Blocks: 922680
By "module", do you mean "directory"?


Why is this a separate bug?  Is it just the issue that the work in bug 922680 is failing to set preferences correctly?  Or is there something specific to font inflation?
(Reporter)

Comment 2

5 years ago
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #1)
> By "module", do you mean "directory"?
> 
> 
> Why is this a separate bug?  Is it just the issue that the work in bug
> 922680 is failing to set preferences correctly?  Or is there something
> specific to font inflation?

yes, the module means directory.
The story is a patch for bug 940036 will result in failing to enable font infaltion.
The patch will give a fake viewport for a APP and it afftects to disable font inflation.
But oop reftest running in a content process will use the fake viewport,
which differs from non-oop reftest running in B2G(fake viewport doesn't apply to this).
(Reporter)

Comment 3

5 years ago
I think the patch shouldn't afftect the normal cases(a app) if the pref font inflation enabled.

Updated

5 years ago
Blocks: 973835

Updated

5 years ago
No longer blocks: 922680

Updated

5 years ago
Summary: [Tracking] (reftest) font inflation enabled doesn't work in module font-inflation → [Tracking] (reftest) tests in layout/reftests/font-inflation fail
This is to follow https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c33, to prevent too diverse discussion in original bug.

Font-inflation fail in B2G/OOP

In bug 706198, we disable font-inflation for mobile site with view port.
In bug 940036, we add a "fake" view port for content process.

So, no font-inflation in B2G/OOP reftests.

Bug 706198 is reasonable as web designer know he is design for mobile device and we do not have do the adjustment. Bug 940036 is a temporary solution (https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c33) and font-inflation is good for non-mobile sites. I suggest we can keep font-inlfation in B2G/OOP.

Two steps:
1. We'll skip font-inflation reftests in B2G/OOP temporary.
2. Discuss with vingtetun, not to have fakeViewPort during reftests.

Please let me know how you think.
Flags: needinfo?(jonas)
Flags: needinfo?(dbaron)
Flags: needinfo?(21)
Are you saying that bug 940036 disabled font inflation entirely on B2G?
Flags: needinfo?(vichen)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #5)
> Are you saying that bug 940036 disabled font inflation entirely on B2G?

1. Disable font-inflation on B2G OOP (but not on B2G) is a temporary method.
2. Font inflation is good feature and should enable in B2G OOP. I'll dig-into to see how to keep font-inflation and fakeViewPort in B2G/OOP. This is my next item to do, should be able to start from 2/26.
Flags: needinfo?(vichen)
(In reply to Vincent Chen [:vichen] from comment #6)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #5)
> > Are you saying that bug 940036 disabled font inflation entirely on B2G?
> 
> 1. Disable font-inflation on B2G OOP (but not on B2G) is a temporary method.
> 2. Font inflation is good feature and should enable in B2G OOP. I'll
> dig-into to see how to keep font-inflation and fakeViewPort in B2G/OOP. This
> is my next item to do, should be able to start from 2/26.

Sorry, answer wrong.
From what I know from https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c33, bug 706198 disable font-inflation with view port and bug 940036 add view port for B2G.
Created attachment 8381153 [details] [diff] [review]
skip font-inflation reftests in B2G/OOP

Updated

5 years ago
Depends on: 976923
Created attachment 8381907 [details] [diff] [review]
Patch v1

From nsIPresShell::RecomputeFontSizeInflationEnabled() (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#9975), by default font inflation is enabled if we define "font.size.inflation.minTwips" and "font.size.inflation.lineThreshold" in pref and we have a heuristic algorithm to disable it (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#9986 to #10042).

Two pref will force to enable/disable font-inflation:
1. "font.size.inflation.forceEnabled"
2. "font.size.inflation.disabledInMasterProcess"

* For 1, it only override part of the heuristic algorithm, so I move the uncovered part (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#10005 - #10042) to only when font-inflation is not forceEnabled.

* For some of the reftests in font-inflation (http://mxr.mozilla.org/mozilla-central/source/layout/reftests/font-inflation/reftest.list#55-#59), it is intented to test font-inflation is not enabled, so we should not force-enabled to these tests. As the "font.size.inflation.disabledInMasterProcess" will force to disable font-inflation in B2G http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#750(http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#750), so we rewrite this pref during reftests.

* disable-fontinfl-on-mobile-4.html fail in B2G/OOP, it is not because font-inflation but do scale. We skip it in B2G/OOP and use bug 976923 to track.

dbaron,
Need your help to review this.
(In reply to Vincent Chen [:vichen] from comment #4)
> This is to follow https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c33,
> to prevent too diverse discussion in original bug.
> 
> Font-inflation fail in B2G/OOP
> 
> In bug 706198, we disable font-inflation for mobile site with view port.
> In bug 940036, we add a "fake" view port for content process.
> 
> So, no font-inflation in B2G/OOP reftests.
> 
> Bug 706198 is reasonable as web designer know he is design for mobile device
> and we do not have do the adjustment. Bug 940036 is a temporary solution
> (https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c33) and font-inflation
> is good for non-mobile sites. I suggest we can keep font-inlfation in
> B2G/OOP.
> 
> Two steps:
> 1. We'll skip font-inflation reftests in B2G/OOP temporary.
> 2. Discuss with vingtetun, not to have fakeViewPort during reftests.
> 
> Please let me know how you think.

The fake viewport is only inserted if the the mozbrowser is a mozapp. So the fake viewport does not affect web rendered content. 
IIRC correctly the test in b2g-desktop are running into a <iframe mozbrowser mozapp>, is that correct ? If yes, maybe we want to run some of the tests inside a <iframe mozbrowser> instead ?
Flags: needinfo?(21)

Updated

5 years ago
Attachment #8381907 - Flags: review?(dbaron)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #10)
> (In reply to Vincent Chen [:vichen] from comment #4)
> > This is to follow https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c33,
> > to prevent too diverse discussion in original bug.
> > 
> > Font-inflation fail in B2G/OOP
> > 
> > In bug 706198, we disable font-inflation for mobile site with view port.
> > In bug 940036, we add a "fake" view port for content process.
> > 
> > So, no font-inflation in B2G/OOP reftests.
> > 
> > Bug 706198 is reasonable as web designer know he is design for mobile device
> > and we do not have do the adjustment. Bug 940036 is a temporary solution
> > (https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c33) and font-inflation
> > is good for non-mobile sites. I suggest we can keep font-inlfation in
> > B2G/OOP.
> > 
> > Two steps:
> > 1. We'll skip font-inflation reftests in B2G/OOP temporary.
> > 2. Discuss with vingtetun, not to have fakeViewPort during reftests.
> > 
> > Please let me know how you think.
> 
> The fake viewport is only inserted if the the mozbrowser is a mozapp. So the
> fake viewport does not affect web rendered content. 
> IIRC correctly the test in b2g-desktop are running into a <iframe mozbrowser
> mozapp>, is that correct ? If yes, maybe we want to run some of the tests
> inside a <iframe mozbrowser> instead ?

We should run <iframe mozbrowser> in b2g reftests (https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c7).

Ahal, 
Please help to confirm.

Please also help to check comment 9, it should help us to pass reftest test and keep our design.
Flags: needinfo?(ahalberstadt)
Yes, when you set the pref reftest.browser.iframe.enabled=True then it will use an <iframe mozbrowser> instead of a <xul:window>.
Flags: needinfo?(ahalberstadt)
(In reply to Vincent Chen [:vichen] from comment #9)
> Created attachment 8381907 [details] [diff] [review]
> Patch v1
> 
> From nsIPresShell::RecomputeFontSizeInflationEnabled()
> (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#9975), by default font inflation is enabled if we define
> "font.size.inflation.minTwips" and "font.size.inflation.lineThreshold" in
> pref and we have a heuristic algorithm to disable it
> (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#9986 to #10042).
> 
> Two pref will force to enable/disable font-inflation:
> 1. "font.size.inflation.forceEnabled"
> 2. "font.size.inflation.disabledInMasterProcess"
> 
> * For 1, it only override part of the heuristic algorithm, so I move the
> uncovered part
> (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#10005 - #10042) to only when font-inflation is not forceEnabled.

all.js says:

  /*
   * In products with multi-mode pan-and-zoom and non-pan-and-zoom UIs,
   * this pref forces font inflation to always be enabled in all modes.
   * That is, any heuristics used to detect pan-and-zoom
   * vs. non-pan-and-zoom modes are disabled and all content is treated
   * as pan-and-zoom mode wrt font inflation.
   *
   * This pref has no effect if font inflation is not enabled through
   * either of the prefs above.  It has no meaning in single-mode UIs.
   */
  pref("font.size.inflation.forceEnabled", false);

so I think this code is working as intended.

We should perhaps consider renaming the forceEnabled pref to skipProcessHeuristics.


> * For some of the reftests in font-inflation
> (http://mxr.mozilla.org/mozilla-central/source/layout/reftests/font-
> inflation/reftest.list#55-#59), it is intented to test font-inflation is not
> enabled, so we should not force-enabled to these tests. As the
> "font.size.inflation.disabledInMasterProcess" will force to disable
> font-inflation in B2G
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#750(http://mxr.
> mozilla.org/mozilla-central/source/b2g/app/b2g.js#750), so we rewrite this
> pref during reftests.

This only seems needed because of the previous change.

> * disable-fontinfl-on-mobile-4.html fail in B2G/OOP, it is not because
> font-inflation but do scale. We skip it in B2G/OOP and use bug 976923 to
> track.

I don't understand what you mean by this.

I'd also appreciate either a "yes" or "no" answer to my question in comment 5.
Flags: needinfo?(dbaron)
I think dbaron has answered the question you had for me here, right?

Thanks for spending time on this! It's much appreciated and it seems like real bugs are getting fixed. (Yay for tests!!)
Flags: needinfo?(jonas)
Comment on attachment 8381907 [details] [diff] [review]
Patch v1

review- per comment 13.

I'd still like to know the answer to my question in comment 5.
Attachment #8381907 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #15)
> Comment on attachment 8381907 [details] [diff] [review]
> Patch v1
> 
> review- per comment 13.
> 
> I'd still like to know the answer to my question in comment 5.

dbaron,
Sorry for late reply. Quick answer: NO.
I'm working on Bug 981477 and will go back this bug after that. I think I have to review font-inflation again.

Comment 17

4 years ago
Take this
Assignee: nobody → cku

Comment 18

4 years ago
Created attachment 8446465 [details] [diff] [review]
Figure out which attribute cause font-inflation be disable on B2G::OOP

Comment 19

4 years ago
Created attachment 8446479 [details] [diff] [review]
Figure out which attribute cause font-inflation be disable on B2G::OOP
Attachment #8446465 - Attachment is obsolete: true

Comment 20

4 years ago
Created attachment 8446486 [details] [diff] [review]
work around binding issue on mc

Comment 21

4 years ago
Created attachment 8446487 [details] [diff] [review]
(Debug) Enable font-inflation test cases with more log
Attachment #8446479 - Attachment is obsolete: true

Comment 22

4 years ago
1. nsPresShell disable font-inflation.
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#10638
mFontSizeInflationEnabled be set as false since (a)vInf.IsAutoSizeEnabled()

2. DocShell tells nsDocument the current content is an app.
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7533
The reason of (a) is true is because of docShell->GetIsApp(). We don't really want to enable font-inflation in apps or mobile optimized pages.

Proposed solution:
i. make test page viewport-specific
add this meta tag into font-inflation reftest pages.
<meta name="viewport" content="width=800">

Then, auto size is not enable, fix this bug at #1. All test page in reftest/font-inflation folder need to follow this change.

ii. Find a way to cheat in #2. Let docShell->GetIsApp() return false... Although I still have no idea how to do it.

Comment 23

4 years ago
High level abstraction of this bug

Before Bug 940036
(1.1) no viewport header data = enable font-inflation
(1.2) viewport header data + "width == device-width" = disable font-inflation
(1.3) viewport header data + "width != device-width" = enable font-inflation

B2G::OOP hit (1.1), which is good.

After Bug 940036
(2.1) no viewport header data + is an app = disable font-inflation
(2.2) no viewport header data + is not an app = enable font-inflation
(2.3) viewport header data + "width == device-width" = disable font-inflation
(2.4) viewport header data + "width != device-width" = enable font-inflation

Here. we hit (2.1), to make all font-inflation test case pass as we expect, have to find a way to switch condition to (2.2) or (2.4)

Updated

4 years ago
Summary: [Tracking] (reftest) tests in layout/reftests/font-inflation fail → [reftest::B2G] All reftest cases in layout/reftests/font-inflation fail after enable OOP

Updated

4 years ago
Attachment #8446487 - Attachment description: Enable font-inflation test cases with more log → (Debug) Enable font-inflation test cases with more log

Comment 24

4 years ago
Created attachment 8447165 [details] [diff] [review]
Part 1: skip fake viewport insertion in reftest
Attachment #8381153 - Attachment is obsolete: true
Attachment #8381907 - Attachment is obsolete: true
Attachment #8446486 - Attachment is obsolete: true
Attachment #8446487 - Attachment is obsolete: true

Updated

4 years ago
Attachment #8447165 - Attachment is patch: true

Comment 25

4 years ago
Created attachment 8447183 [details] [diff] [review]
Part 1: skip fake viewport insertion in reftest
Attachment #8447165 - Attachment is obsolete: true

Comment 26

4 years ago
Created attachment 8447185 [details] [diff] [review]
Part 2: Enable font-inflation test

Comment 27

4 years ago
Created attachment 8447186 [details] [diff] [review]
Part 3: (Testing patch) Disable all reftest except font-inflation ones

Comment 28

4 years ago
Created attachment 8447208 [details] [diff] [review]
(Proposal 1)Skip fake viewport insertion in reftest
Attachment #8447183 - Attachment is obsolete: true

Updated

4 years ago
No longer depends on: 976923

Updated

4 years ago
Depends on: 976923

Comment 29

4 years ago
Comment on attachment 8447208 [details] [diff] [review]
(Proposal 1)Skip fake viewport insertion in reftest

Hi dbaron,
Please help to review this patch.

After Bug 940036, an app(reftest OOP page is recognized as an app) with no viewport meta will be append a fake viewport. The reason of doing this is listed at
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7523

Which means, all reftest OOP pages are thought to mobile optimized. As a result. there is no need to apply font-inflation onto.

I add a new preference to prevent fake viewport insertion.
Attachment #8447208 - Flags: review?(dbaron)

Comment 30

4 years ago
Try result:
https://tbpl.mozilla.org/?tree=Try&rev=199b53d85451

The only orange is cause by bug 976923 which is a blocker of this one

Updated

4 years ago
Attachment #8447186 - Attachment is obsolete: true

Updated

4 years ago
Attachment #8447208 - Attachment description: Part 1: skip fake viewport insertion in reftest → Part 1: Skip fake viewport insertion in reftest
I suppose this patch is probably ok, but it seems to me that having nsDocShell::GetIsApp return true for reftest testcases isn't great; is there a way that we could run the reftests in a way that doesn't cause that?  (If that's hard, then perhaps we should just do this, but it seems like it might cause other problems in the future.)

Sorry for the delay getting to this.
Flags: needinfo?(cku)

Comment 32

4 years ago
Hi dbaron,
To distinguish reftest in nsDocShell::GetIsApp, we might need to add a new frame type for reftest frame, not a simple fix but logically doable:
  (origin) eFrameTypeApp: frame type for application. 
  (origin) eFrameTypeRegular: browser or system app on B2G.
  (new) eFrameTypeRefTest: a new from type for ref test page.

The reason that I did not take this approach is, since insert-fake-viewport is a temporary solution, we will eventually remove this solution after all old time apps, with no viewport meta, phase out. We can remove my patch here at the same time when it happens.

IMHO, adding a new frametype only to workaround a problem caused by a temporary solution is not necessary. If we do have another use cases that need to distinguish application and reftesting, I am happy to work on this. In this case, could we have Attachment #8447208 [details] [diff]  check-in to enable font-inflation test case first, and keep this bug open? I will keep finding a way to prevent nsDocShell::GetIsApp return true for a reftest content process. Thanks
Flags: needinfo?(cku) → needinfo?(dbaron)
(In reply to C.J. Ku[:cjku] from comment #32)
> To distinguish reftest in nsDocShell::GetIsApp, we might need to add a new
> frame type for reftest frame, not a simple fix but logically doable:
>   (origin) eFrameTypeApp: frame type for application. 
>   (origin) eFrameTypeRegular: browser or system app on B2G.
>   (new) eFrameTypeRefTest: a new from type for ref test page.

Why would a third type be needed?  Shouldn't reftests act like a page loaded in the browser, and thus use eFrameTypeRegular?
Flags: needinfo?(dbaron)

Updated

4 years ago
Attachment #8447208 - Flags: review?(dbaron)

Updated

4 years ago
Attachment #8447208 - Attachment description: Part 1: Skip fake viewport insertion in reftest → (Proposal 1)Skip fake viewport insertion in reftest

Comment 34

4 years ago
Created attachment 8457892 [details] [diff] [review]
(Proposal 2) Switch from app to browser

Updated

4 years ago
Attachment #8457892 - Attachment is patch: true

Comment 35

4 years ago
Created attachment 8458555 [details] [diff] [review]
(Proposal 2) skip manifest setting
Attachment #8457892 - Attachment is obsolete: true

Updated

3 years ago
Assignee: cku → nobody
I believe this is WONTFIX, given bug 1306391.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.