[Tracking] (reftest) tests in layout/reftests/svg failed on B2G running OOP

RESOLVED FIXED in mozilla34

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: vichen, Assigned: u459114)

Tracking

unspecified
mozilla34
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 53 obsolete attachments)

9.42 KB, image/png
Details
63.81 KB, text/plain
Details
181.26 KB, text/plain
Details
35.93 KB, image/jpeg
Details
4.75 KB, patch
u459114
: review+
Details | Diff | Splinter Review
7.80 KB, patch
u459114
: review+
Details | Diff | Splinter Review
4.49 KB, patch
u459114
: review+
Details | Diff | Splinter Review
Posted file svg.html (obsolete) —
No description provided.
Blocks: B2GRT
Summary: [Tracking] (reftest) a few tests of module svg failed → [Tracking] (reftest) tests in layout/reftests/svg failed
Posted file svg.log (obsolete) —
Posted image wrong viewport size
Some reftests failed like tests/layout/reftests/svg/as-image/zoom/img-fuzzy-zoomOut-1.html, the background of canvas frame is black. The root cause is as below:

PresShell::AddCanvasBackgroundColorItem(...
                                        nscolor               aBackstopColor,
                                        uint32_t              aFlags)
{
    ...
    nscolor bgcolor = NS_ComposeColors(aBackstopColor, mCanvasBackgroundColor);
    ...
    if (AddCanvasBackgroundColor(aList, canvasFrame, bgcolor))
          return;
}

