1055557 - Properly notify the APZ of root zoom constraints and changes to zoom constraints

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The GetRootZoomConstraints callback needs to be hooked up in order to make zooming work. Also it would be good to call APZCTM::UpdateZoomConstraints when the zoom constraints change.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 (WIP) - Hook up GetRootZoomConstraints

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 (WIP) - Hook up UpdateZoomConstraints

I'm not really happy with the way this turned out. I might redo this to be more in C++ and use the nsViewportInfo code rather than the browser.js

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Not actively working on this

Comment 4

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1055557.patch

Would like to discuss those #ifdefs I've added. Looks like B2G had the same issue.

Looks like they were added just to make B2G happy. We might need the same https://bugzilla.mozilla.org/show_bug.cgi?id=619487

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Can you get a backtrace to the pref access that was causing the assertion? I'm surprised that your changes are causing an off-main-thread access to prefs; we should fix that if possible.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Stack to off-main-thread pref access

This is the answer to my comment above. This stack indicates a bug in the patch.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8606995 [details] [diff] [review]
Bug_1055557.patch

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

::: layout/base/nsPresShell.cpp
@@ +11151,5 @@
> +
> +  nsView* view = rootFrame->GetView();
> +  if (view && view->GetWidget()){
> +    nsIntRect rect;
> +    view->GetWidget()->GetBounds(rect);

Based on the IRC discussion it might be better to do something like this:

nsSize size = nsLayoutUtils::CalculateCompositionSizeForFrame(rootFrame, false);
int32_t auPerDevPixel = GetPresContext()->AppUnitsPerDevPixel();
ScreenIntSize screenSize = ScreenIntSize(size.width / auPerDevPixel, size.height / auPerDevPixel);

and then pass screenSize to GetViewportInfo.

::: widget/android/AndroidContentController.cpp
@@ +53,5 @@
>      AndroidBridge::Bridge()->PostTaskToUiThread(aTask, aDelayMs);
>  }
>  
> +bool
> +AndroidContentController::GetRootZoomConstraints(mozilla::layers::ZoomConstraints *aConstraints)

Rather than only implementing this in AndroidContentController I would rather put it in ChromeProcessController and share the code across all platforms.

@@ +56,5 @@
> +bool
> +AndroidContentController::GetRootZoomConstraints(mozilla::layers::ZoomConstraints *aConstraints)
> +{
> +    nsIPresShell* presShell = GetPresShell();
> +    return presShell->GetZoomConstraints(aConstraints);

We shouldn't be calling GetZoomConstraints every time this function is called. Instead we need a model like what happens at [1] where the zoom constraints are updated in the controller and cached there, and then the cached value is returned from GetRootZoomConstraints. That will also make the ifdef stuff in prefapi and Preferences.cpp unnecessary.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=4604104f73a8#225

Comment 8

[Kartikaya Gupta (email:[e-mail])]

During the vidyo check-in today we discussed how to architect this code, putting it here for future reference:

We want to hang a new class off the pres shell (this class needs a good name, TBD). This class would hold a ref to the document, and register for before-first-paint and DOMMetaAdded events. The code would be largely copied from TabChild. This class would also keep the ZoomConstraints for the document and update it as needed based on the events. The GeckoContentController implementation that needs to get the ZoomConstraints could access it through the pres shell. Eventually the TabChild code can also be refactored to get the ZoomConstraints from the presshell rather than having a separate copy of the code.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8606995 [details] [diff] [review]
Bug_1055557.patch

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

Based on comment 8, it looks like we'll be changing the approach here a bit, so I'm not going to review this version in detail.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -1285,5 @@
>    // different x and y scales. If it did, the calculations in this function
>    // would have to be adjusted (as e.g. it would no longer be valid to take
>    // the minimum or maximum of the ratios of the widths and heights of the
>    // page rect and the composition bounds).
> -  MOZ_ASSERT(mFrameMetrics.IsRootScrollable());

I don't think this check should need to be removed - I would expect the zoomed APZC on Fennec to be the one that returns true for IsRootScrollable() (isRootContent after bug 1158424).

Comment 10

[Danilo Cesar Lemes de Paula]

Botond: The thing is that isRootScrollable() simply returns IsRoot() now, I assume it's something that will be changed by bug 1158424, right?
That's something I did to get it working now.

Comment 11

[Botond Ballo [:botond]]

(In reply to Danilo Cesar Lemes de Paula from comment #10)
> Botond: The thing is that isRootScrollable() simply returns IsRoot() now, I
> assume it's something that will be changed by bug 1158424, right?
> That's something I did to get it working now.

As described in bug 1158424 comment 2, I expect the change from mIsRoot to mIsRootContent to basically just be a rename. I dont't expect the APZC which contains this flag to change. Therefore, if we're currently trying to zoom an APZC with mIsRoot = false, I think something is going wrong, and we should investigate.

Comment 12

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1055557.patch

still a wip, but already contains the cached Constraints. 
It's segfaulting, still checking...

Comment 13

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1055557.patch

Crash fix.
Works properly now.

About Botond's comment on the assert: will discuss that with them on IRC.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8608944 [details] [diff] [review]
Bug_1055557.patch

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

Looks like what we had in mind. I have some comments below. The biggest concern I have right now (and I should have realized this earlier, sorry) is that ChromeProcessController::GetRootZoomConstraints gets called on the compositor thread, and so we probably don't want to be touching the presShell in that function. We might be able to get rid of that function, I need to think about this a little more.

::: layout/base/ZoomConstraintsClient.cpp
@@ +9,5 @@
> +#include "nsLayoutUtils.h"
> +#include "nsViewportInfo.h"
> +#include "Units.h"
> +#include "nsPoint.h"
> +#include "FrameMetrics.h"

Organize includes alphabetically after ZoomConstraintsClient.h

@@ +22,5 @@
> +using namespace mozilla::layers;
> +
> +ZoomConstraintsClient::ZoomConstraintsClient() :
> +    mDocument(nullptr),
> +    mPresShell(nullptr),

Please use 2-space indentation throughout this file

@@ +62,5 @@
> +
> +    if (observerService) {
> +        observerService->AddObserver(this,
> +                                     DOMMETADATA,
> +                                     false);

This needs to be an AddEventListener on the document rather than an observer on the observerservice

@@ +81,5 @@
> +
> +NS_IMETHODIMP ZoomConstraintsClient::Observe(nsISupports *aSubject, const char * aTopic, const char16_t * aData)
> +{
> +    if (aSubject == mDocument)
> +        RefreshZoomConstraints();

braces on all single-line ifs please

::: layout/base/ZoomConstraintsClient.h
@@ +28,5 @@
> +
> +public:
> +    void Init(PresShell* aPresShell, nsIDocument *aDocument);
> +    bool GetZoomConstraints(mozilla::layers::ZoomConstraints *aOutConstraints);
> +    void Destroy();

2-space indentation please

::: layout/base/nsPresShell.h
@@ +817,5 @@
>    nsCallbackEventRequest*   mFirstCallbackEventRequest;
>    nsCallbackEventRequest*   mLastCallbackEventRequest;
>  
> +  // ZoomConstraintsClient
> +  nsRefPtr<ZoomConstraintsClient> mZoomConstraintsClient;

No need for this comment, it's kinda useless. The TouchManager one below is also kinda useless :)

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Ok, so I think I figured out how we should go about doing this.

First we need to add an API for the ZoomConstraintsClient to be able to notify the APZ of changes in the zoom constraints. I think we should add an API on nsIWidget for this, and invoke it in ZoomConstraintsClient::RefreshZoomConstraints (you can pass the zoom constraints to the presShell which knows how to get the widget).

The nsBaseWidget implementation of the API will need to store a copy of the zoom constraints into the ChromeProcessController, and will also call UpdateZoomConstraints on the APZCTreeManager. The PuppetWidget override of the API will just delegate to TabChild::SendUpdateZoomConstraints and that path will be mostly the same as it is now on the parent side (i.e. the code in RenderFrameParent.cpp shouldn't need to change at all).

Then we can implement ChromeProcessController::GetRootZoomConstraints to return the cached zoom constraints that were pushed in using the API above. And we don't need to store a copy in the ZoomConstraintsClient. This will make ChromeProcessController very similar to RemoteContentController with respect to handling the zoom constraints, and GetRootZoomConstraints will be safe to call on the compositor thread as long as we lock the zoom constraints appropriately.

Does that sound reasonable?

Comment 16

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1055557.patch

This patch contains kat's suggestions.

Comment 17

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1055557.patch

Missing braces...

Comment 18

[Danilo Cesar Lemes de Paula]

try:
remote:   https://hg.mozilla.org/try/rev/e87156c71e91
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87156c71e91

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8610637 [details] [diff] [review]
Bug_1055557.patch

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

Overall structure looks pretty good. One preliminary comment below and I'll review more carefully later today.

::: layout/base/ZoomConstraintsClient.cpp
@@ +60,5 @@
> +
> +  mDocument->AddEventListener(NS_LITERAL_STRING(DOMMETADATA),
> +                this,
> +                false, false, 0);
> +  mDocument->AddEventListener(NS_LITERAL_STRING(BEFORE_FIRST_PAINT),

the before-first-paint needs to be registered with the nsIObserverService like you were doing before. It's just the DOMMetaAdded that's an event and needs to be done this way.

Comment 20

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1055557.patch

Using nsIObserver and nsDOMEventListener now...

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8610666 [details] [diff] [review]
Bug_1055557.patch

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

This generally looks good, but there are a few things I'd like to see addressed.

::: gfx/layers/apz/public/GeckoContentController.h
@@ +99,5 @@
>    {
>      return false;
>    }
>  
> +  virtual void SetRootZoomConstraints(const ZoomConstraints& aConstraints)

Please add a comment describing what this function does (I know it's clear from the name, just for consistency with the other functions in the file).

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -1285,5 @@
>    // different x and y scales. If it did, the calculations in this function
>    // would have to be adjusted (as e.g. it would no longer be valid to take
>    // the minimum or maximum of the ratios of the widths and heights of the
>    // page rect and the composition bounds).
> -  MOZ_ASSERT(mFrameMetrics.IsRootScrollable());

I still don't think removing this assertion should be necessary. If it's firing, that indicates that something is wrong and we should investigate that.

::: gfx/layers/apz/util/ChromeProcessController.h
@@ +67,5 @@
>    nsRefPtr<APZEventState> mAPZEventState;
>    MessageLoop* mUILoop;
>  
> +  bool mHaveZoomConstraints;
> +  ZoomConstraints mCachedZoomConstraints;

This bool/object pair could nicely be replaced with a Maybe<ZoomConstraints>.

::: layout/base/ZoomConstraintsClient.cpp
@@ +17,5 @@
> +#include "Units.h"
> +
> +NS_IMPL_ISUPPORTS(ZoomConstraintsClient, nsIDOMEventListener, nsIObserver)
> +
> +#define DOMMETADATA "DOMMetaAdded"

s/DATA/ADDED (and maybe add underscores for clarity, as in DOM_META_ADDED)

@@ +52,5 @@
> +  mPresShell = nullptr;
> +}
> +
> +void
> +ZoomConstraintsClient::Init(PresShell* aPresShell, nsIDocument *aDocument)

Please be consistent about spacing: * before the space

@@ +63,5 @@
> +  mDocument = aDocument;
> +
> +  mDocument->AddEventListener(NS_LITERAL_STRING(DOMMETADATA),
> +                this,
> +                false, false, 0);

AddEventListener() has a three-argument version, so I just 'AddEventListener(NS_LITERAL_STRING(DOM_META_ADDED), this, false)' should be sufficient.

@@ +73,5 @@
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::HandleEvent(nsIDOMEvent *event)
> +{
> +  RefreshZoomConstraints();

Perhaps, just for good measure, we should check that the event type is DOM_META_ADDED? I know that's the only event this class subscribes to currently, but that could change in the future.

@@ +79,5 @@
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::Observe(nsISupports *aSubject, const char * aTopic, const char16_t * aData)
> +{
> +    if (aSubject == mDocument)

TabChild uses SameCOMIdentity() to compare the subject to the document. I guess we should do the same here?

Also, similar to HandleEvent(), I think for good measure we should check that the topic is BEFORE_FIRST_PAINT.

@@ +92,5 @@
> +  if (!rootFrame) {
> +    return;
> +  }
> +
> +  nsSize size = nsLayoutUtils::CalculateCompositionSizeForFrame(rootFrame, false);

The document's root scroll frame may not be the same as its root frame. We should be passing it the root scroll frame to this function.

The root scroll frame can be obtained via mPresShell->GetRootScrollFrame().

@@ +94,5 @@
> +  }
> +
> +  nsSize size = nsLayoutUtils::CalculateCompositionSizeForFrame(rootFrame, false);
> +  int32_t auPerDevPixel = mPresShell->GetPresContext()->AppUnitsPerDevPixel();
> +  ScreenIntSize screenSize = ScreenIntSize(size.width / auPerDevPixel, size.height / auPerDevPixel);

Can be simplified to:

ScreenIntSize screenSize(size.width / auPerDevPixel, size.height / auPerDevPixel);

@@ +97,5 @@
> +  int32_t auPerDevPixel = mPresShell->GetPresContext()->AppUnitsPerDevPixel();
> +  ScreenIntSize screenSize = ScreenIntSize(size.width / auPerDevPixel, size.height / auPerDevPixel);
> +
> +  nsViewportInfo viewportInfo =
> +    nsContentUtils::GetViewportInfo(mDocument, ScreenIntSize(screenSize.width, screenSize.height));

The second argument can just be 'screenSize'.

@@ +104,5 @@
> +
> +  zoomConstraints.mAllowZoom = viewportInfo.IsZoomAllowed();
> +  zoomConstraints.mAllowDoubleTapZoom = viewportInfo.IsDoubleTapZoomAllowed();
> +  zoomConstraints.mMinZoom.scale = viewportInfo.GetMinZoom().scale;
> +  zoomConstraints.mMaxZoom.scale = viewportInfo.GetMaxZoom().scale;

It would be nice to extract a helper function for getting a ZoomConstraints from an nsViewportInfo, and use it both here and in TabChild.

@@ +107,5 @@
> +  zoomConstraints.mMinZoom.scale = viewportInfo.GetMinZoom().scale;
> +  zoomConstraints.mMaxZoom.scale = viewportInfo.GetMaxZoom().scale;
> +
> +  uint32_t presShellId = 0;
> +  mozilla::layers::FrameMetrics::ViewID viewId = FrameMetrics::NULL_SCROLL_ID;

You can omit the mozilla::layers::.

@@ +116,5 @@
> +
> +  if (scrollIdentifiersValid && rootFrame->GetView()) {
> +    nsIWidget *widget = rootFrame->GetView()->GetWidget();
> +    if(widget) {
> +      widget->UpdateZoomConstraints(zoomConstraints, presShellId, viewId);

If the pres shell is for a subdocument rather than the root document, will one of GetView() or GetWidget() return null, so that UpdateZoomConstraints() isn't called? If not, we would be calling UpdateZoomConstraints(), which I argue elsewhere should be called Update*Root*ZoomConstraints(), for something that's not the root, which would be bad.

Alternatively, we could not create ZoomConstraintClients for non-root pres shells, as I suggest in a comment for nsPresShell.cpp.

::: layout/base/ZoomConstraintsClient.h
@@ +35,5 @@
> +
> +private:
> +  void RefreshZoomConstraints();
> +  nsCOMPtr<nsIDocument> mDocument;
> +  nsRefPtr<PresShell>   mPresShell;

We should be able to use nsIPresShell instead of PresShell here.

::: layout/base/nsIPresShell.h
@@ +111,5 @@
>  } // namespace dom
>  
>  namespace layers {
>  class LayerManager;
> +class ZoomConstraints;

This forward-declaration shouldn't be necessary.

::: layout/base/nsPresShell.cpp
@@ +982,5 @@
>    SetupFontInflation();
>  
>    mTouchManager.Init(this, mDocument);
> +
> +  mZoomConstraintsClient = new ZoomConstraintsClient();

Do we want to be creating a zoom constraints client for every pres shell? It seems we don't need to for:

  - pres shells in the child process (at least until we
    transition TabChild to use the same mechanism for
    setting the zoom constraints)

  - pres shells for subdocuments

Even if we do the right thing for such pres shells (i.e. make sure that RefreshZoomConstraints() doesn't actually cause anything to happen), we'd be subscribing to events for them unnecessarily.

::: layout/base/nsPresShell.h
@@ +815,5 @@
>    nsRefPtr<nsCaret>         mOriginalCaret;
>    nsCallbackEventRequest*   mFirstCallbackEventRequest;
>    nsCallbackEventRequest*   mLastCallbackEventRequest;
>  
> +  nsRefPtr<ZoomConstraintsClient> mZoomConstraintsClient;

I think this is also introducing a reference cycle between the ZoomConstraintsClient and the PresShell, so we need to make one of them a weak reference.

::: widget/nsBaseWidget.cpp
@@ +19,5 @@
>  #include "nsAppDirectoryServiceDefs.h"
>  #include "nsISimpleEnumerator.h"
>  #include "nsIContent.h"
>  #include "nsIDocument.h"
> +#include "Element.h"

Why is this include necessary?

@@ +1863,5 @@
>  }
>  
> +void nsBaseWidget::UpdateZoomConstraints(const ZoomConstraints& aConstraints,
> +                                         uint32_t aPresShellId,
> +                                         mozilla::layers::FrameMetrics::ViewID aViewId)

Just 'FrameMetrics' after adding the typedef to nsIWidget.h.

@@ +1871,5 @@
> +  }
> +
> +  uint64_t layersId = mCompositorParent->RootLayerTreeId();
> +  mAPZC->UpdateZoomConstraints(ScrollableLayerGuid(layersId, aPresShellId, aViewId), aConstraints);
> +  mGeckoContentController->SetRootZoomConstraints(aConstraints);

If this function is going to be calling SetRootZoomConstraints(), that means it's only valid to call it with the root's pres shell id / view id, not those of other scroll frames.

If that's the case, we should rename the function to UpdateRootZoomConstraints to make this clear.

::: widget/nsBaseWidget.h
@@ +316,5 @@
>    void Shutdown();
>  
> +  virtual void UpdateZoomConstraints(const ZoomConstraints &constraints,
> +                                     uint32_t aPresShellId,
> +                                     mozilla::layers::FrameMetrics::ViewID aViewId) override;

Just 'FrameMetrics' after adding the typedef to nsIWidget.h.

@@ +477,5 @@
>    nsRefPtr<CompositorParent> mCompositorParent;
>    nsRefPtr<mozilla::CompositorVsyncDispatcher> mCompositorVsyncDispatcher;
>    nsRefPtr<APZCTreeManager> mAPZC;
>    nsRefPtr<APZEventState> mAPZEventState;
> +  nsRefPtr<GeckoContentController> mGeckoContentController;

I think this would introduce a reference cycle, because ChromeProcessController holds a strong reference to the widget. Can we use a weak reference here instead?

::: widget/nsIWidget.h
@@ +2400,5 @@
>      virtual int32_t RoundsWidgetCoordinatesTo() { return 1; }
>  
> +    virtual void UpdateZoomConstraints(const ZoomConstraints &constraints,
> +                                     uint32_t aPresShellId,
> +                                     mozilla::layers::FrameMetrics::ViewID aViewId) { };

For consistency, do 'typedef mozilla::layers::FrameMetrics FrameMetrics;' above as well (and then just use 'FrameMetrics' here).

Also, please add a comment that describes what this function does. Mention in particular that (aPresShellId, aViewId) identify the scroll frame to which the zoom constraints apply. Also mention that this is currently only intended to be used by parent-process widgets.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8610666 [details] [diff] [review]
Bug_1055557.patch

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

I think Botond covered pretty much everything I had, except stylistic nits below. Also I do plan on removing the duplicated code from TabChild and re-using this stuff so I wouldn't worry about refactoring to reuse the code in TabChild, or preventing the creation of ZoomConstraintsClient for the child process.

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +226,5 @@
> +bool
> +ChromeProcessController::GetRootZoomConstraints(mozilla::layers::ZoomConstraints *aOutConstraints)
> +{
> +    if (mHaveZoomConstraints && aOutConstraints) {
> +        *aOutConstraints = mCachedZoomConstraints;

nit: 2 space indentation in this file

::: layout/base/ZoomConstraintsClient.cpp
@@ +18,5 @@
> +
> +NS_IMPL_ISUPPORTS(ZoomConstraintsClient, nsIDOMEventListener, nsIObserver)
> +
> +#define DOMMETADATA "DOMMetaAdded"
> +static const char BEFORE_FIRST_PAINT[] = "before-first-paint";

I'm not really sure why one of these is a #define and the other is a static const char, it would be nice to have them both the same.

@@ +71,5 @@
> +    observerService->AddObserver(this, BEFORE_FIRST_PAINT, false);
> +  }
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::HandleEvent(nsIDOMEvent *event)

newline after NS_IMETHODIMP

@@ +77,5 @@
> +  RefreshZoomConstraints();
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::Observe(nsISupports *aSubject, const char * aTopic, const char16_t * aData)

nit: fix spacing around *

@@ +80,5 @@
> +
> +NS_IMETHODIMP ZoomConstraintsClient::Observe(nsISupports *aSubject, const char * aTopic, const char16_t * aData)
> +{
> +    if (aSubject == mDocument)
> +        RefreshZoomConstraints();

braces and indentation

@@ +115,5 @@
> +        &presShellId, &viewId);
> +
> +  if (scrollIdentifiersValid && rootFrame->GetView()) {
> +    nsIWidget *widget = rootFrame->GetView()->GetWidget();
> +    if(widget) {

space after if

::: layout/base/nsPresShell.h
@@ +806,5 @@
>    nsCOMPtr<nsIContent>      mCurrentEventContent;
>    nsTArray<nsIFrame*>       mCurrentEventFrameStack;
>    nsCOMArray<nsIContent>    mCurrentEventContentStack;
> +
> +

remove blank lines

::: widget/nsBaseWidget.cpp
@@ +1866,5 @@
> +                                         uint32_t aPresShellId,
> +                                         mozilla::layers::FrameMetrics::ViewID aViewId)
> +{
> +  if (!mCompositorParent || !mAPZC) {
> +      return;

indentation

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8610666 [details] [diff] [review]
Bug_1055557.patch

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

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +232,5 @@
> +    return mHaveZoomConstraints;
> +}
> +
> +void
> +ChromeProcessController::SetRootZoomConstraints(const mozilla::layers::ZoomConstraints& aConstraints)

One thing I forgot: do we need some synchronization in these methods? IIUC SetRootZoomConstraints() will be called from the Gecko thread, while GetRootZoomConstraints() will be called from the compositor thread.

Comment 24

[Danilo Cesar Lemes de Paula]

Comment on attachment 8610666 [details] [diff] [review]
Bug_1055557.patch

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

::: gfx/layers/apz/public/GeckoContentController.h
@@ +99,5 @@
>    {
>      return false;
>    }
>  
> +  virtual void SetRootZoomConstraints(const ZoomConstraints& aConstraints)

Done

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -1285,5 @@
>    // different x and y scales. If it did, the calculations in this function
>    // would have to be adjusted (as e.g. it would no longer be valid to take
>    // the minimum or maximum of the ratios of the widths and heights of the
>    // page rect and the composition bounds).
> -  MOZ_ASSERT(mFrameMetrics.IsRootScrollable());

Ok. Kats and I agreed that I would left it here. It's going to crash the application but it will be obvious where to look.

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +226,5 @@
> +bool
> +ChromeProcessController::GetRootZoomConstraints(mozilla::layers::ZoomConstraints *aOutConstraints)
> +{
> +    if (mHaveZoomConstraints && aOutConstraints) {
> +        *aOutConstraints = mCachedZoomConstraints;

Done

@@ +232,5 @@
> +    return mHaveZoomConstraints;
> +}
> +
> +void
> +ChromeProcessController::SetRootZoomConstraints(const mozilla::layers::ZoomConstraints& aConstraints)

Probably yes. Something the one getting this task after me will have to take a look.

::: gfx/layers/apz/util/ChromeProcessController.h
@@ +67,5 @@
>    nsRefPtr<APZEventState> mAPZEventState;
>    MessageLoop* mUILoop;
>  
> +  bool mHaveZoomConstraints;
> +  ZoomConstraints mCachedZoomConstraints;

Ok, done.

::: layout/base/ZoomConstraintsClient.cpp
@@ +17,5 @@
> +#include "Units.h"
> +
> +NS_IMPL_ISUPPORTS(ZoomConstraintsClient, nsIDOMEventListener, nsIObserver)
> +
> +#define DOMMETADATA "DOMMetaAdded"

DONE

@@ +18,5 @@
> +
> +NS_IMPL_ISUPPORTS(ZoomConstraintsClient, nsIDOMEventListener, nsIObserver)
> +
> +#define DOMMETADATA "DOMMetaAdded"
> +static const char BEFORE_FIRST_PAINT[] = "before-first-paint";

Bith are now nsStrings (one is nsLiteral and the other nsCString because of how observer and addeventlistener requires the target string)

@@ +52,5 @@
> +  mPresShell = nullptr;
> +}
> +
> +void
> +ZoomConstraintsClient::Init(PresShell* aPresShell, nsIDocument *aDocument)

Done.

@@ +63,5 @@
> +  mDocument = aDocument;
> +
> +  mDocument->AddEventListener(NS_LITERAL_STRING(DOMMETADATA),
> +                this,
> +                false, false, 0);

Ok, done.

@@ +71,5 @@
> +    observerService->AddObserver(this, BEFORE_FIRST_PAINT, false);
> +  }
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::HandleEvent(nsIDOMEvent *event)

done.

@@ +73,5 @@
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::HandleEvent(nsIDOMEvent *event)
> +{
> +  RefreshZoomConstraints();

OK, done.

@@ +77,5 @@
> +  RefreshZoomConstraints();
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::Observe(nsISupports *aSubject, const char * aTopic, const char16_t * aData)

Done.

@@ +79,5 @@
> +}
> +
> +NS_IMETHODIMP ZoomConstraintsClient::Observe(nsISupports *aSubject, const char * aTopic, const char16_t * aData)
> +{
> +    if (aSubject == mDocument)

Done.

@@ +92,5 @@
> +  if (!rootFrame) {
> +    return;
> +  }
> +
> +  nsSize size = nsLayoutUtils::CalculateCompositionSizeForFrame(rootFrame, false);

It didn't work with the GetRootScrollFrame()

Probably the same issue as the MOZ_ASSERT?

@@ +94,5 @@
> +  }
> +
> +  nsSize size = nsLayoutUtils::CalculateCompositionSizeForFrame(rootFrame, false);
> +  int32_t auPerDevPixel = mPresShell->GetPresContext()->AppUnitsPerDevPixel();
> +  ScreenIntSize screenSize = ScreenIntSize(size.width / auPerDevPixel, size.height / auPerDevPixel);

Done.

@@ +97,5 @@
> +  int32_t auPerDevPixel = mPresShell->GetPresContext()->AppUnitsPerDevPixel();
> +  ScreenIntSize screenSize = ScreenIntSize(size.width / auPerDevPixel, size.height / auPerDevPixel);
> +
> +  nsViewportInfo viewportInfo =
> +    nsContentUtils::GetViewportInfo(mDocument, ScreenIntSize(screenSize.width, screenSize.height));

Done.

@@ +104,5 @@
> +
> +  zoomConstraints.mAllowZoom = viewportInfo.IsZoomAllowed();
> +  zoomConstraints.mAllowDoubleTapZoom = viewportInfo.IsDoubleTapZoomAllowed();
> +  zoomConstraints.mMinZoom.scale = viewportInfo.GetMinZoom().scale;
> +  zoomConstraints.mMaxZoom.scale = viewportInfo.GetMaxZoom().scale;

I made a new method, but I didn't place it in a common place as Kats will refactor ChildTab later.

@@ +107,5 @@
> +  zoomConstraints.mMinZoom.scale = viewportInfo.GetMinZoom().scale;
> +  zoomConstraints.mMaxZoom.scale = viewportInfo.GetMaxZoom().scale;
> +
> +  uint32_t presShellId = 0;
> +  mozilla::layers::FrameMetrics::ViewID viewId = FrameMetrics::NULL_SCROLL_ID;

ok.

@@ +115,5 @@
> +        &presShellId, &viewId);
> +
> +  if (scrollIdentifiersValid && rootFrame->GetView()) {
> +    nsIWidget *widget = rootFrame->GetView()->GetWidget();
> +    if(widget) {

ok

::: layout/base/ZoomConstraintsClient.h
@@ +35,5 @@
> +
> +private:
> +  void RefreshZoomConstraints();
> +  nsCOMPtr<nsIDocument> mDocument;
> +  nsRefPtr<PresShell>   mPresShell;

Done.

::: layout/base/nsIPresShell.h
@@ +111,5 @@
>  } // namespace dom
>  
>  namespace layers {
>  class LayerManager;
> +class ZoomConstraints;

Done.

::: layout/base/nsPresShell.cpp
@@ +982,5 @@
>    SetupFontInflation();
>  
>    mTouchManager.Init(this, mDocument);
> +
> +  mZoomConstraintsClient = new ZoomConstraintsClient();

I believe you're right.
In our first meetings about this we talked about getting it into all presShells, and just ignore those in childprocess. I think that's what's happening. Those events are not necessary, but it's something that the one getting this task after me will have to take a look.

::: layout/base/nsPresShell.h
@@ +806,5 @@
>    nsCOMPtr<nsIContent>      mCurrentEventContent;
>    nsTArray<nsIFrame*>       mCurrentEventFrameStack;
>    nsCOMArray<nsIContent>    mCurrentEventContentStack;
> +
> +

Done.

@@ +815,5 @@
>    nsRefPtr<nsCaret>         mOriginalCaret;
>    nsCallbackEventRequest*   mFirstCallbackEventRequest;
>    nsCallbackEventRequest*   mLastCallbackEventRequest;
>  
> +  nsRefPtr<ZoomConstraintsClient> mZoomConstraintsClient;

Fair. Done.

::: widget/nsBaseWidget.cpp
@@ +19,5 @@
>  #include "nsAppDirectoryServiceDefs.h"
>  #include "nsISimpleEnumerator.h"
>  #include "nsIContent.h"
>  #include "nsIDocument.h"
> +#include "Element.h"

It was before. It's not anymore. Fixed.

@@ +1863,5 @@
>  }
>  
> +void nsBaseWidget::UpdateZoomConstraints(const ZoomConstraints& aConstraints,
> +                                         uint32_t aPresShellId,
> +                                         mozilla::layers::FrameMetrics::ViewID aViewId)

Done

@@ +1866,5 @@
> +                                         uint32_t aPresShellId,
> +                                         mozilla::layers::FrameMetrics::ViewID aViewId)
> +{
> +  if (!mCompositorParent || !mAPZC) {
> +      return;

Done

@@ +1871,5 @@
> +  }
> +
> +  uint64_t layersId = mCompositorParent->RootLayerTreeId();
> +  mAPZC->UpdateZoomConstraints(ScrollableLayerGuid(layersId, aPresShellId, aViewId), aConstraints);
> +  mGeckoContentController->SetRootZoomConstraints(aConstraints);

Fair. Renamed.

::: widget/nsBaseWidget.h
@@ +316,5 @@
>    void Shutdown();
>  
> +  virtual void UpdateZoomConstraints(const ZoomConstraints &constraints,
> +                                     uint32_t aPresShellId,
> +                                     mozilla::layers::FrameMetrics::ViewID aViewId) override;

Done.

@@ +477,5 @@
>    nsRefPtr<CompositorParent> mCompositorParent;
>    nsRefPtr<mozilla::CompositorVsyncDispatcher> mCompositorVsyncDispatcher;
>    nsRefPtr<APZCTreeManager> mAPZC;
>    nsRefPtr<APZEventState> mAPZEventState;
> +  nsRefPtr<GeckoContentController> mGeckoContentController;

I don't think we can use WeakRef here, as GeckoContentController is not a ISupport (I assume it's the requirement to support weakref)

Comment 25

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1055557.patch

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Thanks Danilo! I can pick this up and look into the remaining issues with the patch.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Move storage of ZoomConstraints from RemoteContentController to APZCTM

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Const-ify some methods

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Add a ZoomConstraintsClient class to push ZoomConstraints updates to APZCTM

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d40c3d2be64f includes these patches. Will wait for green before flagging this last patch for review since it's the highest-risk one.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Add a ZoomConstraintsClient class to push ZoomConstraints updates to APZCTM

Slightly updated, this works better. Clean try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=698badca2736

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8621661 [details] [diff] [review]
Part 1 - Move storage of ZoomConstraints from RemoteContentController to APZCTM

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

::: dom/ipc/PBrowser.ipdl
@@ +83,5 @@
> +union MaybeZoomConstraints
> +{
> +  ZoomConstraints;
> +  void_t;
> +};

I'm not a huge fan of this method of getting Maybes across IPDL. Could we instead:

  - typedef Maybe<ZoomConstraints> to MaybeZoomConstraints in C++ code

  - do "using whatever::MaybeZoomConstraints" in PBrowser.ipdl

      (these two steps are unnecessary if you can write
       'Maybe<ZoomConstraints>'in IPDL, but I'm guessing you can't)

  - add serialization support for Maybe<T> to GfxMessageUtils.h
    or similar (that doesn't need to be specific to ZoomConstraints)

::: dom/ipc/TabChild.cpp
@@ +2806,5 @@
>  
> +  if (mLastZoomConstraintsGuid) {
> +    DoUpdateZoomConstraints(mLastZoomConstraintsGuid->mPresShellId,
> +                            mLastZoomConstraintsGuid->mScrollId,
> +                            Nothing());

I don't know much about shutdown - is it safe to send an IPDL message in RecvDestroy()?

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1036,5 @@
> +  // which have their own zoom constraints or are in a different layers id.
> +  if (aConstraints) {
> +    APZCTM_LOG("Recording constraints %s for guid %s\n",
> +      Stringify(aConstraints.value()).c_str(), Stringify(aGuid).c_str());
> +    mZoomConstraints[aGuid] = aConstraints.value();

You can use ref() here, too (the map already makes a copy).

@@ +1042,5 @@
> +    APZCTM_LOG("Removing constraints for guid %s\n", Stringify(aGuid).c_str());
> +    mZoomConstraints.erase(aGuid);
> +  }
> +  if (node && aConstraints) {
> +    UpdateZoomConstraintsRecursively(node.get(), aConstraints.ref());

Do we need to do anything if the constraints are being removed?

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8622072 [details] [diff] [review]
Part 3 - Add a ZoomConstraintsClient class to push ZoomConstraints updates to APZCTM

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

Is the clearing of the zoom constraints something that we weren't doing before but should have been?

::: dom/ipc/TabChild.cpp
@@ -478,5 @@
> -        ConvertScaleForRoot(viewportInfo.GetMinZoom()),
> -        ConvertScaleForRoot(viewportInfo.GetMaxZoom()));
> -      DoUpdateZoomConstraints(presShellId,
> -                              viewId,
> -                              Some(constraints));

Do you know why TabChild was (potentially) calling DoUpdateZoomConstraints() twice in HandlePossibleViewportChange() to begin with?

::: layout/base/ZoomConstraintsClient.cpp
@@ +157,5 @@
> +  if (!scrollIdentifiersValid) {
> +    return;
> +  }
> +
> +  nsSize size = nsLayoutUtils::CalculateCompositionSizeForFrame(rootFrame, false);

All other callers of CalculateCompositionSizeForFrame() pass in the root scroll frame, not the root frame. I think we should be doing that here too.

@@ +164,5 @@
> +        size, auPerDevPixel);
> +
> +  nsViewportInfo viewportInfo = nsContentUtils::GetViewportInfo(
> +    mDocument,
> +    ViewAs<ScreenPixel>(screenSize, PixelCastJustification::LayoutDeviceIsScreenForBounds));

Does this give us the value we were passing before in TabChild (mInnerSize)?

@@ +175,5 @@
> +    // then we disable double-tap-to-zoom behaviour.
> +    CSSToLayoutDeviceScale scale(
> +      (float)nsPresContext::AppUnitsPerCSSPixel() / auPerDevPixel);
> +    if ((viewportInfo.GetSize() * scale).width <= screenSize.width) {
> +      zoomConstraints.mAllowDoubleTapZoom = false;

If I'm reading the TabChild code correctly, it was also doing the reverse adjustment, i.e. setting mAllowDoubleTapZoom to true if the viewport is wider than the screen (conditioned on mAllowZoom being true). Should we do doing that too?

::: layout/base/ZoomConstraintsClient.h
@@ +37,5 @@
> +private:
> +  void RefreshZoomConstraints();
> +
> +  nsCOMPtr<nsIDocument> mDocument;
> +  nsIPresShell* mPresShell; // raw ref since the presShell owns this

Consider marking this MOZ_NON_OWNING_REF, to make the life of people working on bug 1114683 a little bit easier.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #31)
> I'm not a huge fan of this method of getting Maybes across IPDL. Could we
> instead:
> 
>   - typedef Maybe<ZoomConstraints> to MaybeZoomConstraints in C++ code
> 
>   - do "using whatever::MaybeZoomConstraints" in PBrowser.ipdl
> 
>       (these two steps are unnecessary if you can write
>        'Maybe<ZoomConstraints>'in IPDL, but I'm guessing you can't)
> 
>   - add serialization support for Maybe<T> to GfxMessageUtils.h
>     or similar (that doesn't need to be specific to ZoomConstraints)

Good suggestions, thanks! I also didn't really like the union thing. Turns out there is already support for Maybe<T> in ipc/glue/IPCMessageUtils.h (dvander added it recently) but the IPDL syntax still doesn't support it. Adding a typedef and using that in the IPDL seems to work though.

> ::: dom/ipc/TabChild.cpp
> @@ +2806,5 @@
> >  
> > +  if (mLastZoomConstraintsGuid) {
> > +    DoUpdateZoomConstraints(mLastZoomConstraintsGuid->mPresShellId,
> > +                            mLastZoomConstraintsGuid->mScrollId,
> > +                            Nothing());
> 
> I don't know much about shutdown - is it safe to send an IPDL message in
> RecvDestroy()?

I'm not sure about the general case but I did exercise this patch in isolation and didn't run into any problems here, so I assume yes. That being said I didn't exercise it very heavily since I knew that this code would be removed in part 3.

> > +    APZCTM_LOG("Recording constraints %s for guid %s\n",
> > +      Stringify(aConstraints.value()).c_str(), Stringify(aGuid).c_str());
> > +    mZoomConstraints[aGuid] = aConstraints.value();
> 
> You can use ref() here, too (the map already makes a copy).

Done

> > +  if (node && aConstraints) {
> > +    UpdateZoomConstraintsRecursively(node.get(), aConstraints.ref());
> 
> Do we need to do anything if the constraints are being removed?

Not really. In practice the APZCs will also be removed shortly (if they haven't been removed already) and so it's a moot point. If we did want to update APZCs in the tree, then the most correct thing to do would be to find the APZC whose constraints are being removed, walk upwards from there to find the nearest ancestor zoom constraints in the same process, and then propagate that downwards to the affected APZC and its descendants (until we find another APZC which has constraints or we hit the layer boundary). This seemed rather complex which is why I decided to just not do anything, given that the APZCs themselves are going to be removed from the tree.

(In reply to Botond Ballo [:botond] from comment #32)
> Comment on attachment 8622072 [details] [diff] [review]
> 
> Is the clearing of the zoom constraints something that we weren't doing
> before but should have been?

No, previously we only stored the root zoom constraints in RenderFrameParent. It was always either getting updated, or if the child process was terminated then the RenderFrameParent itself would get destroyed, so "clearing the zoom constraints" wasn't something we had to worry about. Now that we are storing the zoom constraints in the APZCTM which can outlive the child process, we do need to worry about clearing out stale zoom constraints.

> 
> Do you know why TabChild was (potentially) calling DoUpdateZoomConstraints()
> twice in HandlePossibleViewportChange() to begin with?

I think it was just silliness on my part when I added this code in bug 941995. I should have moved up the check once I decided that the condition was based entirely on the viewport metadata rather than the actual content width.

> > +
> > +  nsSize size = nsLayoutUtils::CalculateCompositionSizeForFrame(rootFrame, false);
> 
> All other callers of CalculateCompositionSizeForFrame() pass in the root
> scroll frame, not the root frame. I think we should be doing that here too.

Ok, I'll try that and see what happens. Danilo said that it didn't work for him but I can investigate.

> > +  nsViewportInfo viewportInfo = nsContentUtils::GetViewportInfo(
> > +    mDocument,
> > +    ViewAs<ScreenPixel>(screenSize, PixelCastJustification::LayoutDeviceIsScreenForBounds));
> 
> Does this give us the value we were passing before in TabChild (mInnerSize)?

I didn't check that explicitly; I can do that.

> If I'm reading the TabChild code correctly, it was also doing the reverse
> adjustment, i.e. setting mAllowDoubleTapZoom to true if the viewport is
> wider than the screen (conditioned on mAllowZoom being true). Should we do
> doing that too?

Technically you're right but I don't think we should ever fall into a case where it matters. There are only two cases where GetViewportInfo returns mAllowZoom=true with mAllowDoubleTapZoom=false ([1] and [2]) and in both of those cases we set the viewport width equal to the screen width value, so we should have mAllowDoubleTapZoom=false anyway. I also find my change makes the intent of the code clearer (i.e. do what the meta-viewport tag says, but explicitly disable double-tapping in certain cases).

> 
> Consider marking this MOZ_NON_OWNING_REF, to make the life of people working
> on bug 1114683 a little bit easier.

Will do, thanks!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=c219548106bb#7917
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=c219548106bb#7929

Comment 34

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #33)
> there is already support for Maybe<T> in ipc/glue/IPCMessageUtils.h
> (dvander added it recently) but the IPDL syntax still doesn't support it.

(Filed bug 1174875 for this.)

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Move storage of ZoomConstraints from RemoteContentController to APZCTM (v2)

Rebased to master and updated per review comments.

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Add a ZoomConstraintsClient class to push ZoomConstraints updates to APZCTM (v2)

Rebased to master and updated per review comments. In particular:
- I changed the GetRootFrame to GetRootScrollFrame as requested, it seems to work fine
- I extracted a GetWidget helper method in ZoomConstraintsClient because I noticed that in the Destroy() function I was still getting the widget from the view, when on Android we should always be using GetNearestWidget(). The helper method moves that into one place
- I also verified that mInnerSize is the same as the composition size returned in the new code, at least in basic B2G use cases (homescreen, settings app, browser pages with and without meta-viewport)

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Correctly untransform parent-process clicks when zooming is applied

I could split this out into a separate bug but it's easier to throw it in here :) On Android we can have zoomed presShells in the parent process and so we need to account for that in ChromeProcessController.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

New try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf6bc14f15d5

Comment 39

[Botond Ballo [:botond]]

Comment on attachment 8622691 [details] [diff] [review]
Part 1 - Move storage of ZoomConstraints from RemoteContentController to APZCTM (v2)

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

Thanks!

Comment 40

[Botond Ballo [:botond]]

Comment on attachment 8622694 [details] [diff] [review]
Part 3 - Add a ZoomConstraintsClient class to push ZoomConstraints updates to APZCTM (v2)

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #33)
> (In reply to Botond Ballo [:botond] from comment #32)
> > Comment on attachment 8622072 [details] [diff] [review]
> > 
> > Is the clearing of the zoom constraints something that we weren't doing
> > before but should have been?
> 
> No, previously we only stored the root zoom constraints in
> RenderFrameParent. It was always either getting updated, or if the child
> process was terminated then the RenderFrameParent itself would get
> destroyed, so "clearing the zoom constraints" wasn't something we had to
> worry about. Now that we are storing the zoom constraints in the APZCTM
> which can outlive the child process, we do need to worry about clearing out
> stale zoom constraints.

Thanks, I didn't fully appreciate this when I first read the patch.

> > If I'm reading the TabChild code correctly, it was also doing the reverse
> > adjustment, i.e. setting mAllowDoubleTapZoom to true if the viewport is
> > wider than the screen (conditioned on mAllowZoom being true). Should we do
> > doing that too?
> 
> Technically you're right but I don't think we should ever fall into a case
> where it matters. There are only two cases where GetViewportInfo returns
> mAllowZoom=true with mAllowDoubleTapZoom=false ([1] and [2]) and in both of
> those cases we set the viewport width equal to the screen width value, so we
> should have mAllowDoubleTapZoom=false anyway. I also find my change makes
> the intent of the code clearer (i.e. do what the meta-viewport tag says, but
> explicitly disable double-tapping in certain cases).

OK, this is fine as-is then.

> - I also verified that mInnerSize is the same as the composition size
> returned in the new code, at least in basic B2G use cases (homescreen,
> settings app, browser pages with and without meta-viewport)

Thanks for checking!

Comment 41

[Botond Ballo [:botond]]

Comment on attachment 8622695 [details] [diff] [review]
Part 4 - Correctly untransform parent-process clicks when zooming is applied

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

A couple of things:

  - What if HandleSingleTap() or HandleLongTap() is called with a guid belonging to
    a subdocument of the root content document? It looks to me like the current
    implementation of GetPresShellResolution() will return 1 in such a case, but I
    think we want it to reutrn the root content doc's resolution.

  - Not strictly related, but I noticed that in TabChild we also consume the pres
    shell resolution in TabChild::RecvRealTouchEvent() where we apply the callback
    transform to the touch event. Should the equivalent chrome-process code path
    (nsBaseWidget::ProcessUntransformedAPZEvent()) be doing the same?

Comment 42

[Kartikaya Gupta (email:[e-mail])]

Excellent points, I'll address both. It seems easiest to just stop passing around the presShell resolution from TabChild and nsBaseWidget and just compute it from the guid in ApplyCallbackTransform directly, which is where it all flows in to.

Also the try push above had a lot of crashes, it looks like because GetRootScrollFrame often returns nullptr. I think in those cases it should be ok to use the root frame instead? I'll update the patch to do that but if you think we should just bail out in those cases let me know.

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Add a ZoomConstraintsClient class to push ZoomConstraints updates to APZCTM (v3)

Same as before, but now it uses GetRootFrame if GetRootScrollFrame returns null.

Comment 44

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Ensure the right presShell resolution is used in ApplyCallbackTransform (v2)

Comment 45

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c927931dd171

Comment 46

[Botond Ballo [:botond]]

Comment on attachment 8623201 [details] [diff] [review]
Part 3 - Add a ZoomConstraintsClient class to push ZoomConstraints updates to APZCTM (v3)

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

> Also the try push above had a lot of crashes, it looks like because
> GetRootScrollFrame often returns nullptr. I think in those cases it should
> be ok to use the root frame instead? I'll update the patch to do that but if
> you think we should just bail out in those cases let me know.

I guess when there's no root scroll frame, CalculateCompositionSizeForFrame() will get a null result out of GetScrollTargetFrame() [1], but it handles that gracefully, so this should be fine.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=16e74ccdc8bf#7806

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8623202 [details] [diff] [review]
Part 4 - Ensure the right presShell resolution is used in ApplyCallbackTransform (v2)

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

Nice to be passing around things less.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +68,5 @@
>  
>  // #define APZC_ENABLE_RENDERTRACE
>  
> +// #define ENABLE_APZC_LOGGING 0
> +#define ENABLE_APZC_LOGGING 1

Exclude this change

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +365,5 @@
> +    nsPresContext* context = shell->GetPresContext();
> +    if (!context) {
> +        return nullptr;
> +    }
> +    context = context->GetToplevelContentDocumentPresContext();

Convenient :)

@@ +388,5 @@
> +
> +    // First, scale inversely by the root content document's pres shell
> +    // resolution to cancel the scale-to-resolution transform that the
> +    // compositor adds to the layer with the pres shell resolution. The points
> +    // sent to Gecko by APZ don't have/ this transform unapplied (unlike other

stray '/'

::: gfx/layers/apz/util/ChromeProcessController.h
@@ +61,5 @@
>    nsRefPtr<APZEventState> mAPZEventState;
>    MessageLoop* mUILoop;
>  
>    void InitializeRoot();
> +  float GetPresShellResolution(const ScrollableLayerGuid& aGuid) const;

Since we're removing both callers, just remove the function.

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Addressed review comments and landed.

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0ce095ef3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7e1b9479c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc009c8ce7c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44cae0757a4

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d0ce095ef3e
https://hg.mozilla.org/mozilla-central/rev/5a7e1b9479c3
https://hg.mozilla.org/mozilla-central/rev/dc009c8ce7c3
https://hg.mozilla.org/mozilla-central/rev/b44cae0757a4