Closed Bug 1178847 Opened 9 years ago Closed 9 years ago

Refactor code that computes the CSS viewport

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files, 7 obsolete files)

Part 1 - Extract a helper
5.21 KB, patch
botond
: review+
Details | Diff | Splinter Review
Part 3 - Don't use the new code on Mulet or Fennec yet (v2)
2.27 KB, patch
kats
: review+
Details | Diff | Splinter Review
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v5)
53.19 KB, patch
botond
: review+
tnikkel
: review+
Details | Diff | Splinter Review 
We have various bits of code that compute the CSS viewport and sets it via DOMWindowUtils (for Fennec) or nsLayoutUtils::SetCSSViewport (for B2G). I would like to unify this code and hang it directly off nsPresShell so that:
1) we can reuse it across platforms, particularly for the fennec-apz work (bug 776030)
2) make it easier to track down and fix bugs (see bug 1126346 and friends)
3) avoid doing unnecessary reflows on background tabs (see bug 1158640, bug 1178591)
Attached patch Sketch (obsolete) — — Splinter Review 
This is the basic idea of the change I would like to do. Instead of having random bits of code all over the place computing and setting the CSS viewport, we can just have the presShell do it as part of its regular resize/reflow code. The MVM additionally listens for DOMMetaAdded events and should trigger a reflow if the CSS viewport changes. This should in theory allow us to rip out a lot of code from TabChild.cpp and Fennec's browser.js.
This is going to be so nice.
Attached patch Delete a bunch of stuff (obsolete) — — Splinter Review 
/me looks at the carnage and giggles

