The various viewport-configuration "APIs" allow setting an initial zoom value. Currently, APZC doesn't honor this, AFAICT. Likely to be a compat problem.

In a bit more detail, the reason defaultZoom doesn't work is - the apzc for a TabChild uses the FrameMetrics it receives at the first layers txn as the "canonical configuration" - we compute the default zoom here http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#491 - but starting after DLBI, we're seeing a layers txn triggered before the "beforefirstpaint" notification here http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#213 That layers txn is "initializing" apzc with a FrameMetrics that does not match what we wanted. I suspect that the underlying problem here is leading to a lot of the current flakiness in browser rendering.

This patch doesn't do anything more than get the default zoom from TabChild to AsyncPanZoomController. This will let us implement a similar (but incorrect bug 807447) solution to supporting initial-scale as Firefox for Android.

Comment on attachment 677249 [details] [diff] [review] Part 1. Expose default zoom to AsyncPanZoomController This is not a "correct" approach. It's just going to cause other fuzziness during loading. If it's an overall improvement and we rule out the right fix as being too hard, I would consider taking this. Did comment 1 make sense?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3) > Comment on attachment 677249 [details] [diff] [review] > Part 1. Expose default zoom to AsyncPanZoomController > > This is not a "correct" approach. It's just going to cause other fuzziness > during loading. If it's an overall improvement and we rule out the right > fix as being too hard, I would consider taking this. > > Did comment 1 make sense? Not entirely. As far as I can tell, APZC has no knowledge of the default zoom and the fact that TabChild has adjusted the resolution. APZC just clobbers the adjusted resolution the next time that it sets it. Perhaps, you could outline how you envision this working?

Sure. For APZC, all information is communicated through FrameMetrics (except for the min/max/user-scalable which doesn't really affect rendering). The resolution-independent zoom is http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h#211 Hopefully the comment adequately describes the semantics of mZoom. FrameMetrics is also used to describe what *last painted*. This is how apzc computes content transforms. mZoom is approximately what APZC requests of content, and then mResolution is what it gets back on paints. So the basic flow looks like content paints, notifies mResolution --> apzc apzc changes mZoom, makes repaint request --> content The problem that arises with page navigation is that the mZoom flow of information is reversed for one transaction. Content has to tell apzc what the new initial zoom is for the document that just loaded. We ride on the presshell "firstpaint" state to do that. Specifically, we compute the viewport configuration (initial zoom etc.) for loaded content off of the beforefirstpaint event, and then set them up to sent to apzc in that layers txn. This worked just fine until dlbi. DLBI changed the timing of beforefirstpaint to happen extremely early in navigation, as far as I can tell right when the document navigation happens. At that point, we don't have the viewport configuration available, so we use fallback values. The fallback initial zoom is 1.0, which is why apzc never honors it. So the "real fix" in this setup is to restore the atomicity of "some" firstpaint notification with the first layers txn that's sent to apzc, which as you note clobbers its "target framemetrics", the metrics it uses to make requests to content. Your patch would allow us to implement initialscale, but at the cost of making the old document fuzzy while we're loading the new document. We would prematurely clobber the zoom value. Does that make sense?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > Does that make sense? Yes, enough to work with anyways.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > Does that make sense? Thinking some more, I don't think this is going to work. Using the preshell "firstpaint" state seems fragile and will not work for pages that set the default zoom after first paint. If you look at this test case in iOS: http://people.mozilla.com/~jmuizelaar/implementation-tests/zoom/viewport-default-zoom2-onload.html It will paint at the non-default zoom and then switches to the default zoom when the meta tag is added.

Does fennec handle that? I'm a lot less concerned about dynamic additions of meta viewport than implementing initialZoom on first paint correctly. What we want instead of the firstpaint hack is a "force override viewport configuration on next txn". But to do that correctly, we have to implement the current firstpaint hack correctly, which we don't.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8) > Does fennec handle that? I'm a lot less concerned about dynamic additions > of meta viewport than implementing initialZoom on first paint correctly. Yes, fennec does handle adding the viewport dynamically. The java code knows about the default zoom and will clamp to it. Fennec doesn't get this completely right as described in bug 807447. > What we want instead of the firstpaint hack is a "force override viewport > configuration on next txn". But to do that correctly, we have to implement > the current firstpaint hack correctly, which we don't. This is a little bit tricky because you don't want override all the time. You want to override only if userScalable=no or if userScalable=yes and the user hasn't already zoomed. It feels to me like the pan-zoom controller is in the best position to make this choice but I'm not certain.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #8) > > What we want instead of the firstpaint hack is a "force override viewport > > configuration on next txn". But to do that correctly, we have to implement > > the current firstpaint hack correctly, which we don't. > > This is a little bit tricky because you don't want override all the time. > You want to override only if userScalable=no or if userScalable=yes and the > user hasn't already zoomed. It feels to me like the pan-zoom controller is > in the best position to make this choice but I'm not certain. APZC is always free to decline the override request depending on user state. But the only way to get non-glitchy switches is for the request to happen in-band, through layers txns. (Well, without making things vastly more complicated anyway.)

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #9) > APZC is always free to decline the override request depending on user state. > But the only way to get non-glitchy switches is for the request to happen > in-band, through layers txns. (Well, without making things vastly more > complicated anyway.) Presumably we should also be communicating our zoom constraints though layer txns instead of Send/RecvUpdateZoomConstraints then too.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #9) > > APZC is always free to decline the override request depending on user state. > > But the only way to get non-glitchy switches is for the request to happen > > in-band, through layers txns. (Well, without making things vastly more > > complicated anyway.) > > Presumably we should also be communicating our zoom constraints though layer > txns instead of Send/RecvUpdateZoomConstraints then too. Also, just to confirm, APZC doesn't have anything like SetFirstPaintViewport does it?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #9) > > APZC is always free to decline the override request depending on user state. > > But the only way to get non-glitchy switches is for the request to happen > > in-band, through layers txns. (Well, without making things vastly more > > complicated anyway.) > > Presumably we should also be communicating our zoom constraints though layer > txns instead of Send/RecvUpdateZoomConstraints then too. Yes, but it doesn't make much difference in practice so I r+'d the current impl.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #12) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #9) > > > APZC is always free to decline the override request depending on user state. > > > But the only way to get non-glitchy switches is for the request to happen > > > in-band, through layers txns. (Well, without making things vastly more > > > complicated anyway.) > > > > Presumably we should also be communicating our zoom constraints though layer > > txns instead of Send/RecvUpdateZoomConstraints then too. > > Also, just to confirm, APZC doesn't have anything like SetFirstPaintViewport > does it? http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1130

Does this make sense? Should we be doing the stuff after ScheduleViewManagerFlush() if we're suppressing paints?

Comment on attachment 684217 [details] [diff] [review] Avoid turning on the refresh driver early Review of attachment 684217 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +4969,5 @@ > if (!pres || (pres->Document() && pres->Document()->GetDisplayDocument())) { > return; > } > + > + if (pres->PresShell()->ShouldIgnoreInvalidation()) This should be ! I think. You can just return if this is the case, no need to do the rest of the function. Also 'pres' the is the PresShell for the display root, that may not be what you want.

So we're getting a solid color layer paint early on because the refresh driver starts ticking and nothing stops it from painting. It sounds like pre-dlbi these paints wouldn't happen as often. So this patch is insufficient for a couple of reasons: 1. We haven't turned on paint suppression yet because SchedulePaint is called from an earlier part of PresShell::Initialize() before we turn on paint suppression. 2. If we turn on paint suppression earlier we still get into trouble because lots of other things want to start the refresh driver (AddStyleFlushObserver...) Perhaps we should check in the refresh driver if paints are suppressed only paint if they aren't. I'm on vacation tomorrow so I'll assign this to mattwoodrow in the meantime. He should have a better idea about how this is supposed to work anyways.

> Perhaps we should check in the refresh driver if paints are suppressed only > paint if they aren't. The problem with this is that the presshell we paint isn't usually the one that has suppression enabled. At least on desktop, the presshell/refresh driver are for the entire browser window, and we only suppress painting for the content document. I'm not sure how the presshell hierarchy is setup for b2g, but I suspect it will have a similar issue. As discussed with Jeff, I think it would be better to fire the first-paint notification for the presshell when we stop suppressing. Or possibly add a new notification for this event.

In content processes, the top-level presshell in any content-document tree should be exactly the one we care about before-first-paint on, if that makes a difference. As long as the new notification sets presshell::isfirstpaint and we throw away the old layer tree for the previous document prior to that notification (so that we don't have new framemetrics mixed with old document), that should work.

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

This delays setting first paint until we've unsuppressed paints. This seems to fix some of the symptoms of the problem but is insufficient to actually fix the problem. I'm not sure why yet.

It looks like our resolution is later getting clobbered in SetZoomAndResolution() as called by TabParent::UpdateDimensions(). It seems like when we get the frame metrics in NotifyLayersUpdated we have a resolution that's set to 2, but a zoom set to 1.

It seems like this is because RecordFrameMetrics() doesn't set the zoom but leaves it as 1

We should probably be setting the zoom there. Something like this would do it if (TabChild* tc = GetTabChildFrom(presShell)) { metrics.mZoom = tc->GetZoom(); }

This seems to do the trick.

Comment on attachment 691092 [details] [diff] [review] Delay first paint and propagate zoom >diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp >+// Include all the things \o/ >+#include "mozilla/dom/PBrowserChild.h" >+#include "mozilla/dom/TabChild.h" >+#include "mozilla/dom/ContentParent.h" >+#include "mozilla/dom/ContentChild.h" Que? > using namespace mozilla::layers; >+using namespace mozilla::dom; >+using mozilla::dom::TabChild; The explicit |using TabChild| is superfluous with above. >diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp >- if (mIsFirstPaint) { >+ if (mIsFirstPaint && !mPaintingSuppressed) { > layerManager->SetIsFirstPaint(); > mIsFirstPaint = false; > } This looks correct to me, but this code is shared with fennec-android and TBH I don't know what effect this might have. f? kats. Otherwise looks good.

Comment on attachment 691092 [details] [diff] [review] Delay first paint and propagate zoom Review of attachment 691092 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, but I will build with the nsPresShell change and test a few things to make sure it doesn't introduce fennec regressions. Also maybe it'll fix bug 799530! (fingers crossed)

My testing didn't turn up any problems (also it didn't fix bug 799530).

Comment on attachment 691092 [details] [diff] [review] Delay first paint and propagate zoom r=me with the nits in comment 27 addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0d968e33b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/380930ddb930

https://hg.mozilla.org/mozilla-central/rev/380930ddb930 https://hg.mozilla.org/mozilla-central/rev/9d0d968e33b4

https://hg.mozilla.org/releases/mozilla-aurora/rev/c999276c8c18 https://hg.mozilla.org/releases/mozilla-aurora/rev/4dba9c208e9a https://hg.mozilla.org/releases/mozilla-b2g18/rev/84c5c58831a5 https://hg.mozilla.org/releases/mozilla-b2g18/rev/3be06a1bb239