(a) 
'aBackstopColor' is set by drawWindow(..., "rgb(255,255,255)"..) in reftest.js 
(http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js?from=reftest.js&case=true#1416)

(b)
mCanvasBackgroundColor is set as RGBA(0,0,0,255)

(c)
bgcolor is the composed color
The NS_ComposeColors is source over blend(mCanvasBackgroundColor is forground, aBackstopColor is background), which makes bgcolor black since the alpha of mCanvasBackgroundColor is 255.
So the aBackstopColor is always not applied, which means RGB settings doesn't work in drawWindow.

But the code was synced today, I found the background of canvas frame become white. I'm not sure which patch or timing issue fixed(The right setting is coming later). The change is mCanvasBackgroundColor become RGBA(0,0,0,0), so the aBackstopColor is always can be applied.

There is still problem of scrollbar fade-out although the background is right.
I checked Bug 986807, which can seem to solve the problem.

Do you have any thoughts on the background? I have no idea which patch might fix it.
Flags: needinfo?(dbaron)
So there seem to be two related chunks of failures in that log:

 (1) many zoomed-out tests are failing with an incorrect background

 (2) many XBL-related tests are failing

It seems like these two things should be in two separate bugs (and similar failures in other directories should also be in those same bugs).

Given the comments here, maybe this bug should cover (1) and a separate bug should be filed for (2)?


(In reply to Abel Lin(alin, abel) from comment #2)
> But the code was synced today, I found the background of canvas frame become
> white. I'm not sure which patch or timing issue fixed(The right setting is
> coming later). The change is mCanvasBackgroundColor become RGBA(0,0,0,0), so
> the aBackstopColor is always can be applied.

So you're saying this problem no longer exists?

> There is still problem of scrollbar fade-out although the background is
> right.
> I checked Bug 986807, which can seem to solve the problem.

I don't understand what you mean by "checked".  But bug 986404 should have solved the scrollbar fade-out issues.

> Do you have any thoughts on the background? I have no idea which patch might
> fix it.

Do you have a range for when it got fixed?  On what revision was it broken, and in what revision was it fixed?  I'd rather not start trying to figure this out without a reasonable range to start from.

(Also, if it's fixed, why do you need to know which patch fixed it?  Do you want to backport it to another branch?)
Flags: needinfo?(dbaron)
Summary: [Tracking] (reftest) tests in layout/reftests/svg failed → [Tracking] (reftest) tests in layout/reftests/svg failed on B2G running OOP
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #3)
> So there seem to be two related chunks of failures in that log:
> 
>  (1) many zoomed-out tests are failing with an incorrect background
> 
>  (2) many XBL-related tests are failing
> 
> It seems like these two things should be in two separate bugs (and similar
> failures in other directories should also be in those same bugs).
> 
> Given the comments here, maybe this bug should cover (1) and a separate bug
> should be filed for (2)?
> 
> 
> (In reply to Abel Lin(alin, abel) from comment #2)
> > But the code was synced today, I found the background of canvas frame become
> > white. I'm not sure which patch or timing issue fixed(The right setting is
> > coming later). The change is mCanvasBackgroundColor become RGBA(0,0,0,0), so
> > the aBackstopColor is always can be applied.
> 
> So you're saying this problem no longer exists?
Right. But these tests have a regression issue. Now it's ok. I

> 
> > There is still problem of scrollbar fade-out although the background is
> > right.
> > I checked Bug 986807, which can seem to solve the problem.
> 
> I don't understand what you mean by "checked".  But bug 986404 should have
> solved the scrollbar fade-out issues.
I would take a closer look at it when OOP tests are ready to be enabled.
And these tests also have a problem of scrollbar fade-out, fixing all of them once.
> 
> > Do you have any thoughts on the background? I have no idea which patch might
> > fix it.
> 
> Do you have a range for when it got fixed?  On what revision was it broken,
> and in what revision was it fixed?  I'd rather not start trying to figure
> this out without a reasonable range to start from.
I am intended to request a regression test, finding out what resulted in this.
More than a month ago, these test is ok. But I synced the code in a few days,
It was broken. Now it's ok again.
> 
> (Also, if it's fixed, why do you need to know which patch fixed it?  Do you
> want to backport it to another branch?)
No. I am not sure if a side effect make the symptom disappear or really having a patch fix it.
I am afraid these tests are broken again since OOP still isn't enabled to detect it. Just want to realize what patches would affect it in case I can quickly fixed it in the future.
But I plan to do a regression test, using it to find what patches affected it.
Do you have any ideas on the the background of canvas frame?
What can affect the background color?
As the attached, the broken tests are drawn black.

I found these are broken more than ten days ago.
These broken tests are gone(background drawn white) after codebase was synced two days ago.

It seems PresShell::ComputeBackstopColor would afftect the color.
Just clarify why the zooming background of tests are broken intermittently.
Flags: needinfo?(roc)
(In reply to Abel Lin(alin, abel) from comment #5)
> Do you have any ideas on the the background of canvas frame?
> What can affect the background color?
> As the attached, the broken tests are drawn black.
> 
> I found these are broken more than ten days ago.
> These broken tests are gone(background drawn white) after codebase was
> synced two days ago.
> 
> It seems PresShell::ComputeBackstopColor would afftect the color.
Correct -> PresShell::UpdateCanvasBackground
> Just clarify why the zooming background of tests are broken intermittently.
I don't know why without doing more analysis.

If tests are currently passing, enable them on tinderbox and then if they regress again we'll know why!
Flags: needinfo?(roc)
Assignee: nobody → cku
Posted file FAIL log (obsolete) —
Attachment #8460099 - Attachment description: enable skiped svg test cases → Part 1. enable skiped svg test cases
Posted patch Enable skiped SVG test cases (obsolete) — Splinter Review
Attachment #8460099 - Attachment is obsolete: true
Attachment #8460101 - Attachment is obsolete: true
This bug is still there.
I think someone change default background color from black to white, so that we think it had been fixed.

Since it's 100% re-produce-able, I am looking into the root cause.
Attachment #8460133 - Attachment description: Zoom in test case failed → [LOG] failed zoom-in svg test cases
Attachment #8460134 - Attachment description: SVG failed test case → [LOG] failed svg test cases
Posted patch Enable skiped SVG test cases (obsolete) — Splinter Review
Attachment #8460112 - Attachment is obsolete: true
Bug 974242
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/image/image-svg-inline-zoom-out-01a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/image/image-svg-inline-zoom-out-01b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/image/image-svg-inline-zoom-out-01c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/image/image-svg-inline-zoom-out-01d.html | image comparison (==)

bug 1041998 
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/sizing/scrollbars-01.svg | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/sizing/scrollbars-02.svg | image comparison (!=)

Unknown
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/moz-only/foreignObject-zoom-01.svg | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/moz-only/zoom-invalidation-01.svg | image comparison (==)

XBL test case - skip on B2G
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/svg-integration/mask-html-xbl-bound-01.html | image comparison (==)

CJK rendering issue, unknow
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/text-language-00.xhtml | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.74:8888/tests/layout/reftests/svg/text-language-01.xhtml | image comparison (!=)
Attachment #8460145 - Attachment is patch: true
Posted patch Enable skiped SVG test cases (obsolete) — Splinter Review
Attachment #8460145 - Attachment is obsolete: true
Posted patch Enable skiped SVG test cases (obsolete) — Splinter Review
Attachment #8460163 - Attachment is obsolete: true
Posted patch Enable skiped SVG test cases (obsolete) — Splinter Review
Skip foreignObject-zoom-01.svg  & zoom-invalidation-01.svg
Attachment #8460164 - Attachment is obsolete: true
Attachment #8460166 - Flags: review?(ahalberstadt)
Hi Ahal,
We still have several test cases failed in SVG folder.
My plan is to enable test cases which already pass on B2G ASAP and keep this bug open to fix problem that I mentioned in comment 15. Please help to review this patch, thanks

Try result
https://tbpl.mozilla.org/?tree=Try&rev=d71c72ee8dff
Comment on attachment 8460166 [details] [diff] [review]
Enable skiped SVG test cases

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

Awesome! Thanks so much for doing this, it's great to see tests being enabled again :)

::: layout/reftests/svg/reftest.list
@@ +402,5 @@
>  pref(layout.css.mix-blend-mode.enabled,true) == blend-normal.svg blend-normal-ref.svg
> +#skip-if(Android) pref(layout.css.mix-blend-mode.enabled,true) == blend-overlay.svg blend-overlay-ref.svg
> +#skip-if(Android)  pref(layout.css.mix-blend-mode.enabled,true) == blend-saturation.svg blend-saturation-ref.svg
> +#skip-if(Android) pref(layout.css.mix-blend-mode.enabled,true) == blend-screen.svg blend-screen-ref.svg
> +#skip-if(Android) pref(layout.css.mix-blend-mode.enabled,true) == blend-soft-light.svg blend-soft-light-ref.svg

I know this was like this before.. but why are these commented out instead of skipped? Maybe we should uncomment and skip them everywhere (if they are busted).
Attachment #8460166 - Flags: review?(ahalberstadt) → review+
Attachment #8378022 - Attachment is obsolete: true
Attachment #8378038 - Attachment is patch: true
Attachment #8378038 - Attachment mime type: text/x-log → text/plain
Attachment #8378038 - Attachment is obsolete: true
Attachment #8378038 - Attachment is patch: false
Attachment #8460081 - Attachment is obsolete: true
Comment on attachment 8460166 [details] [diff] [review]
Enable skiped SVG test cases

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

::: layout/reftests/svg/reftest.list
@@ +402,5 @@
>  pref(layout.css.mix-blend-mode.enabled,true) == blend-normal.svg blend-normal-ref.svg
> +#skip-if(Android) pref(layout.css.mix-blend-mode.enabled,true) == blend-overlay.svg blend-overlay-ref.svg
> +#skip-if(Android)  pref(layout.css.mix-blend-mode.enabled,true) == blend-saturation.svg blend-saturation-ref.svg
> +#skip-if(Android) pref(layout.css.mix-blend-mode.enabled,true) == blend-screen.svg blend-screen-ref.svg
> +#skip-if(Android) pref(layout.css.mix-blend-mode.enabled,true) == blend-soft-light.svg blend-soft-light-ref.svg

bug 902525 comment 114
These test cases are commented out in check-in patch.
In this bug, I am going to figure out why behavior of snapshot is different with fullZoom change after enable remote browser.

All the failed test cases have zoom out attribute in html tag
<html reftest-zoom="0.5">
Hi,

applying this patch failed with :

patching file layout/reftests/svg/reftest.list
Hunk #1 FAILED at 67
1 out of 3 hunks FAILED -- saving rejects to file layout/reftests/svg/reftest.list.rej

could you maybe rebase this patch ?
Keywords: checkin-needed
Posted patch Enable skiped SVG test cases (obsolete) — Splinter Review
Rebase.
Attachment #8460166 - Attachment is obsolete: true
Attachment #8460821 - Flags: review+
Sorry, had corrected that conflict.
Keywords: checkin-needed
(In reply to C.J. Ku[:cjku] from comment #25)
> Sorry, had corrected that conflict.

thanks! and no problem :) did the checkin now
None-OOP  
  item->GetType = 14. << CANVAS_THEMED_BACKGROUND
  item->GetVisibleRect = 0, 0, 96000, 120000.

OOP
  item->GetType = 14. << CANVAS_THEMED_BACKGROUND
  item->GetVisibleRect = 0, 0, 48000, 60000.
There are two proposals 
1. Enable the size of viewport frame according to full zoom ratio
2. Limit snap shot area
   Don't take full-window snapshot. Only snapshot part of window that we do care.
Attachment #8463140 - Attachment is obsolete: true
Hi Ahal,
This patch is trying to fix test case failure in layout/reftests/svg/image/reftest.list
image-svg-inline-zoom-out-01a.html
image-svg-inline-zoom-out-01b.html
image-svg-inline-zoom-out-01c.html
image-svg-inline-zoom-out-01d.html

Pre-condition:
All these test cases set fullzoom as 0.5
<html reftest-zoom="0.5">

Root cause:
Since we didn't respect fullzoom value in nsDOMWindowUtils::SetCSSViewport, the size of viewport frame and canvas frame shrink from (800, 1000) to (400, 500) device pixels. The size of windows is (800, 1000), as a result, you will see default background color layer while taking snapshot.
Attachment #8463216 - Attachment is obsolete: true
Attachment #8463218 - Flags: review?(ahalberstadt)
Attachment #8463218 - Attachment description: [WIP] Fit canvas frame size with window size in full-zoom-in case → Fit canvas frame size with window size in full-zoom-in case
Attachment #8460821 - Attachment is obsolete: true
Attachment #8463218 - Attachment description: Fit canvas frame size with window size in full-zoom-in case → [Part 1] Fit canvas frame size with window size in full-zoom-in case
Attachment #8463218 - Flags: review?(ahalberstadt)
Attachment #8463218 - Attachment is obsolete: true
Attachment #8463276 - Attachment is patch: true
Attachment #8463276 - Attachment is obsolete: true
Attachment #8463395 - Attachment is patch: true
attachment 8463395 [details] [diff] [review] works fine on my local machine, but still failed on try server.
Need to add more log to identify the problem.
Attachment #8463222 - Attachment is obsolete: true
Attachment #8463395 - Attachment is obsolete: true
Attachment #8396639 - Attachment description: background_black.png → wrong viewport size
Comment on attachment 8464097 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

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

Hi Ahal,
This patch is to fix test case failure in layout/reftests/svg/image/reftest.list
image-svg-inline-zoom-out-01a.html
image-svg-inline-zoom-out-01b.html
image-svg-inline-zoom-out-01c.html
image-svg-inline-zoom-out-01d.html

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=003fd2283090

Pre-condition:
All these test cases set fullzoom as 0.5(zoom out test)
<html reftest-zoom="0.5">

Root cause:
There are two logic errors in this bug
1. Give wrong viewport value in nsDOMWindowUtils::SetCSSViewport
   This function does not respect fullzoom value.
   nsPresContext::CSSPixelsToAppUnits is not multiplied by zoom factor, we should choose presContext->AppUnitsPerDevPixel() here.
   What I changed in nsDOMWindowUtils.cpp is to fix this issue.
2. Viewport change notification is been block 
   nsViewManager::DoSetWindowDimension been suppressed by nsDOMWindowUtils::SetCSSViewport.
   nsDOMWindowUtils::SetCSSViewport is called by TabChild::RecvUpdateDimensions. Ideally, TabChild::RecvUpdateDimensions should be called once during a test run since widget size is fixed.
   After TabChild::RecvUpdateDimensions setting up viewport, nsViewManager::DoSetWindowDimension can "never" change viewport app unit size again. 
   What I did in nsViewManager is to fix this problem.

PS:
I said "Ideally, TabChild::RecvUpdateDimensions should be called once since widget size is fixed during a test run.", but this statement is not totally correct. I noticed TabChild::RecvUpdateDimensions was been called by chrome process many times in one B2G::reftest run. File bug 1045970 to trace this potential problem.
Attachment #8464097 - Flags: review?(ahalberstadt)
Attachment #8464098 - Flags: review?(ahalberstadt)
Comment on attachment 8464097 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

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

Hi C.J, I probably shouldn't review changes to these files. Looking at the logs, I'd try someone like smaug or bz instead.
Attachment #8464097 - Flags: review?(ahalberstadt)
Attachment #8464098 - Flags: review?(ahalberstadt) → review+
Attachment #8464097 - Attachment is obsolete: true
I pushed a full try and notice several mochitest failed because of previous change.
https://tbpl.mozilla.org/?tree=Try&rev=e70202b0e92a

Failure
  test_resize_move_windows.html | Test timed out.
  test_sizetocontent_clamp.html | Test timed out.
  test_sizetocontent_clamp.html | Test timed out.
  test_resize_move_windows.xul | Test timed out.

The latest patch is to prevent unnecessary resize-reflow overridden.
Comment on attachment 8464823 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

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

Hi smaug,
Could you help to review this patch?
Attachment #8464823 - Flags: review?(bugs)
Comment on attachment 8464823 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

Rather layout-ish change.
Attachment #8464823 - Flags: review?(bugs) → review?(dbaron)
Comment on attachment 8464823 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

Handing off to tn, although I'd note that local style is "} else {" all on a single line.
Attachment #8464823 - Flags: review?(dbaron) → review?(tnikkel)
Comment on attachment 8464823 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

>@@ -277,22 +277,24 @@ nsDOMWindowUtils::SetCSSViewport(float a
>-  nscoord width = nsPresContext::CSSPixelsToAppUnits(aWidthPx);
>-  nscoord height = nsPresContext::CSSPixelsToAppUnits(aHeightPx);
>+  int32_t p2a = presContext->AppUnitsPerDevPixel();
>+  nscoord width  = NSFloatPixelsToAppUnits(aWidthPx, float(p2a));
>+  nscoord height = NSFloatPixelsToAppUnits(aHeightPx, float(p2a));

Why do we need to change this to assume the passed in values are in device pixels? The interface in nsIDOMWindowUtils clearly defines them as being in CSS pixels, so that is probably what all callers are expecting.
Attachment #8464823 - Flags: review?(tnikkel)
Attachment #8464823 - Attachment is obsolete: true
Comment on attachment 8465337 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

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

Hi tn,
To address the problem you mentioned, I fix the wrong viewport problem from the source: setup correct nsViewportInfo::mDefaultZoom in nsDocument::GetViewportInfo in this patch.
Please help to review, thanks
Attachment #8465337 - Flags: review?(tnikkel)
Take back mScaleFloat, which is removed by accident
Attachment #8465337 - Attachment is obsolete: true
Attachment #8465337 - Flags: review?(tnikkel)
Attachment #8465536 - Flags: review?(tnikkel)
Comment on attachment 8465536 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

>-    mScaleFloat = LayoutDeviceToScreenScale(scaleStr.ToFloat(&scaleErrorCode));
>+    mScaleFloat = LayoutDeviceToScreenScale(scaleStr.ToFloat(&scaleErrorCode) * fullZoom);

fullzoom is a scale from CSS pixels to layout device pixels. So I don't understand how it makes sense to include the fullzoom in a LayoutDeviceToScreenScale.

I think Matt owns this code though, so I'll defer review on the GetViewportInfo code to him.
Attachment #8465536 - Flags: review?(mbrubeck)
Comment on attachment 8465536 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

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

> I think Matt owns this code though, so I'll defer review on the
> GetViewportInfo code to him.

Uh oh!  I never should have let jwir3 leave.  :)

::: content/base/src/nsDocument.cpp
@@ +7571,5 @@
>      nsAutoString scaleStr;
>      GetHeaderData(nsGkAtoms::viewport_initial_scale, scaleStr);
>  
>      nsresult scaleErrorCode;
> +    mScaleFloat = LayoutDeviceToScreenScale(scaleStr.ToFloat(&scaleErrorCode) * fullZoom);

> fullzoom is a scale from CSS pixels to layout device pixels. So I don't
> understand how it makes sense to include the fullzoom in a
> LayoutDeviceToScreenScale.

I think the change here is basically correct, but I agree with tn that the units here are wrong.  I think we should remove the fullZoom factor from this line...

@@ +7643,4 @@
>      // Now convert the scale into device pixels per CSS pixel.
>      nsIWidget *widget = nsContentUtils::WidgetForDocument(this);
>      CSSToLayoutDeviceScale pixelRatio = widget ? widget->GetDefaultScale()
>                                                 : CSSToLayoutDeviceScale(1.0f);

...and move it here instead, setting pixelRatio = fullZoom * widget scale.  This is consistent with the comment at:
http://hg.mozilla.org/mozilla-central/file/104254bd1fc8/layout/base/Units.h#l146

Would this still give the desired behavior?
Attachment #8465536 - Flags: review?(mbrubeck) → review-
Comment on attachment 8465536 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

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

::: dom/ipc/TabChild.cpp
@@ +312,4 @@
>      if (viewportInfo.GetDefaultZoom().scale < 0.01f) {
>        viewportInfo.SetDefaultZoom(metrics.CalculateIntrinsicScale());
>      }
>  

Line #312~#315 can be removed.

@@ +315,5 @@
>  
>      CSSToScreenScale defaultZoom = viewportInfo.GetDefaultZoom();
>      MOZ_ASSERT(viewportInfo.GetMinZoom() <= defaultZoom &&
>                 defaultZoom <= viewportInfo.GetMaxZoom());
>      metrics.SetZoom(defaultZoom);

I think I should move #316 ~ #319 and #322 ~ #329 out of this if statement.
viewportInfo.GetDefaultZoom may change after first paint
Comment on attachment 8465536 [details] [diff] [review]
[Part 1] Setup correct viewport size according to full-zoom factor

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

::: content/base/src/nsDocument.cpp
@@ +7643,5 @@
>      // Now convert the scale into device pixels per CSS pixel.
>      nsIWidget *widget = nsContentUtils::WidgetForDocument(this);
>      CSSToLayoutDeviceScale pixelRatio = widget ? widget->GetDefaultScale()
>                                                 : CSSToLayoutDeviceScale(1.0f);
> +

Understand.
Please hold on. I notice some persistent failure on Linux(Ripc) after apply a full try. I am reworking this patch.
Attachment #8465536 - Flags: review?(tnikkel)
Comment on attachment 8467549 [details] [diff] [review]
[Part 1] Enable ResizeReflow while FullZoom value change even if ResizeReflow had been overridden

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

Hi tn,
Please help to review this new patch, thanks.

In this patch, I fixed two wrong behaviors
1. ResizeReflow is overridden by TabChild::SetCSSViewport
Changed files:
* nsViewportInfo.h
* nsPresContext.cpp
* nsViewManager.cpp
* nsViewManager.h

Both TabChild::SetCSSViewport and nsViewManager::SetWindowDimensions trigger resize reflow, with one "minor" difference 
* TabChild::SetCSSViewport calls nsPresShell->ResizeReflow"Override"
* nsViewManager::SetWindowDimensions calls nsPresShell->ResizeReflow
When means if we call TabChild::SetCSSViewport before nsViewManager::SetWindowDimensions, nsViewManager::SetWindowDimensions has no way to reset viewport size, according to full-zoom value, again.

2. Wrong viewport setup in TabChild::SetCSSViewport
Changed files:
* TabChild.cpp

With the fix in #1, If nsViewManager::SetWindowDimensions is called after TabChild::SetCSSViewport, everything is fine.
Unfortunately, we can't guarantee this sequence. TabChild::SetCSSViewport might be called before or after nsViewManager::SetWindowDimensions and setup wrong viewport size
TabChild::SetCSSViewport sets viewport size without considerate of full-zoom factor;  nsViewManager::SetWindowDimensions does take fullZoom into account.(http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1482)

The change in TabChild.cpp is to consist behavior of these two functions.

3. Try result
Full test
https://tbpl.mozilla.org/?tree=Try&rev=3ae08b7a2d29
Attachment #8467549 - Flags: review?(tnikkel)
(In reply to C.J. Ku[:cjku] from comment #60)
> 2. Wrong viewport setup in TabChild::SetCSSViewport
> Changed files:
> * TabChild.cpp
> 
> With the fix in #1, If nsViewManager::SetWindowDimensions is called after
> TabChild::SetCSSViewport, everything is fine.
> Unfortunately, we can't guarantee this sequence. TabChild::SetCSSViewport
> might be called before or after nsViewManager::SetWindowDimensions and setup
> wrong viewport size
> TabChild::SetCSSViewport sets viewport size without considerate of full-zoom
> factor;  nsViewManager::SetWindowDimensions does take fullZoom into
> account.(http://dxr.mozilla.org/mozilla-central/source/layout/base/
> nsPresContext.cpp#1482)

Values in appunits or CSS pixels never take into account the fullZoom. nsPresContext::SetFullZoom is a special case because it is trying to maintain the same device pixel size for the window when the app unit to device pixel ratio changes (remember that 1 CSS pixel == 60 app units always). It first gets the existing size of the window in app units. Converts that to the existing window size in device pixels. Then it changes the app units to device pixel ratio with the mDeviceContext->SetPixelScale call. Then it takes the old window size in device pixels and determines how many app units that represents with the new app units to device pixels ratio.

So the change in TabChild.cpp is taking a CSSSize, dividing it by the fullzoom, which converts it into device pixels, but it still holds the value in a CSSSize and passes it to the SetCSSViewport function which is expecting CSS pixels.
Attachment #8467549 - Flags: review?(tnikkel) → review-
nsPresContext::SetFullZoom comes from the desktop era, before we could change the size of the document in CSS pixels for mobile concerns. So perhaps for documents who have had their CSS viewport overridden it makes sense to keep the size of the document constant in CSS pixels in nsPresContext::SetFullZoom instead of device pixels.
Attachment #8469176 - Attachment is obsolete: true
(In reply to Timothy Nikkel (:tn) from comment #61)
> Values in appunits or CSS pixels never take into account the fullZoom.
> nsPresContext::SetFullZoom is a special case because it is trying to
> maintain the same device pixel size for the window when the app unit to
> device pixel ratio changes (remember that 1 CSS pixel == 60 app units
> always). It first gets the existing size of the window in app units.
> Converts that to the existing window size in device pixels. Then it changes
> the app units to device pixel ratio with the mDeviceContext->SetPixelScale
> call. Then it takes the old window size in device pixels and determines how
> many app units that represents with the new app units to device pixels ratio.
Hi tn,
Sorry, I am a little bit confuse here.

If nsPresContext::SetFullZoom wants to maintain the same device pixel size, it essentially needs to change css pixel size, right?

Take fullZoom =0.5 as an example, screen size is (800, 1000) and assume DeviceToScreenScale is 1.0.
If we want to keep device size unchanged, we have to change AppUnit size from (48000, 60000) to (96000, 120000). Then, since 1 css pixel == 60 appunit, we essentially change css size from 
(48000/60, 60000/60) = (800, 1000) 
to 
(96000/60, 120000 / 60) = (1600, 2000).
So, I think viewport css size do change here.
> So the change in TabChild.cpp is taking a CSSSize, dividing it by the
> fullzoom, which converts it into device pixels, but it still holds the value
> in a CSSSize and passes it to the SetCSSViewport function which is expecting
> CSS pixels.
Here, what I really want to do is to stretch the size of viewport size, instead of converting css size into device size.
I would like to write down what I know/ learn regarding to this bug.
==
When running reftest, here is the pre-conditions
1. Window size is (800, 1000)
2. LayoutDevicePixelToScreenScale is 1.0

==
Test case failure condition
1. reftest attribute is added in html tag
   e.g <html reftest-zoom=0.5>
2. Failure ones indeed passed on non-remote-browser platforms, such as desktop browser or B2G before remote iframe turning on

==
The root cause of testing failed is relative to viewport size. 
There are two place setup viewport size

1. TabChild::SetCSSViewport
Unless remote iframe is on, we don't really go through this path.
After TabChild recieve RecvUpdateDimensions, the initial viewport size is (800, 1000) CSS units.
In this path, it pass viewport size via CSS unit. Since FullZoom is 0.5, so CSSToLayoutDeviceSale is 0.5. If we keep viewport size as (800, 1000) CSS units, we can find symptom in attachment 8396639 [details]
  * CSSSize(800, 1000) * CSSToLayoutDeviceScale(0.5) * LayoutDeviceToScreen(1.0) = ScreenSize(400, 500) 

And that's why I inflate CSSSize in TabChild::SetCSSViewport. 
  CSSSize size = aSize / presContext->GetFullZoom(); // Not aSize * presContext->GetFullZoom()

I take presContext->GetFullZoom as a scalar quantity instead of CSS to Device size transform factor. With this stretching, we can get correct viewport size in ResizeReflow
  * CSSSize(800 / 0.5, 1000 / 0.5) * CSSToLayoutDeviceScale(0.5) * LayoutDeviceToScreen(1.0) = ScreenSize(800, 1000)

And this is almost the same with what #2 did.

2. nsViewManager::DoSetWindowDimensions
There are many callers of this function, such as nsPresContext::SetFullZoom or nsPresContext::PreferenceChanged.
All these caller give AppUnit base viewport size into nsViewManager::DoSetWindowDimensions
Take nsPresContext::PreferenceChanged as an expample. This function
  nscoord width = NSToCoordRound(oldWidthDevPixels * AppUnitsPerDevPixel());
  nscoord height = NSToCoordRound(oldHeightDevPixels * AppUnitsPerDevPixel());
  vm->SetWindowDimensions(width, height);
Where AppUnitsPerDevPixel() is effected by FullZoom because of this call(http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1475)
When FullZoom = 0.5, AppUnitsPerDevPixel() actually returns 120, instead of 60. As a result, code in nsPresContext::PreferenceChanged can be translated into
  nscoord width = NSToCoordRound(800 * 60 / 0.5);  
  nscoord height = NSToCoordRound(1000 * 60 / 0.5);
  vm->SetWindowDimensions(96000, 120000); or m->SetWindowDimensions(48000 / 0.5, 60000 / 0.5)
Then
  * AppUnitSize(96000, 120000) = CSSSize(1600, 2000)
  * CSSSize(1600, 2000) * CSSToLayoutDeviceScale(0.5) * LayoutDeviceToScreen(1.0) = ScreenSize(800, 1000)
That's why these tast cases passed on desktop browser.
(In reply to Timothy Nikkel (:tn) from comment #62)
> nsPresContext::SetFullZoom comes from the desktop era, before we could
> change the size of the document in CSS pixels for mobile concerns. So
> perhaps for documents who have had their CSS viewport overridden it makes
> sense to keep the size of the document constant in CSS pixels in
> nsPresContext::SetFullZoom instead of device pixels.
Hi tn,
If we take this approach, all these four test cases will fail on all platforms.
image-svg-inline-zoom-out-01a.html
image-svg-inline-zoom-out-01b.html
image-svg-inline-zoom-out-01c.html
image-svg-inline-zoom-out-01d.html

Please refer to comment 65.
By taking this approach, except SetFullZoom, we have go back to alter  reftest 
  * http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js?from=reftest.js&case=true#1645
This function compare two canvases with full window size. Take attachment 8396639 [details] as an example, if we only compare that left-top porting of window(porting with white background), test case pass correctly on all platforms.

I think we have to ways to fix this bug
1. Make both two paths in comment 65 be CSS unit base
2. Make both two paths in comment 65 be device unit base - patches I submit in this bug take this approach

Like I said in comment 65, we have on CSS unit base path and one device unit path, unless we make it consist, otherwise, it always fails in one of them.

So, do you think #1 make more sense?
Flags: needinfo?(tnikkel)
Posted image Two approaches (obsolete) —
This diagram shows different result of two approaches.
Let's said we have a reftest page which
1. Background color is green
2. Has a fix size div with red background color.
3. default background color in chrome process is white.
Posted image Two approaches
Correct approach number
Attachment #8469430 - Attachment is obsolete: true
From what I understand here, it sounds like the change you're making to the SetCSSViewport codepath is fine. I agree that there the CSS viewport is wrong when a full-zoom is applied. Personally I think the better fix is what Matt said in comment 53 - that is, change the line at [1] to also multiply by the full zoom. In your case, full zoom is 0.5, so pixelRatio will be half of what it used to be, the defaultPixelScale a few lines will be half as well, and so the size will be doubled. This should result in the correct value being passed to SetCSSViewport in TabChild. Can you confirm that this works?

For the other half of your change - the changes to ViewManager. In comment 60 you said "If nsViewManager::SetWindowDimensions is called after TabChild::SetCSSViewport, everything is fine." The problem only happens if the calling order is reversed, because then TabChild::SetCSSViewport overwrites the viewport set in nsViewManager. But that should only be a problem if the viewport set by TabChild::SetCSSViewport is incorrect. If you apply the fix to nsDocument (or even to TabChild) so that it uses the correct CSS viewport, then both SetWindowDimensions and SetCSSViewport will be setting the same value and it won't matter what order they are called in. Is that correct?

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?rev=50b153848427#7642
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #69)
> From what I understand here, it sounds like the change you're making to the
> SetCSSViewport codepath is fine. I agree that there the CSS viewport is
> wrong when a full-zoom is applied. Personally I think the better fix is what
> Matt said in comment 53 - that is, change the line at [1] to also multiply
> by the full zoom. In your case, full zoom is 0.5, so pixelRatio will be half
> of what it used to be, the defaultPixelScale a few lines will be half as
> well, and so the size will be doubled. This should result in the correct
> value being passed to SetCSSViewport in TabChild. Can you confirm that this
> works?
Yes, that's work. 
> For the other half of your change - the changes to ViewManager. In comment
> 60 you said "If nsViewManager::SetWindowDimensions is called after
> TabChild::SetCSSViewport, everything is fine." The problem only happens if
> the calling order is reversed, because then TabChild::SetCSSViewport
> overwrites the viewport set in nsViewManager. But that should only be a
> problem if the viewport set by TabChild::SetCSSViewport is incorrect. If you
> apply the fix to nsDocument (or even to TabChild) so that it uses the
> correct CSS viewport, then both SetWindowDimensions and SetCSSViewport will
> be setting the same value and it won't matter what order they are called in.
> Is that correct?
It's correct only if we call SetFullZoom before TabChild::SetCSSViewport.
1. At reftest-content.js, we setup fullZoom in onLoad event handler.
2. TabChild::SetCSSViewport is triggered by before-first-paint event.
If #1 goes ahead of #2, we can get correct FullZoom value in #2.
In the other hand, if we setup fullZoom anytime after #2, viewport will be wrong again.
Attachment #8464098 - Attachment description: [Part 2] Enable svg zoom-out test cases → [Part 3] Enable svg zoom-out test cases
Attachment #8469825 - Attachment is obsolete: true
Attachment #8469827 - Attachment is patch: true
Comment on attachment 8469827 [details] [diff] [review]
[Part 2] Correct wrong viewport size and scale factor provided by nsDocument

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

Hi Matt,
Please help to review this patch, thanks
Attachment #8469827 - Flags: review?(mbrubeck)
Comment on attachment 8469827 [details] [diff] [review]
[Part 2] Correct wrong viewport size and scale factor provided by nsDocument

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

Why did you rename size to viewportSize here? It makes it much harder to read this patch when you mix functional changes with things like variable renaming or whitespace changes.
Comment on attachment 8469827 [details] [diff] [review]
[Part 2] Correct wrong viewport size and scale factor provided by nsDocument

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

A couple more drive-by comments.

::: content/base/src/nsDocument.cpp
@@ +7646,5 @@
>      // Now convert the scale into device pixels per CSS pixel.
>      nsIWidget *widget = nsContentUtils::WidgetForDocument(this);
> +    CSSToLayoutDeviceScale pixelRatio = widget ?
> +      CSSToLayoutDeviceScale(fullZoom * widget->GetDefaultScale().scale)
> +      : CSSToLayoutDeviceScale(fullZoom);

Would be cleaner to write something like this:
CSSToLayoutDeviceScale pixelRatio = CSSToLayoutDeviceScale(
  (widget ? widget->GetDefaultScale().scale : 1.0f)
  * fullZoom);

@@ +7660,5 @@
> +    else if (fullZoom != 1.0) {
> +      // Stretch CSS pixel size of viewport to keep device pixel size
> +      // unchanged after full zoom applied.
> +      // See bug 974242.
> +      viewportSize = viewportSize / fullZoom;

I don't agree with this part. The meta-viewport tag specifies CSS pixels for the width and height, so we shouldn't be changing this.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #76)
> Comment on attachment 8469827 [details] [diff] [review]
> [Part 2] Correct wrong viewport size and scale factor provided by nsDocument
> 
> Review of attachment 8469827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why did you rename size to viewportSize here? It makes it much harder to
> read this patch when you mix functional changes with things like variable
> renaming or whitespace changes.
I think viewportSize has better meaning compare to size, that why I did that change
I will prevent this later by putting different things into different patches.
Comment on attachment 8469824 [details] [diff] [review]
[Part 1] Enable ResizeReflow while FullZoom value change even if ResizeReflow had been overridden

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

I understand why you are doing this now, based on your previous comments. However I'm not convinced this is the right approach. The SetWindowDimensions function has a bunch of different code paths that might be taken. For example, if the presshell is not visible, it might end up storing the new width/height in mDelayedResize, to be processed later when FlushDelayedResize() is called. In that case you will end up with the wrong value, because you didn't modify the FlushDelayedResize() call to DoSetWindowDimensions.

Also, I'm not sure what the implications are of calling ResizeReflowOverride without also changing the viewport size stored in the FrameMetrics object in TabChild. If that doesn't get updated then things might go bad if it calls SetCSSViewport again later.

::: dom/ipc/TabChild.cpp
@@ -309,5 @@
> -    // FIXME/bug 799585(?): GetViewportInfo() returns a defaultZoom of
> -    // 0.0 to mean "did not calculate a zoom".  In that case, we default
> -    // it to the intrinsic scale.
> -    if (viewportInfo.GetDefaultZoom().scale < 0.01f) {
> -      viewportInfo.SetDefaultZoom(metrics.CalculateIntrinsicScale());

I think this part is still needed. If the initial-scale value in the meta-viewport tag is bad then we can still end up with a 0.0 value for GetDefaultZoom(), and we still need this code. Without it pages will have the wrong initial scale in B2G.
Comment on attachment 8469827 [details] [diff] [review]
[Part 2] Correct wrong viewport size and scale factor provided by nsDocument

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

::: content/base/src/nsDocument.cpp
@@ +7646,5 @@
>      // Now convert the scale into device pixels per CSS pixel.
>      nsIWidget *widget = nsContentUtils::WidgetForDocument(this);
> +    CSSToLayoutDeviceScale pixelRatio = widget ?
> +      CSSToLayoutDeviceScale(fullZoom * widget->GetDefaultScale().scale)
> +      : CSSToLayoutDeviceScale(fullZoom);

For me, they are the same. Both of them are clear.

@@ +7660,5 @@
> +    else if (fullZoom != 1.0) {
> +      // Stretch CSS pixel size of viewport to keep device pixel size
> +      // unchanged after full zoom applied.
> +      // See bug 974242.
> +      viewportSize = viewportSize / fullZoom;

how about do it while mValidWidth && mValidHeight are both false?
Attachment #8469827 - Flags: review?(mbrubeck)
(In reply to C.J. Ku[:cjku] from comment #80)
> 
> how about do it while mValidWidth && mValidHeight are both false?

I think instead it makes more sense to change this line:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?rev=fcedb0c795f7#7628

to divide the result of Preferences::GetInt by the fullzoom. I think that should have the equivalent effect as only doing it when mValidWidth and mValidHeight are false.
Attachment #8469827 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #79)
> Comment on attachment 8469824 [details] [diff] [review]
> [Part 1] Enable ResizeReflow while FullZoom value change even if
> ResizeReflow had been overridden
> 
> Review of attachment 8469824 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I understand why you are doing this now, based on your previous comments.
> However I'm not convinced this is the right approach. The
> SetWindowDimensions function has a bunch of different code paths that might
> be taken. For example, if the presshell is not visible, it might end up
> storing the new width/height in mDelayedResize, to be processed later when
> FlushDelayedResize() is called. In that case you will end up with the wrong
> value, because you didn't modify the FlushDelayedResize() call to
> DoSetWindowDimensions.
Understand
> Also, I'm not sure what the implications are of calling ResizeReflowOverride
> without also changing the viewport size stored in the FrameMetrics object in
> TabChild. If that doesn't get updated then things might go bad if it calls
> SetCSSViewport again later.
Thanks, I don't know either. I will figure it out
Actually, I think the reason of disable the following test cases might be the same(wrong viewport size)
zoom-out test cases
  layout/reftests/bugs/446100-1h.html
  layout/reftests/bugs/446100-1e.html 
  layout/reftests/bugs/446100-1f.html 
  layout/reftests/bugs/446100-1g.html 
  layout/reftests/bugs/446100-1h.html 
  layout/reftests/bugs/621253-1-externalFilter.html 
  layout/reftests/bugs/621253-1-internalFilter.html 
  layout/reftests/svg/image/image-svg-inline-zoom-out-01a.html
  layout/reftests/svg/image/image-svg-inline-zoom-out-01b.html
  layout/reftests/svg/image/image-svg-inline-zoom-out-01c.html
  layout/reftests/svg/image/image-svg-inline-zoom-out-01d.html 

Meanwhile, there are 4 zoom out test cases are already enable on B2G. If the background color of a testing page is the same with default background color(white), then viewport problem is been hidden.
  layout/reftests/bugs/618071.html 
  layout/reftests/svg/as-image/zoom/img-zoomOut-1.html
  layout/reftests/svg/image/image-svg-inline-sprite-zoom-out-01a.html
  layout/reftests/svg/image/image-svg-inline-sprite-zoom-out-01b.html
I am wondering why this problem does not appear on mochitest.

function zoomDocument defined in layout.js and is widely used by mochitest test case
http://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/layout.js#64

Fortunately, in mochitest, there are only zoom-in test.
zoomDocument(document, 1.5); // zoom in
zoomDocument(document, 2.0); // zoom in
zoomDocument(document, 1.0); // zoom in
Attachment #8469924 - Attachment is obsolete: true
Attachment #8469935 - Attachment is obsolete: true
Attachment #8470534 - Attachment is obsolete: true
Comment on attachment 8470752 [details] [diff] [review]
[Part 2] Reset TabChild's root frame metrics when fullZoom change

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

Hi kats,
May I have your opinion of this change?

In this patch, TabChild listens to "FullZoomChange" event, then trigger resize reflow and change mLastRootMetric accordingly. So, there is no need to override viewport in DoSetWindowDimension path.
Attachment #8470752 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8470752 [details] [diff] [review]
[Part 2] Reset TabChild's root frame metrics when fullZoom change

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

This patch is definitely more along the right track, I think. It makes more sense to just listen for the event and handle it appropriately. However I'm not sure why you need to set the first-paint flag in this case. Can you explain that?
Attachment #8470752 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #91)
> Comment on attachment 8470752 [details] [diff] [review]
> [Part 2] Reset TabChild's root frame metrics when fullZoom change
> 
> Review of attachment 8470752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is definitely more along the right track, I think. It makes more
> sense to just listen for the event and handle it appropriately. However I'm
> not sure why you need to set the first-paint flag in this case. Can you
> explain that?
It's because of [1].

TabChild may receive fullZoomChange event before or after the first paint. Without first-paint flag, we can not update metrics.mZoom and metrics.mDevPixelsPerCSSPixel correctly.

Is it safe to pull metrics.mZoom and metrics.mDevPixelsPerCSSPixel assignment out of if (NS_FAILED(rv) || isFirstPaint) statement?  

[1]: http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#308
It should be safe to pull the mDevPixelsPerCSSPixel assignment out, but not the mZoom. Looking at the code just above the isFirstPaint block, there is a call to FrameMetrics::ZoomBy. I would have expected that call to update the zoom to account for the change in the CSS viewport. If it doesn't do that then we should figure out why not. If for example the fullZoom goes from 1.0 to 0.5 then i would expect the CSS viewport to become twice as wide and so the intrinsic scale becomes halved. So the zoomBy call should reduce the zoom by half as well.
Attachment #8464098 - Attachment is obsolete: true
Attachment #8470750 - Attachment is obsolete: true
Attachment #8470752 - Attachment is obsolete: true
Attachment #8472039 - Attachment is obsolete: true
Attachment #8472040 - Attachment is obsolete: true
Attachment #8472042 - Attachment is obsolete: true
Attachment #8472041 - Attachment is obsolete: true
Attachment #8472075 - Attachment is obsolete: true
Attachment #8472090 - Attachment is obsolete: true
Comment on attachment 8472043 [details] [diff] [review]
Part 3. Enable zoom-out reftest case on B2G

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

Hi ahal,
Please review again. With [Part 1] + [Part 2], we can enable all zoom-out test cases on B2G, except 446100-1g.html(see bug 1053036).
Attachment #8472043 - Flags: review?(ahalberstadt)
Flags: needinfo?(tnikkel)
Comment on attachment 8472094 [details] [diff] [review]
Part 1. Bring FullZoom into nsDocument::GetViewportInfo

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

Hi Matt/Kats,
Please help to review this patch.
Here, I bring FullZoom into nsDocument::GetViewportInfo to provide correct default scale and viewport size to the caller, TabChild.
Attachment #8472094 - Flags: review?(mbrubeck)
Attachment #8472094 - Flags: review?(bugmail.mozilla)
Comment on attachment 8472066 [details] [diff] [review]
Part 2. Registry and handle FullZoomChange chrome event in TabChild

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

Hi tn/ kats,
Please help to review this patch.
Base on your comment, I rewrited the way of handling viewport change. In this version, TabChild recieves "FullZoomChange" chrome event and handles this event in HandlePossibleViewportChange.
Attachment #8472066 - Flags: review?(tnikkel)
Attachment #8472066 - Flags: review?(bugmail.mozilla)
Comment on attachment 8472094 [details] [diff] [review]
Part 1. Bring FullZoom into nsDocument::GetViewportInfo

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

::: content/base/public/nsViewportInfo.h
@@ +27,5 @@
>    public:
>      nsViewportInfo(const mozilla::ScreenIntSize& aDisplaySize,
> +                   bool aAllowZoom = true,
> +                   bool aAllowDoubleTapZoom = true,
> +                   const mozilla::CSSToScreenScale& aDefaultZoom = mozilla::CSSToScreenScale(1.0)) :

Since you're changing all of the call sites to provide all of the arguments, you might as well remove the default values for these parameters.

Alternatively, you could move the aDefaultZoom up to be second parameter like so:

nsViewportInfo(const mozilla::ScreenIntSize& aDisplaySize,
               const mozilla::CSSToScreenScale& aDefaultZoom,
               bool aAllowZoom = true,
               bool aAllowDoubleTapZoom = true)

Either way is fine by me, or you can leave it as-is, I don't feel too strongly about this.
Attachment #8472094 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8472066 [details] [diff] [review]
Part 2. Registry and handle FullZoomChange chrome event in TabChild

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

This looks good to me, thanks!

::: dom/ipc/TabChild.cpp
@@ +769,5 @@
>      // This meta data may or may not have been a meta viewport tag. If it was,
>      // we should handle it immediately.
>      HandlePossibleViewportChange(mInnerSize);
>    }
> +  else if (eventType.EqualsLiteral("FullZoomChange")) {

Move the else up onto the previous line: } else if (...) {
Attachment #8472066 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8472043 [details] [diff] [review]
Part 3. Enable zoom-out reftest case on B2G

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

I love these reviews :p
Attachment #8472043 - Flags: review?(ahalberstadt) → review+
Attachment #8472066 - Flags: review?(tnikkel) → review+
Blocks: 1053975
Attachment #8472094 - Flags: review?(mbrubeck) → review+
I am figuring out two failures in full try
https://tbpl.mozilla.org/?tree=Try&rev=faa6fe2ce72a
B2G ICS Emulator Opt M(9) 100% fail
Linux Opt Cipc 50% fail rate
Oops.. wrong url, here is the correct one
https://tbpl.mozilla.org/?tree=Try&rev=81cc062d0238
M(9) testing fail
11:23:12     INFO -  281 INFO TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug470212.html | rangeCount should be 0 - got 1, expected 0

Cipc - application crash
1:26:04  WARNING -  PROCESS-CRASH | file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/379105-1.xhtml | application crashed [@ nsDocShell::SetupNewViewer(nsIContentViewer*)]
11:26:04     INFO -  Crash dump filename: /tmp/tmphQV0FK.mozrunner/minidumps/0550c416-bbd4-5c4a-557bf5ce-0313ba8b.dmp
bug 1055040 is relative to M(9) testing fail
rebase and carry r+
Attachment #8472094 - Attachment is obsolete: true
Attachment #8474660 - Flags: review+
rebase and carry r+
Attachment #8472066 - Attachment is obsolete: true
Attachment #8474662 - Flags: review+
rebase and carry r+
Attachment #8472043 - Attachment is obsolete: true
Attachment #8474663 - Flags: review+
Depends on: 1055040
Fix comment 105
Attachment #8474660 - Attachment is obsolete: true
Attachment #8475888 - Flags: review+
(In reply to C.J. Ku[:cjku] from comment #117)
> Created attachment 8475890 [details] [diff] [review]
> Part 2. Registry and handle FullZoomChange chrome event in TabChild
> 
> Fix random crash on Cipc
> https://tbpl.mozilla.org/php/getParsedLog.php?id=45955940&tree=Try&full=1
Try result
https://tbpl.mozilla.org/?tree=Try&rev=ab2cf7dde449
Full try result
https://tbpl.mozilla.org/?tree=Try&rev=f042b0aecea9

If the testing result is good, these patches are ready to land after bug 1055040
Keep away verbose FullZoomChange notification
Attachment #8475890 - Attachment is obsolete: true
Attachment #8476523 - Flags: review+
rebase
Attachment #8475888 - Attachment is obsolete: true
rebase
Attachment #8476523 - Attachment is obsolete: true
Attachment #8476527 - Flags: review+
Attachment #8476526 - Flags: review+
Attachment #8476527 - Attachment is obsolete: true
Comment on attachment 8476715 [details] [diff] [review]
Part 2. Registry and handle FullZoomChange chrome event in TabChild

Hi tn/kats,
I did a change in nsDocumentViewer.cpp. It prevents nsDocumentViewer sending out redundant "FullZoomChange" events, which introduce heavy resize actions in HandlePossibleViewportChange.

Please review again, thanks.

Try result
https://tbpl.mozilla.org/?tree=Try&rev=f6ac411d8051
(B2G ICS Emulator Opt M(7): bug 968783)
Attachment #8476715 - Flags: review?(tnikkel)
Attachment #8476715 - Flags: review?(bugmail.mozilla)
Attachment #8476715 - Attachment is obsolete: true
Attachment #8476715 - Flags: review?(tnikkel)
Attachment #8476715 - Flags: review?(bugmail.mozilla)
Attachment #8476838 - Flags: review?(tnikkel)
Attachment #8476838 - Flags: review?(bugmail.mozilla)
Attachment #8476838 - Flags: review?(tnikkel) → review+
Comment on attachment 8476838 [details] [diff] [review]
Part 2. Registry and handle FullZoomChange chrome event in TabChild

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

::: dom/ipc/TabChild.cpp
@@ +764,5 @@
>      // This meta data may or may not have been a meta viewport tag. If it was,
>      // we should handle it immediately.
>      HandlePossibleViewportChange(mInnerSize);
>    }
> +  else if (eventType.EqualsLiteral("FullZoomChange")) {

move the else up to the same line as the }

::: layout/base/nsDocumentViewer.cpp
@@ +3038,5 @@
>    if (!mDocument) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  bool fullZoomChange = (mPageZoom == aFullZoom) ? false : true;

bool fullZoomChange = (mPageZoom != aFullZoom);
Attachment #8476838 - Flags: review?(bugmail.mozilla) → review+
Attachment #8476838 - Attachment is obsolete: true
Attachment #8477149 - Flags: review+
checkin-needed

full try result
https://tbpl.mozilla.org/?tree=Try&rev=f6ac411d8051
(B2G ICS Emulator Opt M(7): bug 968783)
Although I haven't confirmed it, I think this patch regressed image/test/reftest/downscaling/downscale-svg-1d.html , which was reenabled on mozilla-inbound by bug 1043560 and started failing on B2G only after a merge.  See bug 1043560 comment 82.
Comment on attachment 8477149 [details] [diff] [review]
Part 2. Registry and handle FullZoomChange chrome event in TabChild

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

::: dom/ipc/TabChild.cpp
@@ +330,5 @@
>  
> +  if (nsIPresShell* shell = document->GetShell()) {
> +    if (nsPresContext* context = shell->GetPresContext()) {
> +      metrics.mDevPixelsPerCSSPixel = CSSToLayoutDeviceScale(
> +        (float)nsPresContext::AppUnitsPerCSSPixel() / context->AppUnitsPerDevPixel());

It looks like metrics.mDevPixelsPerCSSPixel isn't a floating point quantity, so possibly the issue lies in how rounding is handled here. It might be that rounding away from zero is better. Looking at the actual numbers we see here during the failing test should make it clear what's going on.
I filed bug 1057881 on fixing the dowwnscale-svg-1d.html failure.
You need to log in before you can comment on or make changes to this bug.