(Some of this code will probably need to be moved into MobileViewportManager, but we don't need any of this for desktop at least).
Comment on attachment 8627907 [details] [diff] [review]
Delete a bunch of stuff

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

::: dom/ipc/TabChild.cpp
@@ -456,5 @@
> -  metrics.SetScrollableRect(CSSRect(CSSPoint(), pageSize));
> -
> -  // Calculate a display port _after_ having a scrollable rect because the
> -  // display port is clamped to the scrollable rect.
> -  metrics.SetDisplayPortMargins(APZCTreeManager::CalculatePendingDisplayPort(

This code is currently reponsible for setting the initial displayport on page load. What will do that if this is removed?
Comment on attachment 8627907 [details] [diff] [review]
Delete a bunch of stuff

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

::: dom/ipc/TabChild.cpp
@@ +668,5 @@
>          if (shell) {
>            shell->SetIsFirstPaint(true);
>          }
>  
> +        APZCCallbackHelper::InitializeRootDisplayport(shell);

This call to InitializeRootDisplayport does it. (I have another patch that moves ChromeProcessController::Init to APZCCallbackHelper::InitializeRootDisplayport for reuse).
I did a try push with my WIPs so far at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aca07be1f96 - this was intended to be working on desktop and B2G and it was, mostly. One test failure which is easy to fix. Fennec is a mass of orange though, mostly because there's a scrollbar appearing in the test cases and not the reference cases. No idea what that's about - I was expecting it to fail on fennec but not this way. Anyway I cleaned up the patches to see if I can get the B2G/desktop code landed with the Fennec stuff as-is for now, and do that work in a follow-up bug. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ec48fe26e68 is the new try push.
Ah, the scrollbar is appearing on SVG files in the Fennec reftests. Probably we have some special handling for that somewhere that I need to replicate.
Attachment #8629475 - Attachment is patch: true
Mulet prefs are wacky because of bug 1174234.
Attachment #8629478 - Flags: review?(botond)
Comment on attachment 8629477 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager

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

I don't understand this patch enough to comfortably r+ it.

::: dom/ipc/TabChild.cpp
@@ +1640,3 @@
>      SetUnscaledInnerSize(size);
> +    mHasValidInnerSize = !mHasValidInnerSize
> +                      && (size.width != 0 && size.height != 0);

I don't understand this condition. If it was valid before, and the size isn't empty, then it's invalid now? And if it was invalid before, and the size isn't empty, then it's valid now? Why?

::: layout/base/MobileViewportManager.cpp
@@ +114,5 @@
> +    MVM_LOG("Get viewport hasn't been called in %p before, plucking display size from presShell\n", this);
> +    mLastDisplaySize = mPresShell->GetPresContext()->GetVisibleArea().Size();
> +  }
> +
> +  // XXX Ideally this would not call ResizeReflow directly but do something

How did this work before?

@@ +119,5 @@
> +  // like nsViewManager::SetWindowDimensions where it is potentially queued up
> +  // until the presShell is visible. However that doesn't work for us because
> +  // DoSetWindowDimensions throws away the info if the window dimensions are the
> +  // same as they were before (which is what we want here). Maybe we should just
> +  // add a flag to bypass that short-circuit?

Why do we need a reflow if the size hasn't changed? Can this reason be expressed in a more explicit way?

@@ +120,5 @@
> +  // until the presShell is visible. However that doesn't work for us because
> +  // DoSetWindowDimensions throws away the info if the window dimensions are the
> +  // same as they were before (which is what we want here). Maybe we should just
> +  // add a flag to bypass that short-circuit?
> +  mPresShell->ResizeReflow(mLastDisplaySize.width, mLastDisplaySize.height);

So we call ResizeReflow with mLastDisplaySize, which we got from the previous call to GetViewport (right?), and ResizeReflow will trigger another call to GetViewport with that same size. Does that mean that GetViewport does the same work twice? Is that bad?

@@ +136,5 @@
> +
> +  mLastDisplaySize = aDisplaySize;
> +
> +  // TODO: is this correct? Do we need to handle UNCONSTRAINED_SIZE for either
> +  // aDisplaySize.width or aDisplaySize.height?

I don't know. Maybe dholbert or mats would be a better reviewer for the reflow parts.
Attachment #8629477 - Flags: review?(mstange)
(In reply to Markus Stange [:mstange] from comment #12)
> 
> I don't understand this patch enough to comfortably r+ it.

Ok, thanks for looking over it anyway :)

> >      SetUnscaledInnerSize(size);
> > +    mHasValidInnerSize = !mHasValidInnerSize
> > +                      && (size.width != 0 && size.height != 0);
> 
> I don't understand this condition. If it was valid before, and the size
> isn't empty, then it's invalid now? And if it was invalid before, and the
> size isn't empty, then it's valid now? Why?

Oh, that looks like a typo or something. I meant to keep the same semantics as the previous code with respect to setting mHasValidInnerSize but I think I screwed up the refactoring. Good catch, thanks!

> ::: layout/base/MobileViewportManager.cpp
> @@ +114,5 @@
> > +    MVM_LOG("Get viewport hasn't been called in %p before, plucking display size from presShell\n", this);
> > +    mLastDisplaySize = mPresShell->GetPresContext()->GetVisibleArea().Size();
> > +  }
> > +
> > +  // XXX Ideally this would not call ResizeReflow directly but do something
> 
> How did this work before?

Previously we would call nsLayoutUtils::SetCSSViewport on any of the four triggers (DOMMetaAdded, before-first-paint, FullZoomChanged, widget resize) which trickled down to PresShell::ResizeReflowOverride. This would happen even if the presShell was not visible, resulting in things like bug 1158640. With the new code only three of the four triggers (DOMMetaAdded, before-first-paint and FullZoomChanged) will trigger a call to ResizeReflow while the presShell is not visible. In particular, it does *not* happen if the widget is resized; for that case it happens when the presShell is eventually made visible. Therefore this is strictly better than the old code, but can be improved further (which is what the comment refers to).

> @@ +119,5 @@
> > +  // like nsViewManager::SetWindowDimensions where it is potentially queued up
> > +  // until the presShell is visible. However that doesn't work for us because
> > +  // DoSetWindowDimensions throws away the info if the window dimensions are the
> > +  // same as they were before (which is what we want here). Maybe we should just
> > +  // add a flag to bypass that short-circuit?
> 
> Why do we need a reflow if the size hasn't changed? Can this reason be
> expressed in a more explicit way?

We need a reflow if the CSS viewport size changes. Any of the four triggers mentioned above could result in a CSS viewport size change (and in fact all of those triggers will do so with high probability). If the CSS viewport size doesn't change we don't need a reflow, but we don't find that out until later, when the GetViewport function is called. Given that these triggers *not* resulting in a CSS viewport change is pretty unlikely I figured this code wasn't worth optimizing further yet. But I can update the comment to explain this a little better.

> @@ +120,5 @@
> > +  // until the presShell is visible. However that doesn't work for us because
> > +  // DoSetWindowDimensions throws away the info if the window dimensions are the
> > +  // same as they were before (which is what we want here). Maybe we should just
> > +  // add a flag to bypass that short-circuit?
> > +  mPresShell->ResizeReflow(mLastDisplaySize.width, mLastDisplaySize.height);
> 
> So we call ResizeReflow with mLastDisplaySize, which we got from the
> previous call to GetViewport (right?), and ResizeReflow will trigger another
> call to GetViewport with that same size. Does that mean that GetViewport
> does the same work twice? Is that bad?

Not exactly. For example in the case of DOMMetaAdded there is a new meta viewport tag which obsoletes the previous one, so nsContentUtils::GetViewportInfo will return a different result for the same "display size" that was passed in. So the work done, and the CSS viewport that gets calculated, is different. FullZoomChanged similarly affects some of the scale values and so the resulting CSS viewport will be different.

> > +  // TODO: is this correct? Do we need to handle UNCONSTRAINED_SIZE for either
> > +  // aDisplaySize.width or aDisplaySize.height?
> 
> I don't know. Maybe dholbert or mats would be a better reviewer for the
> reflow parts.

Ok, thanks.
No-jun, would you be able to find somebody to apply these patches and do some manual testing in B2G? They're a little on the risky side so getting some more manual testing would be nice. The types of bugs that are most likely are things not rendering at the right size either on initial load, on rotation, or when the keyboard appears/disappears.
Flags: needinfo?(npark)
Comment on attachment 8629477 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager

r? to tn for layout side review after discussion on IRC.
Attachment #8629477 - Flags: review?(tnikkel)
Attachment #8627803 - Attachment is obsolete: true
Adding qawanted to test this locally.
Keywords: qawanted
QA Contact: pcheng
I tested it on local build, didn't find any issues.  Awaiting the result from qanalysts.
Flags: needinfo?(npark)
I tested browser for a while at various sites and saw no issues on the build provided.

Environmental Variables:
Device: Flame 2.5
BuildID: 20150705175829
Gaia: dc6c18c0dea7af3c40bfff86c530fd877d899dc4
Gecko: Unknown
Gonk: a4f6f31d1fe213ac935ca8ede7d05e47324101a4
Version: 42.0a1 (2.5) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Contact: pcheng
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Attachment #8629475 - Flags: review?(botond) → review+
Comment on attachment 8629477 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager

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

General comment: 

  I find the way MobileViewportManager and nsPresShell call into each other a bit unintuitive.

  Here's how I view this conceptually:

    - We have a quantity, the viewport size, which is a function of certain other quantities
      (the display size, the meta-viewport tag / other things that affect GetViewportInfo, 
      the full zoom).

    - Whenever any of these quantities change, we want to recompute the viewport size.

    - Whenever the viewport size changes, we want to do certain things (set resolution, 
      set SPC-SPS, the actual reflow).

  I'm wondering if we could structure things to as to reflect this conceptual model more
  closely. Some concrete(ish) suggestions:

    - Store the display size somewhere where it's queryable, such as nsPresShell.

    - Have whatever changes the display size (ResizeReflow) update that variable,
      and then call the same "a dependency of the viewport size has changed, let's
      recompute the viewport size" function that gets called when one of the other
      dependencies (like the full zoom) changes. That function can query the display
      size from where it's stored, rather than doing the "either it's passed in,
      or we consult a cached value" dance.

    - Have the actual reflow be performed by "let's recompute the viewport size"
      function alongisde the SetResolution and SetSPCSPS calls.

  Thoughts?

Otherwise this looks pretty good, and I like the code simplification/removal being done! The r- is mostly for not excluding the scrollbar areas from the SPC-SPS, but I'd like to see a discussion of the other points as well.

::: dom/ipc/TabChild.cpp
@@ -329,5 @@
> -  CSSSize viewport(viewportInfo.GetSize());
> -
> -  // We're not being displayed in any way; don't bother doing anything because
> -  // that will just confuse future adjustments.
> -  if (!screenW || !screenH) {

Should MVM::GetViewport() likewise be bailing if either dimension of the display size is zero?

@@ -353,5 @@
> -  // point on the DOMMetaAdded handler is so that scripts that use
> -  // window.innerWidth before they are painted have a correct value (bug
> -  // 771575).
> -  if (!mContentDocumentIsDisplayed) {
> -    return false;

This early-exit path is not present in MobileViewportManager::GetViewport(). Is this OK because it doesn't do anything that forces a reflow?

@@ -466,5 @@
> -  metrics.SetUseDisplayPortMargins();
> -
> -  // Force a repaint with these metrics. This, among other things, sets the
> -  // displayport, so we start with async painting.
> -  mLastRootMetrics = ProcessUpdateFrame(metrics);

ProcessUpdateFrame does five things:

  (1) sends the Viewport:Change message
  (2) sets the SPC-SPS
  (3) sets the resolution
  (4) calls ScrollFrame()
  (5) sets the displayport margins, freshly computed above
      by APZCTreeManager::CalculatePendingDisplayPort()

So currently, all of these were done on the various triggers for a viewport change.

MobileViewportManager only does (2) and (3). Let's go through the other ones:

  (1) is only used by BEP.js in non-APZ B2G setups.
      We probably don't care much about those, but
      we should at least be conscious of breaking it.

  (4) is probably OK not to do, as the scroll position
      shouldn't change when the viewport changes

  (5) not doing this worries me, because it means that 
      we will wait until the next APZ repaint to set a
      displayport that reflects the new composition bounds
      Should we be doing this in MVM::GetViewport()?

@@ +1640,3 @@
>      SetUnscaledInnerSize(size);
> +    mHasValidInnerSize = !mHasValidInnerSize
> +                      && (size.width != 0 && size.height != 0);

I think what you want is:

  if (!mHasValidInnerSize && (size.width != 0 && size.height != 0)) {
    mHasValidInnerSize = true;
  }

We certainly don't want to be setting it back to false :)

::: layout/base/MobileViewportManager.cpp
@@ +100,5 @@
> +MobileViewportManager::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData)
> +{
> +  if (SameCOMIdentity(aSubject, mDocument) && BEFORE_FIRST_PAINT.EqualsASCII(aTopic)) {
> +    MVM_LOG("Got a before-first-paint event in %p\n", this);
> +    mIsFirstPaint = true;

Since the constructor initializes mIsFirstPaint to false, does this mean that we can have a viewport computation happen with mIsFirstPaint = false before we get here?

@@ +111,5 @@
> +MobileViewportManager::RefreshViewportSize()
> +{
> +  if (mLastDisplaySize == nsSize()) {
> +    MVM_LOG("Get viewport hasn't been called in %p before, plucking display size from presShell\n", this);
> +    mLastDisplaySize = mPresShell->GetPresContext()->GetVisibleArea().Size();

I'm going to defer to Timothy on this being the right thing to query.

@@ +118,5 @@
> +  // XXX Ideally this would not call ResizeReflow directly but do something
> +  // like nsViewManager::SetWindowDimensions where it is potentially queued up
> +  // until the presShell is visible. However that doesn't work for us because
> +  // DoSetWindowDimensions throws away the info if the window dimensions are the
> +  // same as they were before (which is what we want here). Maybe we should just

Did you mean to say "(which is *not* what we want here)"?

@@ +124,5 @@
> +  mPresShell->ResizeReflow(mLastDisplaySize.width, mLastDisplaySize.height);
> +}
> +
> +CSSSize
> +MobileViewportManager::GetViewport(const nsSize& aDisplaySize)

This method does a lot of things (SetResolutionAndScaleTo, SetScrollPositionClampingScrollPortSize) for a method named GetXXX(). Can we call it something like UpdateViewport[Size]()?

@@ +164,5 @@
> +    }
> +    MOZ_ASSERT(viewportInfo.GetMinZoom() <= defaultZoom &&
> +      defaultZoom <= viewportInfo.GetMaxZoom());
> +
> +    CSSToParentLayerScale zoom = ViewTargetAs<ParentLayerPixel>(defaultZoom,

This seems to be a change in behaviour from the TabChild code, which would only use nsViewportInfo::GetDefaultZoom() as the zoom on first paint. For other viewport size changes, it would "maintain how much actual content is visible within the screen width" by multiplying the current zoom by the ratio of the old and new intrinsic scales [1]. Why are we changing this?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=e17e313a3066#399

@@ +174,5 @@
> +    nsLayoutUtils::SetResolutionAndScaleTo(mPresShell, resolution.scale);
> +
> +    CSSSize compSize = ScreenSize(displaySize) / defaultZoom;
> +    MVM_LOG("Setting SPCSPS %s\n", Stringify(compSize).c_str());
> +    nsLayoutUtils::SetScrollPositionClampingScrollPortSize(mPresShell, compSize);

This is incorrect - we need to exclude the scrollbar areas from the display size before treating as the composition bounds (see bug 1164406).

::: layout/base/MobileViewportManager.h
@@ +28,5 @@
> +private:
> +  ~MobileViewportManager();
> +
> +public:
> +  void Init(nsIPresShell* aPresShell, nsIDocument *aDocument);

nit: nsIDocument*

::: layout/base/nsPresShell.cpp
@@ +1769,5 @@
>    if (mViewportOverridden) {
>      // The viewport has been overridden, and this reflow request
>      // didn't ask to ignore the override.  Pretend it didn't happen.
>      return NS_OK;
> +  } else if (mMobileViewportManager) {

It seems to me that in scenarios where

  - ResizeReflowOverride had been called (via SetCSSViewport) in the past, and
  - ResizeReflow is called by one of the existing callers

no reflow would be performed prior to this patch (because we would early-exit due to mViewportOverridden being true above), but one will be performed with this patch (because instead we get into the mMobileViewportManager case). Is that right (and if so, is that intentional/ok?), or am I missing something?
Attachment #8629477 - Flags: review?(botond) → review-
Comment on attachment 8629478 [details] [diff] [review]
Part 3 - Don't use the new code on Mulet or Fennec yet

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

::: b2g/app/b2g.js
@@ +458,5 @@
>  pref("dom.ipc.processCount", 100000);
>  
>  pref("dom.ipc.browser_frames.oop_by_default", false);
>  
> +#ifndef MOZ_MULET

Please reference bug 1174234.
Attachment #8629478 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #19)
>   I'm wondering if we could structure things to as to reflect this
> conceptual model more
>   closely. Some concrete(ish) suggestions:

You raise some good points. I restructured the patch a little bit so that it's a bit clearer to follow. In particular all of the dependency changes now directly call RefreshViewportSize(), which then passes the new CSS viewport size (if it's changed) to ResizeReflowIgnoreOverride. The display size is stored in the MVM similar to the mIsFirstPaint flag, and any changes to it are pushed over by the presShell. Let me know what you think.

> ::: dom/ipc/TabChild.cpp
> 
> Should MVM::GetViewport() likewise be bailing if either dimension of the
> display size is zero?

I'm not sure if we ever encounter only one dimension being zero in practice, but sure, I updated the guard to check either dimension independently.

> @@ -353,5 @@
> > -  if (!mContentDocumentIsDisplayed) {
> > -    return false;
> 
> This early-exit path is not present in MobileViewportManager::GetViewport().
> Is this OK because it doesn't do anything that forces a reflow?

In a manner of speaking - in that version of the patch MVM::GetViewport only gets called as part of a reflow to begin with, so we don't really need to avoid doing a reflow in it. In the new version of the patch that I'm about to post there's a similar structure in that we're going to be doing a reflow anyway, intentionally, so we don't need that guard.

> ProcessUpdateFrame does five things:
> 
>   (1) sends the Viewport:Change message
>   (2) sets the SPC-SPS
>   (3) sets the resolution
>   (4) calls ScrollFrame()
>   (5) sets the displayport margins, freshly computed above
>       by APZCTreeManager::CalculatePendingDisplayPort()
> 
> So currently, all of these were done on the various triggers for a viewport
> change.
> 
> MobileViewportManager only does (2) and (3). Let's go through the other ones:
> 
>   (1) is only used by BEP.js in non-APZ B2G setups.
>       We probably don't care much about those, but
>       we should at least be conscious of breaking it.
> 
>   (4) is probably OK not to do, as the scroll position
>       shouldn't change when the viewport changes
> 
>   (5) not doing this worries me, because it means that 
>       we will wait until the next APZ repaint to set a
>       displayport that reflects the new composition bounds
>       Should we be doing this in MVM::GetViewport()?

Hm, I think (1) is used in APZ B2G setups as well, for double-tap zooming. I think that will be slightly broken with my patch (in that it may use stale values if you double-tap right after the viewport changes but before the APZ does a repaint). I'm not too worried about this because it's an edge case and once we move the double-tap code to C++ it will have access to up-to-date values from the FrameMetrics and we'll drop the Viewport:Change message entirely.

Agreed on (4) and (5). I added a call to refresh the displayport margins to address (5).

> I think what you want is:
> 
>   if (!mHasValidInnerSize && (size.width != 0 && size.height != 0)) {
>     mHasValidInnerSize = true;
>   }
> 
> We certainly don't want to be setting it back to false :)

Yup, updated.

> ::: layout/base/MobileViewportManager.cpp
> @@ +100,5 @@
> > +MobileViewportManager::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData)
> > +{
> > +  if (SameCOMIdentity(aSubject, mDocument) && BEFORE_FIRST_PAINT.EqualsASCII(aTopic)) {
> > +    MVM_LOG("Got a before-first-paint event in %p\n", this);
> > +    mIsFirstPaint = true;
> 
> Since the constructor initializes mIsFirstPaint to false, does this mean
> that we can have a viewport computation happen with mIsFirstPaint = false
> before we get here?

Yes, that can happen. For example if there is a meta-viewport tag that gets processed before the first paint (which is actually quite often) then we get a viewport computation while mIsFirstPaint=false.

> @@ +111,5 @@
> > +    mLastDisplaySize = mPresShell->GetPresContext()->GetVisibleArea().Size();
> 
> I'm going to defer to Timothy on this being the right thing to query.

Note that in the new patch this is passed in to the MVM constructor. However while testing it seems like it's pretty much always zero so I might be able to skip assigning it entirely. I definitely remember running into problematic cases when I wrote the first version of the patch where it wasn't getting set and stuff wasn't laying out properly.

> @@ +118,5 @@
> > +  // XXX Ideally this would not call ResizeReflow directly but do something
> > +  // like nsViewManager::SetWindowDimensions where it is potentially queued up
> > +  // until the presShell is visible. However that doesn't work for us because
> > +  // DoSetWindowDimensions throws away the info if the window dimensions are the
> > +  // same as they were before (which is what we want here). Maybe we should just
> 
> Did you mean to say "(which is *not* what we want here)"?

No, I meant that we do want to pass in the same window dimensions as before. However after thinking about this some more I think this comment may not be correct. In cases where the meta-viewport tag changes while the tab is in the background (for example) we do still want to update the CSS viewport while in the background because scripts might then try to read layout properties like innerWidth and expect them to have changed. There might be some cases where we can avoid this but I removed the comment since in general the comment is wrong.

> > +MobileViewportManager::GetViewport(const nsSize& aDisplaySize)
> 
> This method does a lot of things (SetResolutionAndScaleTo,
> SetScrollPositionClampingScrollPortSize) for a method named GetXXX(). Can we
> call it something like UpdateViewport[Size]()?

All this stuff is now part of RefreshViewportSize (and actually I split them out into helper functions).

> > +    CSSToParentLayerScale zoom = ViewTargetAs<ParentLayerPixel>(defaultZoom,
> 
> This seems to be a change in behaviour from the TabChild code, which would
> only use nsViewportInfo::GetDefaultZoom() as the zoom on first paint. For
> other viewport size changes, it would "maintain how much actual content is
> visible within the screen width" by multiplying the current zoom by the
> ratio of the old and new intrinsic scales [1]. Why are we changing this?

It took me a long time to actually find a scenario where this matters (turns out it wasn't that hard, the grid page without a meta-viewport tag demonstrates it - I'm just bad at testing). But you're right, this was broken. The updated patch fixes this as well.

> @@ +174,5 @@
> > +    nsLayoutUtils::SetScrollPositionClampingScrollPortSize(mPresShell, compSize);
> 
> This is incorrect - we need to exclude the scrollbar areas from the display
> size before treating as the composition bounds (see bug 1164406).

Updated. (For posterity - Botond discovered that the fix in bug 1164406 was broken by a bad rebase in bug 1125325).

> > +public:
> > +  void Init(nsIPresShell* aPresShell, nsIDocument *aDocument);
> 
> nit: nsIDocument*

Fixed, but I also merged the Init function into the constructor since there was no point in having a separate one.

> ::: layout/base/nsPresShell.cpp
> @@ +1769,5 @@
> >    if (mViewportOverridden) {
> >      // The viewport has been overridden, and this reflow request
> >      // didn't ask to ignore the override.  Pretend it didn't happen.
> >      return NS_OK;
> > +  } else if (mMobileViewportManager) {
> 
> It seems to me that in scenarios where
> 
>   - ResizeReflowOverride had been called (via SetCSSViewport) in the past,
> and
>   - ResizeReflow is called by one of the existing callers
> 
> no reflow would be performed prior to this patch (because we would
> early-exit due to mViewportOverridden being true above), but one will be
> performed with this patch (because instead we get into the
> mMobileViewportManager case). Is that right (and if so, is that
> intentional/ok?), or am I missing something?

I'm not sure I understood this. In the patch the mViewportOverridden flag still takes priority over mMobileViewportManager. So if something called SetCSSViewport, the flag would get set and we would return NS_OK right away, never consulting the MVM. The updated patch maintains this invariant but also takes it a step further by destroying the MVM entirely once the mViewportOverridden flag is set.

        Attachment #8629477 -
        Attachment is obsolete: true

        Attachment #8629477 -
        Flags: review?(tnikkel)

        Attachment #8631787 -
        Flags: review?(tnikkel)

        Attachment #8631787 -
        Flags: review?(botond)

    
    

    
  


  
Added a comment, carrying r+

        Attachment #8629478 -
        Attachment is obsolete: true

        Attachment #8631788 -
        Flags: review+

    
    

    
  
The above try push has some failures, and I suspect it's the change to how I initialize mDisplaySize that's the cause.

I did another try push [1] with a partial revert of that and it fixed some of the failures; I have another one in progress [2] with a second partial revert to see if it fixes the rest of them. If so, then I'll just move the mDisplaySize initialization back to the way it was, which is a pretty simple update to the patch.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba4189b3c41
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b33311034751

    
    

    
  
Comment on attachment 8631787 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v2)

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

Thanks! Just a few minor comments remaining.

Also, let's be sure we get Timothy's input on:
  - handling NS_UNCONSTRAINEDSIZE
  - how to initialize mDisplaySize
before landing.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> (In reply to Botond Ballo [:botond] from comment #19)
> >   I'm wondering if we could structure things to as to reflect this
> > conceptual model more
> >   closely. Some concrete(ish) suggestions:
> 
> You raise some good points. I restructured the patch a little bit so that
> it's a bit clearer to follow. In particular all of the dependency changes
> now directly call RefreshViewportSize(), which then passes the new CSS
> viewport size (if it's changed) to ResizeReflowIgnoreOverride. The display
> size is stored in the MVM similar to the mIsFirstPaint flag, and any changes
> to it are pushed over by the presShell. Let me know what you think.

Definitely nicer, thanks!

> > ProcessUpdateFrame does five things:
> > 
> >   (1) sends the Viewport:Change message
> >   (2) sets the SPC-SPS
> >   (3) sets the resolution
> >   (4) calls ScrollFrame()
> >   (5) sets the displayport margins, freshly computed above
> >       by APZCTreeManager::CalculatePendingDisplayPort()
> > 
> > So currently, all of these were done on the various triggers for a viewport
> > change.
> > 
> > MobileViewportManager only does (2) and (3). Let's go through the other ones:
> > 
> >   (1) is only used by BEP.js in non-APZ B2G setups.
> >       We probably don't care much about those, but
> >       we should at least be conscious of breaking it.
> > 
> >   (4) is probably OK not to do, as the scroll position
> >       shouldn't change when the viewport changes
> > 
> >   (5) not doing this worries me, because it means that 
> >       we will wait until the next APZ repaint to set a
> >       displayport that reflects the new composition bounds
> >       Should we be doing this in MVM::GetViewport()?
> 
> Hm, I think (1) is used in APZ B2G setups as well, for double-tap zooming. I
> think that will be slightly broken with my patch (in that it may use stale
> values if you double-tap right after the viewport changes but before the APZ
> does a repaint). 

Ah, good point, I overlooked that!

> I'm not too worried about this because it's an edge case
> and once we move the double-tap code to C++ it will have access to
> up-to-date values from the FrameMetrics and we'll drop the Viewport:Change
> message entirely.

Sounds good. (We should probably make sure we don't ship a B2G release with the broken behaviour.)

> > ::: layout/base/MobileViewportManager.cpp
> > @@ +100,5 @@
> > > +MobileViewportManager::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData)
> > > +{
> > > +  if (SameCOMIdentity(aSubject, mDocument) && BEFORE_FIRST_PAINT.EqualsASCII(aTopic)) {
> > > +    MVM_LOG("Got a before-first-paint event in %p\n", this);
> > > +    mIsFirstPaint = true;
> > 
> > Since the constructor initializes mIsFirstPaint to false, does this mean
> > that we can have a viewport computation happen with mIsFirstPaint = false
> > before we get here?
> 
> Yes, that can happen. For example if there is a meta-viewport tag that gets
> processed before the first paint (which is actually quite often) then we get
> a viewport computation while mIsFirstPaint=false.

I guess I was asking if that would be a problem, but I can't think of any reason why it would be.

> > ::: layout/base/nsPresShell.cpp
> > It seems to me that in scenarios where
> > 
> >   - ResizeReflowOverride had been called (via SetCSSViewport) in the past, and
> >   - ResizeReflow is called by one of the existing callers
> > 
> > no reflow would be performed prior to this patch (because we would
> > early-exit due to mViewportOverridden being true above), but one will be
> > performed with this patch (because instead we get into the
> > mMobileViewportManager case). Is that right (and if so, is that
> > intentional/ok?), or am I missing something?
> 
> I'm not sure I understood this. In the patch the mViewportOverridden flag
> still takes priority over mMobileViewportManager. So if something called
> SetCSSViewport, the flag would get set and we would return NS_OK right away,
> never consulting the MVM. The updated patch maintains this invariant but
> also takes it a step further by destroying the MVM entirely once the
> mViewportOverridden flag is set.

I was talking about cases like this:
  1) The viewport changes (via any of the triggers).
  2) Later, something calls ResizeReflow().

Before, (1) would call ResizeReflowOverride (via SetCSSViewport) and set mViewportOverridden, and then (2) wouldn't cause a reflow because ResizeReflow() would early-exit.

With your original patch, (1) and (2) would both call ResizeReflow(), and since no one would set mViewportOverridden, (2) would do a reflow as well.

With your current patch, if there's an MVM then ResizeReflow() only actually does a reflow if the CSS viewport changes, so I think this concern is addressed. (Please let me know if it made sense though.)

::: layout/base/MobileViewportManager.cpp
@@ +34,5 @@
> +  MVM_LOG("Creating %p with presShell %p and display size %d,%d\n",
> +    this, mPresShell, mDisplaySize.width, mDisplaySize.height);
> +
> +  if (!mDocument) {
> +    return;

mDocument is used by nsContentUtils::GetViewportInfo() which requires it to be non-null. We should either assert mDocument here, or null-check it before the GetViewportInfo() call.

@@ +161,5 @@
> +  float cssViewportChangeRatio = (mMobileViewportSize.width == 0)
> +     ? 1.0f : aViewport.width / mMobileViewportSize.width;
> +  LayoutDeviceToLayerScale oldRes(nsLayoutUtils::GetResolution(mPresShell));
> +  LayoutDeviceToLayerScale newRes(oldRes.scale * aDisplayWidthChangeRatio
> +    / cssViewportChangeRatio);

Sorry to nitpick on this, but I still think we're doing something different from before: we're using width ratios, while before we used the max of width and height ratios.

I think this may change behaviour in cases where the viewport's aspect ratio doesn't track the display's aspect ratio (for example, if the meta-viewport tag hardcodes both dimensions of the viewport). Arguably an edge case, but is there a motivation for not using the same calculation as before?

@@ +174,5 @@
> +void
> +MobileViewportManager::UpdateSPCSPS(const ScreenIntSize& aDisplaySize,
> +                                    const CSSToScreenScale& aZoom)
> +{
> +  ScreenSize compositionSize = ScreenSize(aDisplaySize);

ScreenSize compositionSize(aDisplaySize);

@@ +179,5 @@
> +  ScreenMargin scrollbars =
> +    CSSMargin::FromAppUnits(
> +      nsLayoutUtils::ScrollbarAreaToExcludeFromCompositionBoundsFor(
> +        mPresShell->GetRootScrollFrame()))
> +    * CSSToScreenScale(1.0f);

Please preserve the comment "Scrollbars are not subject to scaling, so CSS pixels = screen pixels for them", otherwise the use of CSSToScreenScale(1.0f) seems arbitrary.

(Feel free to also add "We know this is wrong, see bug 1168487." :-)).

@@ +192,5 @@
> +MobileViewportManager::UpdateDisplayPortMargins()
> +{
> +  if (nsIScrollableFrame* root = mPresShell->GetRootScrollFrameAsScrollable()) {
> +    nsLayoutUtils::CalculateAndSetDisplayPortMargins(root,
> +      nsLayoutUtils::RepaintMode::DoNotRepaint);

Very nice!

Just one concern about this approach: CalculateAndSetDisplayPortMargins() will get the composition size from CalculateCompositionSizeForFrame(), which for the RCD-RSF gets it from the wigdet size or content viewer size. Will those sizes have been updated to reflect the new display size by this point?

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #26)
> Also, let's be sure we get Timothy's input on:
>   - handling NS_UNCONSTRAINEDSIZE
>   - how to initialize mDisplaySize
> before landing.

Yup, agreed.

> > Yes, that can happen. For example if there is a meta-viewport tag that gets
> > processed before the first paint (which is actually quite often) then we get
> > a viewport computation while mIsFirstPaint=false.
> 
> I guess I was asking if that would be a problem, but I can't think of any
> reason why it would be.

Yeah, I don't think it should be a problem either.

> I was talking about cases like this:
>   1) The viewport changes (via any of the triggers).
>   2) Later, something calls ResizeReflow().
> 
> Before, (1) would call ResizeReflowOverride (via SetCSSViewport) and set
> mViewportOverridden, and then (2) wouldn't cause a reflow because
> ResizeReflow() would early-exit.
> 
> With your original patch, (1) and (2) would both call ResizeReflow(), and
> since no one would set mViewportOverridden, (2) would do a reflow as well.
> 
> With your current patch, if there's an MVM then ResizeReflow() only actually
> does a reflow if the CSS viewport changes, so I think this concern is
> addressed. (Please let me know if it made sense though.)

Ah, I see. And yes, I think your explanation here is correct and so the new patch shouldn't have this problem.

> mDocument is used by nsContentUtils::GetViewportInfo() which requires it to
> be non-null. We should either assert mDocument here, or null-check it before
> the GetViewportInfo() call.

Ok, will do.

> @@ +161,5 @@
> > +  float cssViewportChangeRatio = (mMobileViewportSize.width == 0)
> > +     ? 1.0f : aViewport.width / mMobileViewportSize.width;
> > +  LayoutDeviceToLayerScale oldRes(nsLayoutUtils::GetResolution(mPresShell));
> > +  LayoutDeviceToLayerScale newRes(oldRes.scale * aDisplayWidthChangeRatio
> > +    / cssViewportChangeRatio);
> 
> Sorry to nitpick on this, but I still think we're doing something different
> from before: we're using width ratios, while before we used the max of width
> and height ratios.
> 
> I think this may change behaviour in cases where the viewport's aspect ratio
> doesn't track the display's aspect ratio (for example, if the meta-viewport
> tag hardcodes both dimensions of the viewport). Arguably an edge case, but
> is there a motivation for not using the same calculation as before?

Hm. The original intent of the code was not check the heights at all. If the code in TabChild was changing the zoom based on a change in aspect ratio or viewport height then I think that's wrong. It looks like the height bit was added in bug 1016682, and probably introduced this behaviour inadvertently.

> > +  ScreenSize compositionSize = ScreenSize(aDisplaySize);
> 
> ScreenSize compositionSize(aDisplaySize);

Will fix.

> > +    * CSSToScreenScale(1.0f);
> 
> Please preserve the comment "Scrollbars are not subject to scaling, so CSS
> pixels = screen pixels for them", otherwise the use of
> CSSToScreenScale(1.0f) seems arbitrary.
> 
> (Feel free to also add "We know this is wrong, see bug 1168487." :-)).

Sure :)

> > +  if (nsIScrollableFrame* root = mPresShell->GetRootScrollFrameAsScrollable()) {
> > +    nsLayoutUtils::CalculateAndSetDisplayPortMargins(root,
> > +      nsLayoutUtils::RepaintMode::DoNotRepaint);
> 
> Very nice!
> 
> Just one concern about this approach: CalculateAndSetDisplayPortMargins()
> will get the composition size from CalculateCompositionSizeForFrame(), which
> for the RCD-RSF gets it from the wigdet size or content viewer size. Will
> those sizes have been updated to reflect the new display size by this point?

The widget size should have been updated, yes, since that happens before the resize propagates to the ViewManager and then to the PresShell. I'm not entirely sure where the content viewer fits in though - I'll have to look into that.

    
    

    
  
Comment on attachment 8631787 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v2)

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27)
> > @@ +161,5 @@
> > > +  float cssViewportChangeRatio = (mMobileViewportSize.width == 0)
> > > +     ? 1.0f : aViewport.width / mMobileViewportSize.width;
> > > +  LayoutDeviceToLayerScale oldRes(nsLayoutUtils::GetResolution(mPresShell));
> > > +  LayoutDeviceToLayerScale newRes(oldRes.scale * aDisplayWidthChangeRatio
> > > +    / cssViewportChangeRatio);
> > 
> > Sorry to nitpick on this, but I still think we're doing something different
> > from before: we're using width ratios, while before we used the max of width
> > and height ratios.
> > 
> > I think this may change behaviour in cases where the viewport's aspect ratio
> > doesn't track the display's aspect ratio (for example, if the meta-viewport
> > tag hardcodes both dimensions of the viewport). Arguably an edge case, but
> > is there a motivation for not using the same calculation as before?
> 
> Hm. The original intent of the code was not check the heights at all. If the
> code in TabChild was changing the zoom based on a change in aspect ratio or
> viewport height then I think that's wrong. It looks like the height bit was
> added in bug 1016682, and probably introduced this behaviour inadvertently.

Hmm, good point. In TabChild, the same quantity ('oldIntrinsicScale') was used by the *initial zoom* calculation and by the *zoom change* calculation. The point of the part 3 patch in bug 1016682 was to make the *initial zoom* calculation in TabChild consistent with the *minimum zoom* calculation in APZC::OnScale(), which considers both the width and the height.

In this patch, the *initial zoom* calculation and the *zoom change* calculation are separated, and the *initial zoom* does still consider the height (it uses MaxScaleRatio). 

For the *zoom change* calculation, it's probably appropriate to just consider the width as you say (the comment even says "maintain how much acutal content is visible within the screen *width*).

That's an r+ with comments addressed, then :)

        Attachment #8631787 -
        Flags: review?(botond) → review+

    
    

    
  
Thanks! I think the nsIContentViewer path should also be safe - the bounds used in the composition bounds calculation are updated at [1] which is before the widget/viewmanager resizing (a few lines down) and therefore before the presShell ResizeReflow call.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?rev=368c057b5b00#1935

    
    

    
  


  
Here's an updated part 2 with botond's review comments addressed. It's still causing test_basic_pan to fail on try (but not locally) so I have a bunch of try pushes pending with various attempts to figure out what's going on there. I'm 90% sure it's got to do with the mDisplaySize not being right at some point so maybe tn will find the problem as part of his review. I'll keep looking too.

        Attachment #8631787 -
        Attachment is obsolete: true

        Attachment #8631787 -
        Flags: review?(tnikkel)

        Attachment #8632278 -
        Flags: review?(tnikkel)

    
    

    
  
Something I just realized: unlike HandlePossibleViewportChange(), MobileViewportManager doesn't have a dependence on AsyncPanZoomEnabled(). Should it?

    
    

    
  
No, it's conditional on MetaViewportEnabled() which is more specific. Meta-viewports don't really make sense in a non-APZ world anyway because they assume some sort of zooming behaviour (and this is reflected in our prefs, where MetaViewportEnabled is only true on b2g and fennec).

    
    

    
  
After many try pushes I seem to have isolated the change needed: [1] seems to be the critical bit that's missing in the new patch (i.e. doing a ResizeReflow even if mIsFirstPaint is false and mMobileViewportSize == viewport). Try push at [2] has a green run, but I did a bunch of retriggers to verify. Also doing a few more try pushes to check some other things.

[1] https://hg.mozilla.org/try/rev/1a9b99900f1e
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d594f76d0e3e

    
    

    
  
Comment on attachment 8632991 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v4)

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

::: layout/base/MobileViewportManager.cpp
@@ +237,5 @@
> +    mMobileViewportSize = viewport;
> +  }
> +
> +  // Kick off a reflow.
> +  mPresShell->ResizeReflowIgnoreOverride(

I'm concerned about doing a reflow here unconditionally.

Going back to this comment:

(In reply to Botond Ballo [:botond] from comment #28)
> I was talking about cases like this:
>   1) The viewport changes (via any of the triggers).
>   2) Later, something calls ResizeReflow().
> 
> Before, (1) would call ResizeReflowOverride (via SetCSSViewport) and set
> mViewportOverridden, and then (2) wouldn't cause a reflow because
> ResizeReflow() would early-exit.
> 
> With your original patch, (1) and (2) would both call ResizeReflow(), and
> since no one would set mViewportOverridden, (2) would do a reflow as well.
> 
> With your current patch, if there's an MVM then ResizeReflow() only actually
> does a reflow if the CSS viewport changes, so I think this concern is
> addressed.

with this change, this concern is no longer addressed.

    
    

    
  
That's true, but clearly the change is needed in some cases. Conceptually if we're doing a second call to ResizeReflowOverride with the same arguments as the last time (which is the case here) it should be a no-op in terms of correctness. That is not the case here, so I suspect this is a latent bug in the layout code that is being exposed now. Until I can reproduce the issue on-demand (I haven't managed to get the test running on the loaner yet) it's going to be hard to debug.

    
    

    
  
At this point I'll continue trying to reproduce the problem on the loaner, but would really like Timothy to weigh in with his review comments before I go full-bore on debugging this problem. It might be that he'll spot the problem right away, or have comments that will affect the patch in other ways that make it a moot point.

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #19)
> @@ +111,5 @@
> > +MobileViewportManager::RefreshViewportSize()
> > +{
> > +  if (mLastDisplaySize == nsSize()) {
> > +    MVM_LOG("Get viewport hasn't been called in %p before, plucking display size from presShell\n", this);
> > +    mLastDisplaySize = mPresShell->GetPresContext()->GetVisibleArea().Size();
> 
> I'm going to defer to Timothy on this being the right thing to query.

So you want the screen size of the container that this document is in? So we are relying on no one calling ResizeReflowOverride before this? If that is the case why not just get the size from the content viewer. That should be the quantity you are looking for without the issue of requiring no size override happening, and no worries about unconstrained size.

    
    

    
  
Before I forget you'll need an IID on nsIPresShell.

    
    

    
  
Comment on attachment 8632991 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v4)

Clearing review flag for now. After talking with tn on IRC yesterday I corrected some of my incorrect assumptions and made some adjustments to the patch. The test_basic_pan thing is no longer an issue, but now there's a few reftests failing. I can reproduce at least one of these locally and after investigating it seems to be because the test uses reftest-zoom which can get applied before or after the first-paint. Depending on when it gets applied we can end up changing the resolution or not changing the resolution, and this causes issues. I'm looking into how to fix this properly.

        Attachment #8632991 -
        Flags: review?(tnikkel)

    
    

    
  


  
Ok, I think things are good now. Try push is still going at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0075e1defe7 but a slightly earlier version came back green so I'm putting it back up for review.

The main changes from the old version are:
- SetDisplaySize(...) is now RequestReflow(), and the size passed in to ResizeReflow is not used in MVM. Instead we pull the display size from the content viewer always.
- Updated the logic on when to update the resolution vs not, because full-zoom changes should not trigger resolution changes. Updated the large comment in UpdateResolution to document the changes I made
- Updated a hunk in TabChild where the content viewer bounds were getting set after the widget bounds rather than before (discussed this with tn on IRC). I added a comment to go with that
- Some tweaks to the logging so that they always print out the address of the object
- Possibly some other minor cosmetic changes here and there.

Enough changed that I'm re-requesting review from Botond as well.

        Attachment #8632991 -
        Attachment is obsolete: true

        Attachment #8634368 -
        Flags: review?(tnikkel)

        Attachment #8634368 -
        Flags: review?(botond)

    
    

    
  
Comment on attachment 8634368 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v5)

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

Thanks - this looks quite good!

One comment: I find the use of aForceUpdateResolution a bit fragile. In particular, there are two scenarios where it could do the wrong thing:

  (1) If, after a meta-viewport change, RequestReflow() is called before the 
      DOMMetaAdded event arrives. In this case, the RefreshViewportSize()
      call that picks up the meta-viewport change will have 
      aForceUpdateResolution = false, and will fail to enact a resolution
      change that we would want it to.

  (2) If a RefreshViewportSize() call is the first to see both a meta-viewport
      change and a full-zoom change. In this case, neither boolean value of
      aForceUpdateResolution is correct, because we want to update the
      resolution for the meta-viewport change, but not for the full-zoom change.

I understand that currently neither of these scenarios should arise, but that's a property of code elsewhere in the tree that we're relying on here.

We could avoid this reliance by ditching aForceUpdateResolution and having UpdateResolution just "do the right thing". (An outline of what that might look like: get a hold of / keep track of the "unclamped viewport size"; always adjust the resolution, but use the ratio of the unclamped viewport sizes in the calculation rather than the ratio of the clamped viewport sizes.)

Implementing that may be more trouble than it's worth, so given that the problematic scenarios don't arise in practice, I'm happy with just documenting our assumptions (e.g. "if multiple quantities that influence the viewport size calculation change, RefreshViewportSize() will be triggered separately for each change"). r+ with that change.

::: layout/base/MobileViewportManager.h
@@ +42,5 @@
> +  /* Updates the presShell resolution and returns the new zoom. */
> +  mozilla::CSSToScreenScale UpdateResolution(const nsViewportInfo& aViewportInfo,
> +                                             const mozilla::ScreenIntSize& aDisplaySize,
> +                                             const mozilla::CSSSize& aViewport,
> +                                             const mozilla::Maybe<float>& aDisplayWidthChangeRatio);

Given that Maybe<X> just bundles a boolean with X, I would pass Maybe<X> by value whenever I would pass X by value, but I don't feel strongly about this.

        Attachment #8634368 -
        Flags: review?(botond) → review+

    
    

    
  
Comment on attachment 8634368 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v5)

The ability to change the CSS viewport via ResizeReflowOverride was added for mobile applications. And I think that is still the only use of it (along with testing that functionality). So it seems odd that we need to support other callers of ResizeReflowOverride (via mViewportOverridden) in the PresShell. Can we handle this in MobileViewportManager and make it the only caller of ResizeReflow(Ignore)Override?

    
    

    
  
I was actually planning to get rid of mIsViewportOverridden entirely in a future patch so moving it into MVM seems unnecessary. Once I switch Fennec over to using MVM I want to change any tests that use SetCSSViewport to just have an appropriate meta tag instead so that they exercise the same MVM path as they would in production. If that's not possible for whatever reason I can definitely move the flag into MVM.

    
    

    
  
Comment on attachment 8634368 [details] [diff] [review]
Part 2 - Remove a lot of code from TabChild and add a little to MobileViewportManager (v5)

Okay.

        Attachment #8634368 -
        Flags: review?(tnikkel) → review+

    
    

    
  
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2b2d3c44819ef11e9a4b32ee15ceec9256769d83
changeset:  2b2d3c44819ef11e9a4b32ee15ceec9256769d83
user:       Kartikaya Gupta <kgupta@mozilla.com>
date:       Mon Jul 20 18:19:40 2015 -0400
description:
Bug 1178847 - Move the code from ChromeProcessController::InitializeRoot to APZCCallbackHelper so it can be reused in the child process. r=botond

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/313ea7f814d38ab08ac14c9611eeb1ce829239af
changeset:  313ea7f814d38ab08ac14c9611eeb1ce829239af
user:       Kartikaya Gupta <kgupta@mozilla.com>
date:       Mon Jul 20 18:19:40 2015 -0400
description:
Bug 1178847 - Add a MobileViewportManager to manage setting the CSS viewport on B2G. r=botond,tn

The MobileViewportManager ("MVM") is responsible for setting the CSS viewport on
any of the following events:
- a page is painted for the first time (on the before-first-paint event)
- a meta-viewport tag is added (on the DOMMetaAdded event)
- the full-zoom is changed (on the FullZoomChanged event)
- if the window is resized (ResizeReflow gets called as part of normal layout
  processing, and this will pick up a new CSS viewport from MVM)

If the CSS viewport changes or if it is the initial paint, the MVM additionally
calls SetResolutionAndScaleTo on the presShell to update the displayed zoom.
The APZ code in AsyncPanZoomController::NotifyLayersUpdated already has
corresponding code to accept this updated zoom when the CSS viewport changes.

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff54001c29acba2d3c691eeae0a6494e16df893
changeset:  8ff54001c29acba2d3c691eeae0a6494e16df893
user:       Kartikaya Gupta <kgupta@mozilla.com>
date:       Mon Jul 20 18:19:41 2015 -0400
description:
Bug 1178847 - Don't use the MobileViewportManager code on Mulet or Fennec yet. r=botond

    
    

    
  
sorry i had to back this out for reftest failures on xp like https://treeherder.mozilla.org/logviewer.html#?job_id=11931949&repo=mozilla-inbound

seems the assertion is 

21:58:24 INFO - [3924] ###!!! ASSERTION: Displayport must be a valid texture size: 'result.width <= GetMaxDisplayPortSize(aContent)', file c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/layout/base/nsLayoutUtils.cpp, line 1039
21:58:24 INFO - #01: nsLayoutUtils::GetDisplayPort(nsIContent *,nsRect *) [layout/base/nsLayoutUtils.cpp:1054]
21:58:24 INFO - #02: mozilla::ContainerState::ComputeOpaqueRect(nsDisplayItem *,nsIFrame const *,nsIFrame const *,mozilla::DisplayItemClip const &,nsDisplayList *,bool *,bool *) [layout/base/FrameLayerBuilder.cpp:3583]
21:58:24 INFO - #03: mozilla::ContainerState::ProcessDisplayItems(nsDisplayList *) [layout/base/FrameLayerBuilder.cpp:4003]
21:58:24 INFO - #04: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder *,mozilla::layers::LayerManager *,nsIFrame *,nsDisplayItem *,nsDisplayList *,mozilla::ContainerLayerParameters const &,mozilla::gfx::Matrix4x4 const *,unsigned int) [layout/base/FrameLayerBuilder.cpp:5061]
21:58:24 INFO - #05: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder *,mozilla::layers::LayerManager *,mozilla::ContainerLayerParameters const &) [layout/base/nsDisplayList.cpp:4165]
21:58:24 INFO - #06: nsDisplaySubDocument::BuildLayer(nsDisplayListBuilder *,mozilla::layers::LayerManager *,mozilla::ContainerLayerParameters const &) [layout/base/nsDisplayList.cpp:4213]
21:58:24 INFO - #07: nsDisplayResolution::BuildLayer(nsDisplayListBuilder *,mozilla::layers::LayerManager *,mozilla::ContainerLayerParameters const &) [layout/base/nsDisplayList.cpp:4360]
21:58:24 INFO - #08: mozilla::ContainerState::ProcessDisplayItems(nsDisplayList *) [layout/base/FrameLayerBuilder.cpp:3878]
21:58:24 INFO - #09: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder *,mozilla::layers::LayerManager *,nsIFrame *,nsDisplayItem *,nsDisplayList *,mozilla::ContainerLayerParameters const &,mozilla::gfx::Matrix4x4 const *,unsigned int) [layout/base/FrameLayerBuilder.cpp:5061]
21:58:24 INFO - #10: nsDisplayList::PaintRoot(nsDisplayListBuilder *,nsRenderingContext *,unsigned int) [layout/base/nsDisplayList.cpp:1555]
21:58:24 INFO - #11: nsLayoutUtils::PaintFrame(nsRenderingContext *,nsIFrame *,nsRegion const &,unsigned int,unsigned int) [layout/base/nsLayoutUtils.cpp:3331]
21:58:24 INFO - #12: PresShell::Paint(nsView *,nsRegion const &,unsigned int) [layout/base/nsPresShell.cpp:6144]
21:58:24 INFO - #13: nsViewManager::ProcessPendingUpdatesPaint(nsIWidget *) [view/nsViewManager.cpp:456]
21:58:24 INFO - #14: nsViewManager::ProcessPendingUpdatesForView(nsView *,bool) [view/nsViewManager.cpp:392]
21:58:24 INFO - #15: nsViewManager::ProcessPendingUpdates() [view/nsViewManager.cpp:1086]
21:58:24 INFO - #16: mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver *,__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:197]
21:58:24 INFO - #17: mozilla::RefreshDriverTimer::Tick(__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:187]
21:58:24 INFO - #18: mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:439]
21:58:24 INFO - #19: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:374]
21:58:24 INFO - #20: nsRunnableMethodImpl<void ( mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp),1,mozilla::TimeStamp>::Run() [xpcom/glue/nsThreadUtils.h:831]
21:58:24 INFO - #21: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:867]
21:58:24 INFO - #22: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/glue/nsThreadUtils.cpp:277]
21:58:24 INFO - #23: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:95]
21:58:24 INFO - #24: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:234]
21:58:24 INFO - #25: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:228]
21:58:24 INFO - #26: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:202]
21:58:24 INFO - #27: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:167]
21:58:24 INFO - #28: nsAppShell::Run() [widget/windows/nsAppShell.cpp:180]
21:58:24 INFO - #29: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:281]
21:58:24 INFO - #30: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4288]
21:58:24 INFO - #31: XREMain::XRE_main(int,char * * const,nsXREAppData const *) [toolkit/xre/nsAppRunner.cpp:4385]
21:58:24 INFO - #32: XRE_main [toolkit/xre/nsAppRunner.cpp:4474]
21:58:24 INFO - #33: do_main [browser/app/nsBrowserApp.cpp:212]
21:58:24 INFO - #34: NS_internal_main(int,char * *) [browser/app/nsBrowserApp.cpp:399]
21:58:24 INFO - #35: wmain [toolkit/xre/nsWindowsWMain.cpp:138]
21:58:24 INFO - #36: __tmainCRTStartup [f:/dd/vctools/crt/crtw32/startup/crt0.c:255]
21:58:24 INFO

    
    

    
  
Oh, looks like I was wrong in comment 32 - there are some tests that enable meta-viewport support without APZ. In this case the test has a viewport 10000 pixels wide, so it's probably getting zoomed way out and then setting a giant displayport with a low resolution, or something.

    
    

    
  
So it looks like this is happening on all the desktop platforms, but on OS X the tiles pref is enabled so we don't trip the assertion, and on Linux (on my machine at least) GetMaxDisplayportSize is sufficiently large that the assertion doesn't fire. Presumably on the WinXP the max displayport size is smaller and so this is tripping.

I think the right answer here is to only allow adjusting the resolution on platforms that support it. It's really starting to feel like we need a pref or #define to separate the zooming platforms from the non-zooming platforms.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #51)
> I think the right answer here is to only allow adjusting the resolution on
> platforms that support it. It's really starting to feel like we need a pref
> or #define to separate the zooming platforms from the non-zooming platforms.

For now I'll add an ifdef and reland (assuming my try push is green) but I filed bug 1186004 for this.

    
  
Depends on: 1187616

    
  
Depends on: 1187792
Depends on: 1194876
Depends on: 1200093

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: