1092450 - Disentangle meta-viewport from e10s async scrolling

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: meta_viewport_changes.patch

Currently, to make remote browser frames scrollab, you have to enable layout.browser_frames.useAsyncPanZoom. This also enables meta "viewport", which we don't want on Desktop.

This patch renames the pref to "layout.css.meta-viewport.enabled", and if it's disabled, we just set a displayport instead. It also automatically enables browser-frame async scrolling if apzc is enabled.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

That should read "scrollable" and "dom.browser_frames.useAsyncPanZoom".

Comment 2

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

Comment on attachment 8515405 [details] [diff] [review]
meta_viewport_changes.patch

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

There's some issues with this patch which I've commented on below but at a high level I'm also tagging f? to fabrice to confirm that it's ok to remove the dom.browser_frames.useAsyncPanZoom pref. If we go ahead with this, there's additional stuff that should be cleaned up as part of this:
- A gecko patch that removes the gaia setting at http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js?rev=32b0ce626ec8#513 because the dom.browser_frames.useAsyncPanZoom pref is a no-op now
- A gaia patch that removes usage of the apz.force-enable setting (see http://mxr.mozilla.org/gaia/search?string=apz.force-enable). You should effectively hard-code it to true
- We can probably also remove the mozasyncpanzoom attribute that gets set on iframes - the usage of it in nsFrameLoader.cpp and any gaia code that sets it.

If we do for whatever reason want to retain the ability to turn off APZ on general browser frames while allowing specific frames to opt-in, then we should keep the dom.browser_frames.useAsyncPanZoom pref as-is but also introduce the meta-viewport pref in addition to that.

::: b2g/app/b2g.js
@@ +433,5 @@
>  // We'll run out of PIDs on UNIX-y systems before we hit this limit.
>  pref("dom.ipc.processCount", 100000);
>  
>  pref("dom.ipc.browser_frames.oop_by_default", false);
> +pref("layout.css.meta-viewport.enabled", true);

I would rather call this dom.meta-viewport.enabled as the viewport is specified via a meta tag in the DOM rather than via CSS. Also the code that reads and parses the tag is in dom/base/

::: dom/ipc/TabChild.cpp
@@ +298,1 @@
>      return false;

I don't think this is the right approach to be taking to implement this. There is code in HandlePossibleViewportChange that needs to run even if meta-viewport is disabled. Having to extract a EnsureDisplayPortIfNeeded is an example of this, but I think that doesn't cover all the cases.

I think a better fix would be to change nsDocument::GetViewportInfo to read this pref, and then if meta-viewport is disabled, return a nsViewportInfo that reflects whatever we want by default. I think you'll basically want to do the equivalent of the DisplayWidthHeight configuration (i.e. set the CSS viewport to the aDisplaySize).

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1092450-viewport-pref.patch

Comment 4

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

Comment on attachment 8516325 [details] [diff] [review]
bug1092450-viewport-pref.patch

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

LGTM, thanks!

::: modules/libpref/init/all.js
@@ +4389,5 @@
>  pref("intl.collation.mac.use_icu", true);
>  #endif
> +
> +// Enable meta-viewport support in remote APZ-enabled frames.
> +pref("dom.meta-viewport.enabled", false);

s/remote//

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8515405 [details] [diff] [review]
meta_viewport_changes.patch

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

I will defer to kats here. Just make sure that doesn't break the system app that doesn't use apz yet iirc.

Comment 6

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

Thanks. I don't think the new patch should break anything but it's worth doing a try push for B2G and Fennec to verify.

Note that Fennec has it's own meta-viewport parsing code which is why the new pref doesn't affect it. When bug 799585 lands that pref will need to be flipped on Fennec. Although depending on how the APZ work goes, bug 799585 may turn out to be INVALID and we might route the viewport stuff differently. Either way, I don't expect this patch to break anything there.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53e23b1b3642

Comment 8

[Carsten Book [:Tomcat]]

Hi, this caused a m1 test bustage on all platforms like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3803705&repo=mozilla-inbound and so i had to back this out

Comment 9

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

It looks like the meta-viewport tests just need to be disabled on the platforms where the meta-viewport support is disabled. i.e. everything except B2G. The run in comment 8 had the tests passing on B2G, as expected.

Comment 10

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

I updated the patch locally and will do a try push once the trees reopen.

Comment 12

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

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d388819ad8a

Comment 13

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

^ had font-inflation test failures, because font-inflation depends on meta-viewport as well [1]. New try push with meta-viewport enabled for the font-inflation tests:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=846d48e781cf

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=59f3a9bd26f0#11112

Comment 14

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

Try looked green, except for assertion failures from another patch I pushed along with it for the ride.

Landed on inbound with updates to the relevant tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ad8948bea9

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3ad8948bea9

Comment 16

[Aaron Train [:aaronmt]]

This broke Android viewport, see bug 1106255