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

Status: RESOLVED WONTFIX

(Testing :: Reftest, defect)

Description

[Abel Lin(alin, abel)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 2

[Abel Lin(alin, abel)]

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

Comment 3

[Abel Lin(alin, abel)]

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

Comment 4

[Vincent Chen [:vichen]]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are you saying that bug 940036 disabled font inflation entirely on B2G?

Comment 6

[Vincent Chen [: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.

Comment 7

[Vincent Chen [: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.

Comment 8

[Vincent Chen [:vichen]]

Attachment: skip font-inflation reftests in B2G/OOP

Comment 9

[Vincent Chen [:vichen]]

Attachment: 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.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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 ?

Comment 11

[Vincent Chen [:vichen]]

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

Comment 12

[Andrew Halberstadt [:ahal]]

Yes, when you set the pref reftest.browser.iframe.enabled=True then it will use an <iframe mozbrowser> instead of a <xul:window>.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 16

[Vincent Chen [:vichen]]

(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

[u459114]

Take this

Comment 18

[u459114]

Attachment: Figure out which attribute cause font-inflation be disable on B2G::OOP

Comment 19

[u459114]

Attachment: Figure out which attribute cause font-inflation be disable on B2G::OOP

Comment 20

[u459114]

Attachment: work around binding issue on mc

Comment 21

[u459114]

Attachment: (Debug) Enable font-inflation test cases with more log

Comment 22

[u459114]

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

[u459114]

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)

Comment 24

[u459114]

Attachment: Part 1: skip fake viewport insertion in reftest

Comment 25

[u459114]

Attachment: Part 1: skip fake viewport insertion in reftest

Comment 26

[u459114]

Attachment: Part 2: Enable font-inflation test

Comment 27

[u459114]

Attachment: Part 3: (Testing patch) Disable all reftest except font-inflation ones

Comment 28

[u459114]

Attachment: (Proposal 1)Skip fake viewport insertion in reftest

Comment 29

[u459114]

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.

Comment 30

[u459114]

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

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 32

[u459114]

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

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 34

[u459114]

Attachment: (Proposal 2) Switch from app to browser

Comment 35

[u459114]

Attachment: (Proposal 2) skip manifest setting

Comment 36

[Daniel Holbert [:dholbert]]

I believe this is WONTFIX, given bug 1306391.