Closed Bug 795657 Opened 13 years ago Closed 13 years ago

B2G meta viewport isn't setting zoom correctly, but resolution looks ok

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: drs, Assigned: cjones)

References

Details

Attachments

(2 files, 18 obsolete files)

don't reframe for adding a transform when absolute descendants are present, when the frame is already positioned
4.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Rollup
50.39 KB, patch
cjones
: checkin+
Details | Diff | Splinter Review
Go to nytimes.com and the page will load at a low res, which it should, but the graphics code doesn't seem to be zooming it correctly (should be zoomed out).
Yuck, looks like "before-first-paint" is coming _after_ the layers transaction marked with aFirstPaint==true 0_o
Confirmed, nytimes.com: I/Gecko (31138): **********FIRST PAINT: 1.000000 1.000000 D/memalloc(31138): /dev/pmem: Freeing buffer base:0x4bbf7000 size:245760 offset:4157440 fd:183 D/memalloc(31255): /dev/pmem: Unmapping buffer base:0x44a63000 size:4403200 offset:4157440 D/memalloc(31138): /dev/pmem: Freeing buffer base:0x4bae4000 size:245760 offset:3031040 fd:199 I/Gecko (31255): !!!!!!!!!!!!!!!!!!!!!! BEFORE-FIRST-PAINT I/Gecko (31255): *************** HANDLEPOSSIBLEMETAVIEWPORTCHANGE I/Gecko (31255): {"x":0,"y":0,"zoom":0.326531,"displayPort":{"x":0,"y":0,"width":980,"height":1117,"resolution":0.326531},"compositionBounds":{"x":0,"y":0,"width":320,"height":365},"cssPageRect":{"x":0,"y":0,"width":980,"height":1117}} The sequence of events happening is: 1) First paint is coming in with the default resolution of 1.0, 1.0. 2) before-first-paint hits, and the resolution is changed to 0.326531 (which is 320/980) 3) APZC gets a layers update, but doesn't accept the new resolution since it isn't marked as a first paint 4) The next repaint brings us back to 1.0 I'm pretty sure this is a recent regression. Not really sure what to do from here but will keep digging.
blocking-basecamp: --- → ?
This seems to be causing some website mis-renders.
blocking-basecamp: ? → +
We don't get to nsLayoutUtils::PaintFrame() with firstpaint=true on the first gdb run where I saw this.
We hit firstpaint=true for a PaintType_Composite paint. Will see what's going on there. I think dlbi changed the timing here.
Assignee: nobody → jones.chris.g
It looks like we get *three* viewport updates during page load. That can't be right!
We get a crapton of viewport updates while just panning around. Doug, is that expected?
gdb says that at if (!screenW || !screenH) { Breakpoint 5, mozilla::dom::TabChild::HandlePossibleMetaViewportChange (this=0x43354530) at /home/cjones/mozilla/inbound/dom/ipc/TabChild.cpp:378 w = 320, h = 0 That doesn't look right!
What I'm seeing in gdb (in the browser process) is different than comment 0, but the results end up the same somehow - 3 HandlePossibleMetaViewportChange() (sigh), each of which computes a zoom of 0.32653060555458069. - first paint in PresShell, which sets the layers firstpaint flag - 10 more HandlePossibleMetaViewportChange() (siiiiiiiiiigh) The firstpaint flag on layers is "sticky", not reset until we actually forward a transaction. So this is not expected.
OK, after adjusting my breakpoints, I'm able to see what doug sees in comment 0. I'm afraid the layers txn that unsets the firstpaint flag might be clearing out the tree of the previous page :/. This is a pretty terribly fragile mechanism, will think on something better.
Looks like the backstop background. Hmm .... I/Gecko (30728): First-paint transaction; edits are I/Gecko (30728): CreateContainerLayer I/Gecko (30728): CreateThebesLayer I/Gecko (30728): CreateColorLayer I/Gecko (30728): AppendChild I/Gecko (30728): InsertAfter I/Gecko (30728): SetRoot I/Gecko (30728): SetLayerAttributes I/Gecko (30728): SetLayerAttributes I/Gecko (30728): SetLayerAttributes
(In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > We get a crapton of viewport updates while just panning around. > > Doug, is that expected? No, but note that if the URL bar is being hidden or shown there are several viewport updates during this process.(In reply to Chris Jones [:cjones] [:warhammer] from comment #9) > What I'm seeing in gdb (in the browser process) is different than comment 0, > but the results end up the same somehow > - 3 HandlePossibleMetaViewportChange() (sigh), each of which computes a > zoom of 0.32653060555458069. > - first paint in PresShell, which sets the layers firstpaint flag > - 10 more HandlePossibleMetaViewportChange() (siiiiiiiiiigh) > > The firstpaint flag on layers is "sticky", not reset until we actually > forward a transaction. So this is not expected. I can recall there being several calls to it around first paint, but there shouldn't be this many.
Possibly related to the extra viewport updates, I see us hitting if (!mContentDocumentIsDisplayed) { return; } in TabChild::HandlePossibleMetaViewportChange() numerous times, well after the page is loading and partially displayed.
This fixes nytimes and a few other desktop-scale sites I tested. It also seems to work well with facebook and twitter. However, we're now being served m.cnn.com again, and it really flips out; we end up zoomed in by what looks like a factor of over 100x. Not sure if this patch broke things or it was already like that. Will investigate.
Attachment #668124 - Flags: review?(bugzilla)
m.cnn.com doesn't flip out without this patch, but we also don't apply a viewport configuration, so I don't know whether we're accidentally getting away with that.
The behavior is different before/after this patch, but both behaviors are insane so I think there's another bug somewhere Before I/Gecko (22743): VIEWPORT computed zoom factor 2.95757e-07 I/Gecko (22743): VIEWPORT computed zoom factor 0 After I/Gecko (23354): VIEWPORT computed zoom factor 721.903 I/Gecko (23354): VIEWPORT computed zoom factor 0 Will file a followup. I think I'll some sanity checking here. It's not a good idea to zoom in 700x!
We're getting some completely bonkers data from somewhere I/Gecko (24536): VIEWPORT last zoom 0.326531, zoomScale 9.05841e-07, new zoom 2.95785e-07, clamping to (-6.98217e-07, 2.07784e+08) I/Gecko (24536): VIEWPORT last zoom 2.95785e-07, zoomScale 2.96402e+06, new zoom 0.876712, clamping to (1.74498e+08, 5.35786e-315)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #14) > Created attachment 668124 [details] [diff] [review] > Attempt to make viewport resets more robust > > This fixes nytimes and a few other desktop-scale sites I tested. It also > seems to work well with facebook and twitter. > > However, we're now being served m.cnn.com again, and it really flips out; we > end up zoomed in by what looks like a factor of over 100x. Not sure if this > patch broke things or it was already like that. Will investigate. This sounds like a really bad workaround for a bigger problem going on beneath the hood. It seems so completely wrong to me to add a work around for "before event X" coming _after_ "event X" as opposed to fixing the ordering, which should make the fix fall in place on its own.
This code relies intimately on layers transaction semantics, which is *not* the same thing as painting content. We now use layer trees for widget invalidation. This first layers transaction is a Paint_CompositeOnly, which we block from forwarding to the compositor. The second transaction is Paint_NoComposite, with what appears to be the backstop color. I'm not entirely sure what this is, but it seems to be resetting the layer tree for the purposes of widget invalidation. (Matt?) This transaction is forwarded across. Is that a bug? Not sure. It's a very bad idea to tie the semantics of beforefirstpaint to layer transactions, since that's an internal implementation detail. Based on what I can tell from my logging, we're firing beforefirstpaint right around the first paint of the actual content. The problem is, TabChild is making assumptions about layer transactions that are being violated. Matt, do you have an opinion on this?
Comment on attachment 668124 [details] [diff] [review] Attempt to make viewport resets more robust This patch does a pretty bad job on the (dirt simple) tests here http://www.quirksmode.org/m/tests/widthtest.html . Sometimes works sometimes doesn't. Not sure if it's enough of an improvement.
Attachment #668124 - Attachment is obsolete: true
Attachment #668124 - Flags: review?(bugzilla)
Another reason why we can't tie this code to layers transactions and beforefirstpaint timing is that we need to dynamically force viewport resets. For example, the test here uses width=device-width. http://www.quirksmode.org/m/tests/widthtest_vpdevice.html If you load it in portrait orientation, and then change to landscape orientation, the page should reflow to the new device-width and update resolution/zoom to match. Both fennec-android and android-browser do this. The current native code, however, does indeed reflow, but doesn't reset the zoom because tabchild/apzc are confusing each other.
This patch was also leading to scrolling glitches, I think because we were taking whatever value happened to be set when seeing firstpaint.
- it looks like TabChild doesn't look at ViewportInfo.autoSize. I think that's because any potential change "widget" to widget geometry will re-request HandlePossibleMetaViewportChange(). That seems fine. - I don't see where TabChild honors defaultZoom. I think that's just a bug. Will file. - APZC doesn't look at viewport changes, but I don't think it needs to care about them. The displayport is calculated from composition bounds. So OK. - APZC doesn't look at "widget" geometry changes in /FrameMetrics/, but we have the out-of-band UpdateCompositionBounds() to handle that. This means that the layer tree will be updated non-atomically with geometry changes, but I don't think we understand the implications of that well. Fine enough for v1. - what *does* seem to be a problem is that there's no notion of "intrinsic scale" in APZC. That is, f.e. if nsContentUtils::GetViewportInfo() computes us a viewport of 980x1117, and our widget geometry is 320x360, then the "intrinsic scale" is 320/980, 0.327. This is the value that "user zoom" should be applied on top of. TabChild computes this as |zoomScale|, but AFAICT APZC doesn't do this. This is a bug. So there are several scale-y/zoom-y values that are relevant - intrinsic scale. Only changed by widget geometry or viewport updates. - user zoom. Only changed by <meta viewport> or, you know, the user. - last-paint resolution. intrinsicScale*userZoom at time of content layers txn. - target resolution. The current intrinsicScale*userZoom in APZC. - (DPI too, but we can consider that to be a constant.) The bug here is arising because we don't separate user zoom from intrinsic scale when comparing FrameMetrics resolutions. This also causes the bug where we "zoom in" on switching portrait->landscape. Doug, does all that makes sense to you? You're in the best position to cook up a fix; let me know if you've got the cycles.
After we fix this bug, then the firstpaint event issue will prevent bug 798245 from working reliably. But that's a much less severe problem.
An amusing effect of this bug is that repeatedly changing the orientation of the phone keep zooming in each time.
I think I'm going to have to rewrite ZoomToRect(). Sorry.
Attached patch Core of the change (obsolete) — — Splinter Review
I'm still working through the code affected by this change, but wanted to get the meat of it posted for review asap. roc, also, drs is swamped with school, so the bus factor for this code is down to 1 + \epsilon. Is there someone else we can pull in to help out? The core of the change is, previously FrameMetrics.mZoom meant "target resolution". Now it means, "user zoom", independent of resolution. The target resolution (mZoom) was previously calculated as zoom = (compositionBounds / viewport) * userZoom but we can calculate |(compositionBounds / viewport)| (the "intrinsic scale") anytime we want from other data in FrameMetrics. So we let mZoom become just the "user zoom". This lets us preserve the user's zoom across viewport changes, orientation changes, etc.
Attachment #668702 - Flags: review?(roc)
Attachment #668702 - Flags: feedback?(bugzilla)
Comment on attachment 668702 [details] [diff] [review] Core of the change Review of attachment 668702 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.
Attachment #668702 - Flags: review?(roc) → review+
I have the viewport and resolution calculations fixed up, but double-tap zoom is horked. There are a few more bugs introduced by the nsContentUtils user-zoom limits (0.0-10.0 is the default range :/).
Double-tap fixed. Looks like there are some fallout bugs in scroll clamping and displayport calculations (seeing NaNs :/).
A problem is that by accepting whatever content has computed as the current CSS viewport, we end up oscillating on widget geometry changes. I think I know how to fix this.
Alright, looking good. Need to find reviewers now.
I have a number of patches enqueued, but I tried very hard to break them into bite-size reviews. The patches work towards the zoom/resolution redefinition, fixing little bugs that arose here and there. There's nothing particularly subtle about any of the patches.
Addresses an old review comment that wasn't.
Attachment #669478 - Flags: review?(roc)
During reflows, sometimes nsFrameLoader/TabParent see updates of size to 0,0. This happens off of a script blocker. This causes a lot of problems when we rely on those dimensions to compute resolution. I don't know why we see that intermediate state during reflow. This patch might cause us to hang on to layers for "background tabs" longer than we should, but I suspect we would want a different fix for that.
Attachment #669479 - Flags: review?(roc)
When there's no defaultZoom or width set in a viewport configuration, nsContentUtils::GetViewportInfo() will return defaultZoom == 0.0. TabChild trusts that value, and getting 0.0 back caused some bad problems. I don't really know what the expected result in this case is supposed to be. This patch adds a guard to TabChild.
Attachment #669480 - Flags: review?(sjohnson)
This is the helper we'll be using now that FrameMetrics has been fixed up to always be internally consistent.
Attachment #669483 - Flags: review?(roc)
This is one of the important patches in this series. What we do here is have apzc accept the CSS viewport that content has calculated, *only* when it's composition bounds match what we expect. This lets apzc naturally adjust when the screen is rotated, e.g. It just picks up the new viewport, and further calculations of resolution adjust to the new intrinsic scale induced by that viewport.
Attachment #669489 - Flags: review?(roc)
Implements the resolution-independent "user zoom" vs. render resolution introduced in the first patch here. Mechanical change.
Attachment #669490 - Flags: review?(roc)
(These patches are to be folded together for landing.)
Comment on attachment 669480 [details] [diff] [review] nsContentUtils::GetViewportInfo() can return a defaultZoom of 0.0, which is bad Review of attachment 669480 [details] [diff] [review]: ----------------------------------------------------------------- Perhaps we should add a test that tests this condition? It seems like nsContentUtils::GetViewportInfo is incorrect here... a defaultZoom of 0.0 shouldn't be allowed. Although, after looking at the code, it seems like this could happen only if the page specified an initial-scale of 0.0 (stupid), or if DevicePixelsPerMetaViewportPixel() returns 0 (which is not possible). So, I do think this is an exceptional case, and we should probably test for it
Attachment #669480 - Flags: review?(sjohnson) → review+
OK. I mostly wanted to confirm that the value returned from GetViewportInfo() is intended to be used, and that 0.0 isn't a placeholder for "leave it up to the frontend".
There's one other unfortunately ambiguity here ... the values being computed are "scales", i.e. what we would apply for resolution scaling, but the apzc code is interpreting them as "zooms" (resolution independent). I think this is causing a problem on http://www.quirksmode.org/m/tests/widthtest_vp380.html . Seeing what fennec does with that page.
Comment on attachment 669479 [details] [diff] [review] Ignore updates of dimensions to w=0,h=0 Review of attachment 669479 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a workaround for some other bug. Can we look more closely into why these 0,0 sizes are being sent?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48) > Comment on attachment 669479 [details] [diff] [review] > Ignore updates of dimensions to w=0,h=0 > > Review of attachment 669479 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks like a workaround for some other bug. Can we look more closely > into why these 0,0 sizes are being sent? I imagine you're right, but I was hoping you might be able to provide guidance on where to look. Is it expected that we might see dimensions of 0x0 during normal reflow?
Comment on attachment 669480 [details] [diff] [review] nsContentUtils::GetViewportInfo() can return a defaultZoom of 0.0, which is bad Review of attachment 669480 [details] [diff] [review]: ----------------------------------------------------------------- Seems like we should fix this in GetViewportInfo()? Seems to me that kViewportMinScale should be greater than zero, and scaleMinFloat should not be allowed to go below kViewportMinScale.
Attachment #669480 - Flags: review-
Comment on attachment 669489 [details] [diff] [review] Accept the viewport that content has calculated, when it's received the latest widget geometry update Review of attachment 669489 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +1101,5 @@ > } > } > > mWaitingForContentToPaint = false; > + bool needContentRepaint = false; Shouldn't this default to true?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50) > Comment on attachment 669480 [details] [diff] [review] > nsContentUtils::GetViewportInfo() can return a defaultZoom of 0.0, which is > bad > > Review of attachment 669480 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems like we should fix this in GetViewportInfo()? > > Seems to me that kViewportMinScale should be greater than zero, and > scaleMinFloat should not be allowed to go below kViewportMinScale. Yes, jwir3 and I discussed this earlier. Should have obsoleted the patch.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #51) > Comment on attachment 669489 [details] [diff] [review] > Accept the viewport that content has calculated, when it's received the > latest widget geometry update > > Review of attachment 669489 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/ipc/AsyncPanZoomController.cpp > @@ +1101,5 @@ > > } > > } > > > > mWaitingForContentToPaint = false; > > + bool needContentRepaint = false; > > Shouldn't this default to true? Repainting is usually driven by the heartbeat in GetContentTransformForFrame(). This is a special case where the viewport changes in a way that requires us to force a repaint, even if we're not in the middle of an animation.
Attachment #669480 - Attachment is obsolete: true
Attachment #669820 - Flags: review?(sjohnson)
Attachment #669820 - Flags: feedback?(mbrubeck)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #49) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48) > > Comment on attachment 669479 [details] [diff] [review] > > Ignore updates of dimensions to w=0,h=0 > > > > Review of attachment 669479 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This looks like a workaround for some other bug. Can we look more closely > > into why these 0,0 sizes are being sent? > > I imagine you're right, but I was hoping you might be able to provide > guidance on where to look. Is it expected that we might see dimensions of > 0x0 during normal reflow? http://pastebin.mozilla.org/1862488 is the stack. I can't make much out of it, but we're in PresShell::FlushPendingNotifications() and it seems like we're doing frame-constructor-y stuff.
It looks like we may be reconstructing frames for the nsSubdocumentFrame, i.e. an IFRAME. Maybe we shouldn't be doing that ... hard to say without knowing why we're doing it. However, I think nsSubdocumentFrame::Init should not call ShowViewer via AsyncFrameInit if it successfully rescued the old presentation. That might help. Then again, if we're reconstructing frames I still don't know why the final size would ever be 0,0 for an IFRAME that should be visible. After reconstruction we should successfully reflow the frame.
Hi bz, We're seeing a frame reconstruction here that's causing some problems; roc suggested pinging you. What ends up happening is that we tell an <iframe remote>'s TabParent that its size is 0x0 from this stack http://pastebin.mozilla.org/1862488 and this causes rather a few problems because we're in the middle of repainting with various scales etc. The nsSubDocumentFrame that's being reconstructed is destroyed from this context http://pastebin.mozilla.org/1862505 There are various workarounds we can apply here, but roc says it's unexpected for this frame to be reconstructed. See comment 56. Do you have any thoughts / suggestions?
Chris says the frame destroy comes before the stack that shows the ShowViewer(), which is sorta as expected. I can look into why we reconstruct here, but chances are the site is just doing stuff that involves a reframe. Note that we're actually reconstructing something which has the subdocument as an abs pos kid of some descendant of itself. So the page is likely just changing some styles on the "something".
Attached patch Try to skip ShowViewer() (obsolete) — — Splinter Review
f? to see if I did this correctly. It doesn't work; we still end up in UpdateDimensions(0, 0) below ShowViewer().
Attachment #669852 - Flags: feedback?(roc)
Comment on attachment 669852 [details] [diff] [review] Try to skip ShowViewer() Review of attachment 669852 [details] [diff] [review]: ----------------------------------------------------------------- this looks right. Too bad it doesn't help
Attachment #669852 - Flags: feedback?(roc) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #58) > I can look into why we reconstruct here, but chances are the site is just > doing stuff that involves a reframe. Note that we're actually > reconstructing something which has the subdocument as an abs pos kid of some > descendant of itself. So the page is likely just changing some styles on > the "something". The subdocument is the browser's tab's document, so it's not Web content triggering the reframe, it's the browser app itself. We need to stop it doing that, if possible!
What kinds of things cause reframing? There's not that much code comprising the browser app, so I might be able to spot it quickly if I know approximately what I'm looking for.
> The subdocument is the browser's tab's document Oh! Well, in that case, for the reframing stack, what can we say about the aElement in ProcessOneRestyle? That's what's getting reframed here. > What kinds of things cause reframing? In this case, a CSS property changed on the aElement passed to ProcessOneRestyle in a way that triggered a frame reconstruct. There are a bunch of properties that can trigger that. You can search for NS_STYLE_HINT_FRAMECHANGE and nsChangeHint_ReconstructFrame in nsStyleStruct.cpp to see what all can cause it.
Chris and I looked into this. What's being reframed is <div id="main-screen">. It's being reframed because it dynamically picks up a transform when it normally doesn't have one. And it happens to have a child that's position:absolute, so FrameHasAbsPosPlaceholderDescendants returns true and we reframe on the transform change. So we can work around that by giving main-screen an identity transform by default. And/or making FrameHasAbsPosPlaceholderDescendants handle cases when the abspos thing in question already has the main-screen as its parent frame (in this case because it's position:relative all along).
A |translateY(0)| on our friend main-screen doesn't seem to work around the bug. But maybe we boil that away.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #66) > A |translateY(0)| on our friend main-screen doesn't seem to work around the > bug. But maybe we boil that away. Correction: it does work. Helps if I actually push the change to device. That's insurance against approval-aurora-, anyway.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65) > Created attachment 669865 [details] [diff] [review] > don't reframe for adding a transform when absolute descendants are present, > when the frame is already positioned This patch does the trick too. Additionally, this might be confirmation bias, but it seems to make the start of this UI transition a little quicker, although it doesn't improve the framerate.
Attachment #669479 - Attachment is obsolete: true
Attachment #669479 - Flags: review?(roc)
Comment on attachment 669489 [details] [diff] [review] Accept the viewport that content has calculated, when it's received the latest widget geometry update Did comment 51 make sense?
Fair number of reftest failures on try https://tbpl.mozilla.org/?tree=Try&rev=a8892429b618 Will try to see which patches are at fault.
Failures are caused by attachment 669820 [details] [diff] [review] . *scratches head*
Comment on attachment 669865 [details] [diff] [review] don't reframe for adding a transform when absolute descendants are present, when the frame is already positioned Please add static asserts that the two NS_STYLE_* constants are in the range [0,31], and r=me This will still be a bit too eager of the transformed thing has a rel pos descendant which has abs pos descendants, but that's probably ok.
Attachment #669865 - Flags: review?(bzbarsky) → review+
Comment on attachment 669820 [details] [diff] [review] Have defaultZoom default to 1.0 when there's no override > if (scaleStr.IsEmpty() && !widthStr.IsEmpty()) { > scaleFloat = NS_MAX(scaleFloat, float(aDisplayWidth) / float(width)); >+ } else if (scaleStr.IsEmpty()) { >+ scaleFloat = 1.0f; > } I think this will have undesired changes in user-facing behavior. Fennec's behavior (and the desired B2G behavior, I think?) when initial-scale is unspecified is to zoom the viewport to fit the screen width (e.g. scale = screen_width/viewport_width). For example, when you load a page with no meta viewport element, on a 320-pixel-wide device, the viewport width is set to the default 980px, and the initial scale should be 320/980 = 32.65%. This patch will change that to 100%. To get the Fennec behavior (and avoid defaulting defaultZoom to 0), I think it would be better to remove the "&& !widthStr.isEmpty()" from the if statement here.
Attachment #669820 - Flags: feedback?(mbrubeck) → feedback-
Comment on attachment 669820 [details] [diff] [review] Have defaultZoom default to 1.0 when there's no override Review of attachment 669820 [details] [diff] [review]: ----------------------------------------------------------------- Yes, I agree with mbrubeck here. I think we need to do something other than just return 1 if it's not specified. Perhaps we should restructure this function to return some type of error code if meta viewport isn't specified? It seems like we're confusing "meta viewport isn't specified" with "meta viewport is specified to be 0".
Attachment #669820 - Flags: review?(sjohnson) → review-
The "b2g behavior" is whatever this code returns :).
The font-inflation code relies on the computed defaultZoom to be less than 1.0 for the tests to work. That /should/ be the case in the reftest window ... except that the code checks the screen size, not the window size :(. So on my huge monitor we end up computing a *zoom-in*.
I would prefer to fix nsLayoutUtils::FontSizeInflationEnabled(), but that has risk for fennec. The font-inflation tests depending on the client screen is pretty bogus regardless, so I think I'll fix those too for insurance.
Attachment #669820 - Attachment is obsolete: true
Attachment #669852 - Attachment is obsolete: true
Attachment #670098 - Flags: review?(sjohnson)
Attachment #670098 - Flags: feedback?(mbrubeck)
We're not really sure why the screen size was checked here instead of the nearest widget.
Attachment #670100 - Flags: review?(sjohnson)
Attachment #670100 - Flags: feedback?(mbrubeck)
Now that GetViewportInfo() consistently returns a scale, the ambiguity is gone. This fixes the problem on http://www.quirksmode.org/m/tests/widthtest_vp380.html .
Attachment #670101 - Flags: review?(roc)
Attachment #670098 - Flags: feedback?(mbrubeck) → feedback+
Comment on attachment 670100 [details] [diff] [review] Ask the nearest widget for its bounds, instead of the screen Perhaps we should even do this inside of GetViewportInfo, and get rid of the aDisplayHeight/aDisplayWidth parameters?
Attachment #670100 - Flags: feedback?(mbrubeck) → feedback+
(In reply to Matt Brubeck (:mbrubeck) from comment #83) > Comment on attachment 670100 [details] [diff] [review] > Ask the nearest widget for its bounds, instead of the screen > > Perhaps we should even do this inside of GetViewportInfo, and get rid of the > aDisplayHeight/aDisplayWidth parameters? That would break all the tests.
Comment on attachment 669865 [details] [diff] [review] don't reframe for adding a transform when absolute descendants are present, when the frame is already positioned Review of attachment 669865 [details] [diff] [review]: ----------------------------------------------------------------- There was a bug in the patch, which caused a test failure --- the recursive call to FrameHasPositionedPlaceholderDescendants needs to be outside the f->GetType() == nsGkAtoms::placeholderFrame check. I fixed that.
Attachment #669865 - Flags: checkin+
Attachment #670100 - Flags: review?(sjohnson) → review+
Attachment #670098 - Flags: review?(sjohnson) → review+
Something re-broke android font-inflation tests. Sigh.
Comment on attachment 669865 [details] [diff] [review] don't reframe for adding a transform when absolute descendants are present, when the frame is already positioned Review of attachment 669865 [details] [diff] [review]: ----------------------------------------------------------------- Attachment 669865 [details] [diff] was backed out due to build bustage, which I'm quite sure was not related to this patch. So I'll reland it later.
Attachment #669865 - Flags: checkin+
These patches are in a pretty bad situation - attachment 670098 [details] [diff] [review] causes reftest failures on android, but the local harness is broken so I can't even attempt to repro locally. I suspect they would pass anyway, because I only have phones to test with, nothing with tegra resolution. - if the nearest widget is for the reftest window, it's 800px wide or so, so the results should be no different than on desktop - but if we're somehow getting a bogus widget size, we may be computing a zoom-in and disabling font inflation - if we hack attachment attachment 670100 [details] [diff] [review] to return the screen size ifdef MOZ_WIDGET_ANDROID, then we're likely to see the same too-large dimension and compute a zoom-out - if we hack attachment 670098 [details] [diff] [review] to return the defaultZoom 0.0 ifdef MOZ_WIDGET_ANDROID, then we'll fail the viewport mochitest everywhere - if we globally revert attachment 670098 [details] [diff] [review], then we'll need the TabChild hack again to avoid bogus computations I don't see a better way out of this than reverting attachment 670098 [details] [diff] [review] and attachment 670100 [details] [diff] [review] and going back to attachent 669480 :(.
I really hate to do this, but I don't see a way out of comment 90. On the plus side, this eliminates the two patches in this series that have any regression risk, so no approval-aurora needed. Carrying r=jwir3.
Attachment #670098 - Attachment is obsolete: true
Attachment #670100 - Attachment is obsolete: true
Attachment #670667 - Flags: review?(roc)
https://tbpl.mozilla.org/?tree=Try&rev=659d7211db83 Try run looks good, inbound is closed with no reopen criteria, going to do it the old-fashioned way https://hg.mozilla.org/mozilla-central/rev/1301a72b1c39
Attached patch Rollup — — Splinter Review
Change of plans.
Attachment #668702 - Attachment is obsolete: true
Attachment #669478 - Attachment is obsolete: true
Attachment #669481 - Attachment is obsolete: true
Attachment #669482 - Attachment is obsolete: true
Attachment #669483 - Attachment is obsolete: true
Attachment #669484 - Attachment is obsolete: true
Attachment #669486 - Attachment is obsolete: true
Attachment #669489 - Attachment is obsolete: true
Attachment #669490 - Attachment is obsolete: true
Attachment #670101 - Attachment is obsolete: true
Attachment #670667 - Attachment is obsolete: true
Attachment #668702 - Flags: feedback?(bugzilla)
It would be great to get it, if it's low risk! It's not 100% required because we landed a gaia workaround as insurance.
Priority: -- → P1
Then I probably should spend my Aurora-destabilization budget on other things :-).
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Target Milestone: --- → mozilla19
Depends on: 883568
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: