1180267 - Update Fennec code to use MobileViewportManager instead of the big pile of browser.js code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

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

Bug 1178847 adds a MobileViewportManager class to manage setting the CSS viewport. Switching Fennec over to using it turned out to be non-trivial so this is a follow-up bug to do that.

Comment 1

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

I think in order to do this I need to redo the dynamic toolbar stuff first. There's too much margins goop in browser.js interwoven into the viewport code.

Comment 2

[Wesley Johnston (:wesj)]

Matt Brubeck had code to make Fennec use the platform viewport code at one point. Maybe its still around?

Comment 3

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

Yeah, bug 799585. This bug encompasses that one and is a more comprehensive re-use of code.

Comment 5

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

Attachment: WIP

First cut. It depends on the patches in bug 1180295.

Comment 6

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

A try push with this stuff fails a lot of tests [1], largely because (as expected) the MobileViewportManager code is now setting a resolution based on the default CSS viewport. This wasn't really happening before because of the wacky environment android reftests run in (i.e. almost none of the browser.js code actually did anything in reftests - see bug 1156817).

I think that although the patch is probably fine for production, we need to run the reftests in a "desktop mode" viewport so that it doesn't get scaled, because most of the tests are not going to be very happy with that. Luckily we already have code for desktop-mode so I just need to enable it in reftests. I'm trying that now.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aadaca89d7c is a sampling of the types of failures

Comment 7

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

Attachment: Part 1 - Switch Fennec over to using MVM

Comment 8

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

Attachment: Part 2 - Don't be setting displayport margins in MVM for Fennec

Comment 9

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

Attachment: Part 3 - Fix desktop mode for Fennec with MVM

Comment 10

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

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9017734bd640

Comment 11

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

That try push has a B2G emulator R7 failure caused by part 1 of this series, because the prefs I fiddled with affect B2G as well. I did a slightly modification to that which passes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9017734bd640

Updated patches coming for review.

Comment 12

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

Err I meant https://treeherder.mozilla.org/#/jobs?repo=try&revision=df560bf8cef6

Comment 13

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

Attachment: Part 1 - Switch Fennec over to using MVM

Note that these patches apply on top of bug 1197592.

r? to snorp for the bulk of the patch.

f? to gbrown to look at the change to boostrap.js to make sure it's ok. The intent is to set a pref but only for Fennec reftests; adding it to reftest-preferences.js or runreftest.py seems to affect multiple platforms.

f? to botond also for the pref fiddling. I realize that it's apz.allow_zooming is probably not the best name since I'm now enabling it on Fennec without having APZ enabled, but after we migrate to APZ the name will make sense again :)

Comment 14

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

Attachment: Part 2 - Don't be setting displayport margins in MVM for Fennec

For now the displayport margins are still set in browser.js for Fennec, so we don't want to be setting them also in MVM. In particular this causes tests to fail because the way the tests are set up they're not expecting a displayport.

Comment 15

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

Attachment: Part 3 - Fix desktop mode for Fennec with MVM

The desktop mode is a Fennec-only feature, and this code in particular was not really used previously except for determining whether or not font inflation would happen. Now that we're using this code path to actually determine the CSS viewport for layout, I noticed that it's got a bug and was returning the wrong viewport in desktop mode.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8651753 [details] [diff] [review]
Part 1 - Switch Fennec over to using MVM

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

YESSSSS

Comment 17

[Geoff Brown [:gbrown]]

Comment on attachment 8651753 [details] [diff] [review]
Part 1 - Switch Fennec over to using MVM

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

Setting that pref in bootstrap.js is a little strange, isn't it? I would prefer you didn't do that unless absolutely necessary.

You should be able to set a pref only for Android reftests at http://hg.mozilla.org/mozilla-central/annotate/beb9cc29efb9/layout/tools/reftest/remotereftest.py#l386.

Comment 18

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

(In reply to Geoff Brown [:gbrown] from comment #17)
> You should be able to set a pref only for Android reftests at
> http://hg.mozilla.org/mozilla-central/annotate/beb9cc29efb9/layout/tools/
> reftest/remotereftest.py#l386.

Ah thanks. Updated try push to verify: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56b4da8940fa

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8651753 [details] [diff] [review]
Part 1 - Switch Fennec over to using MVM

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

Setting apz.allow_zooming on non-APZ Fennec for now seems OK to me.

Comment 20

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> (In reply to Geoff Brown [:gbrown] from comment #17)
> > You should be able to set a pref only for Android reftests at
> > http://hg.mozilla.org/mozilla-central/annotate/beb9cc29efb9/layout/tools/
> > reftest/remotereftest.py#l386.
> 
> Ah thanks. Updated try push to verify:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=56b4da8940fa

This is green. I'll land (with the update you suggested) once inbound reopens.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/636fa61990f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3862feb3978b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0720ea24b69a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/636fa61990f3
https://hg.mozilla.org/mozilla-central/rev/3862feb3978b
https://hg.mozilla.org/mozilla-central/rev/0720ea24b69a