961047 - Ensure all mozbrowser iframes use APZ when APZ is enabled.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Here are the iframes that are created with mozbrowser=true. Some of these don't set mozasyncpanzoom=true and have caused problems (see dep bugs). We should make sure all of these set mozasyncpanzoom=true when APZ is enabled.

kats@kgupta-air apps$ rfind "setAttribute('mozbrowser'"
./browser/js/browser.js:      iframe.setAttribute('mozbrowser', true);
./calendar/js/oauth_window.js:      iframe.setAttribute('mozbrowser', true);
./system/js/browser_frame.js:    browser.setAttribute('mozbrowser', 'true');
./system/js/cost_control.js:    widgetFrame.setAttribute('mozbrowser', true);
./system/js/identity.js:          frame.setAttribute('mozbrowser', 'true');
./system/js/keyboard_manager.js:    keyboard.setAttribute('mozbrowser', 'true');
./system/js/payment.js:        frame.setAttribute('mozbrowser', 'true');
./system/js/payment.js:        frame.setAttribute('mozbrowser', 'true');

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> Here are the iframes that are created with mozbrowser=true. Some of these
> don't set mozasyncpanzoom=true and have caused problems (see dep bugs). We
> should make sure all of these set mozasyncpanzoom=true when APZ is enabled.
> 
> kats@kgupta-air apps$ rfind "setAttribute('mozbrowser'"
> ./browser/js/browser.js:      iframe.setAttribute('mozbrowser', true);
> ./calendar/js/oauth_window.js:      iframe.setAttribute('mozbrowser', true);
> ./system/js/browser_frame.js:    browser.setAttribute('mozbrowser', 'true');
> ./system/js/cost_control.js:    widgetFrame.setAttribute('mozbrowser', true);
> ./system/js/identity.js:          frame.setAttribute('mozbrowser', 'true');
> ./system/js/keyboard_manager.js:    keyboard.setAttribute('mozbrowser',
> 'true');
> ./system/js/payment.js:        frame.setAttribute('mozbrowser', 'true');
> ./system/js/payment.js:        frame.setAttribute('mozbrowser', 'true');

I was thinking of moving the code from Gaia to Gecko and using a pref. That would ensure that all mozbrowser iframes use apzc.

What do you think ?

Comment 2

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

Sounds good to me. I just want to make sure we retain the ability to turn it off easily in case we run into problem late in 1.3's release cycle.

Do you want to take this bug then?

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Sounds good to me. I just want to make sure we retain the ability to turn it
> off easily in case we run into problem late in 1.3's release cycle.
> 

We can bind a mozSetting to a Gecko pref in b2g/chrome/content/settings.js so we can keep the Gaia setting to easily switch it on/off.

> Do you want to take this bug then?

Will try to provide a patch.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP activatesyncpanzoom.bydefault.patch

Completely untested Gecko patch.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP activateasyncpanzoom.forall.patch

Completely untested Gaia patch

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8361803 [details] [diff] [review]
WIP activatesyncpanzoom.bydefault.patch

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

After testing it seems to work as expected.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8361809 [details] [diff] [review]
WIP activateasyncpanzoom.forall.patch

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

Alive this is a followup of the last patch I have landed. This one remove custom set up of mozasyncpanzoom from Gaia. I kept the setting as a way to disable the platform feature if needed.

Comment 8

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

Comment on attachment 8361803 [details] [diff] [review]
WIP activatesyncpanzoom.bydefault.patch

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

Technically I'm not qualified to review this either but it looks good to me. You may want to get a second review from somebody else.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8361803 [details] [diff] [review]
WIP activatesyncpanzoom.bydefault.patch

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

small nit, but r=me too.

::: content/base/src/nsFrameLoader.cpp
@@ +2060,2 @@
>      scrollingBehavior = ASYNC_PAN_ZOOM;
>    }

ScrollingBehavior scrollingBehavior = Preferences::GetBool("dom.browser_frames.useAsyncPanZoom", false) ? ASYNC_PAN_ZOOM : DEFAULT_SCROLLING

Comment 10

[Preeti Raghunath(:Preeti)]

1.3+ depends to QC bug: 959198

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

After thinking about that again I realize that this will enable/disable asyncpanzoom for the browser app too :/

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8361809 [details] [diff] [review]
WIP activateasyncpanzoom.forall.patch

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

Removing review until I found a smarter way to not break the browser app.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> Comment on attachment 8361809 [details] [diff] [review]
> WIP activateasyncpanzoom.forall.patch
> 
> Review of attachment 8361809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Removing review until I found a smarter way to not break the browser app.

for v1.4 check AppWindow.prototype.isOOP to see it's browser app or not. (only browser app is not OOP)
for v1.3 .... life would find its way.
Or did I misunderstand something?

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> > Comment on attachment 8361809 [details] [diff] [review]
> > WIP activateasyncpanzoom.forall.patch
> > 
> > Review of attachment 8361809 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Removing review until I found a smarter way to not break the browser app.
> 
> for v1.4 check AppWindow.prototype.isOOP to see it's browser app or not.
> (only browser app is not OOP)
> for v1.3 .... life would find its way.
> Or did I misunderstand something?

My plan was mostly to remove the ability to activate/deactivate async pan zoom from Gaia. So I probably need to find a gecko magic to make sure the pref turn on/off asyncpanzoom with the right things.

On the other hand it will likely ends up as a dirty hack in the frame loader. Sounds dirty :(

Comment 15

[Kai-Chih Hu [:khu]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #14)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> > > Comment on attachment 8361809 [details] [diff] [review]
> > > WIP activateasyncpanzoom.forall.patch
> > > 
> > > Review of attachment 8361809 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Removing review until I found a smarter way to not break the browser app.
> > 
> > for v1.4 check AppWindow.prototype.isOOP to see it's browser app or not.
> > (only browser app is not OOP)
> > for v1.3 .... life would find its way.
> > Or did I misunderstand something?
> 
> My plan was mostly to remove the ability to activate/deactivate async pan
> zoom from Gaia. So I probably need to find a gecko magic to make sure the
> pref turn on/off asyncpanzoom with the right things.
> 
> On the other hand it will likely ends up as a dirty hack in the frame
> loader. Sounds dirty :(

Vivien, looks like you're working on this bug. Do you think that you can be the assignee for this bug? Then, we will know this bug is on-going? Thanks. 

Also, you mentioned about "gecko magic". Does it mean we need to find someone from Gecko to do it or provide some suggestions? Thanks.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Hu [:khu] from comment #15)

> Also, you mentioned about "gecko magic". Does it mean we need to find
> someone from Gecko to do it or provide some suggestions? Thanks.

We just need to find the right heuristic. No need for a real Gecko hacker :)

Comment 17

[Kai-Chih Hu [:khu]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> (In reply to Kevin Hu [:khu] from comment #15)
> 
> > Also, you mentioned about "gecko magic". Does it mean we need to find
> > someone from Gecko to do it or provide some suggestions? Thanks.
> 
> We just need to find the right heuristic. No need for a real Gecko hacker :)

Thank you, Vivien.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

kats, I know you want a way to quickly fallback if we decide to disable APZC at the last minute.

The issue I have with this patch is the following:
 - mozbrowser iframes can be loaded into the system app, where in a pre-apzc for all time, apzc was enabled programmatically for some iframes.
 - mozbrowser iframes can be loaded into the browser app, where they always need to run APZC

If I land the Gecko patch, then all mozbrowser iframes from both the system app and the browser app will support APZC or not. No fine grained control anymore.

It means that if we want to fallback quickly we will have to backout the gecko patch. Is it fine for you?

If so we probably have nothing to do on Gaia as of today, and one day we will all Gaia code to control APZC.

What do you think ?

Comment 19

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

Can we do something like this in the FrameLoader code?

if (pref("dom.browser_frames.useAsyncPanZoom") || attr("mozasyncpanzoom")) {
 // use APZC
}

and then only keep the "mozasyncpanzoom" attribute in the gaia browser tabs? That way if we need to fallback we can just set the dom.browser_frames.useAsyncPanZoom attribute to false, which will turn it off in everything except the browser tabs.

Comment 20

[Preeti Raghunath(:Preeti)]

Vivien

ni so its on your radar.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> Can we do something like this in the FrameLoader code?
> 
> if (pref("dom.browser_frames.useAsyncPanZoom") || attr("mozasyncpanzoom")) {
>  // use APZC
> }
>

Not sure why I didn't though about that. I was probably obsessed by the removal of mozasyncpanzoom.

thanks :)

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: activatesyncpanzoom.bydefault.patch

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: activate.mozasyncpanzoom.patch

Gaia clean up. I just need a stamp from Alive or Fabrice.

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Fabrice,

I was under the feeling that there is an urgent need for that. If you got a chance to review it during the night (mine), feel free to land the Gecko part. The Gaia part is really not needed for the feature to work. This is just some cleanup.

Comment 25

[Preeti Raghunath(:Preeti)]

Yes this is urgent for 1.3 QC CS.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/b2g-inbound/rev/47c7cd88eee1

I will wait a bit for the Gecko part to hit m-c before merging the Gaia part. The Gecko part should be enough for the QC CS.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for test failures: https://hg.mozilla.org/integration/b2g-inbound/rev/0cad6b13997a

https://tbpl.mozilla.org/php/getParsedLog.php?id=33427208&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33429581&tree=B2g-Inbound

Comment 30

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8363625 [details] [diff] [review]
activate.mozasyncpanzoom.patch

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

r+ if you remove the MockSettingsListener stuff in apps/system/test/unit/browser_frame_test.js and make it green.

Comment 31

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Wes Kocher (:KWierso) from comment #29)
> I had to back this out for test failures:
> https://hg.mozilla.org/integration/b2g-inbound/rev/0cad6b13997a
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33427208&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33429581&tree=B2g-Inbound

I have sent something to try to see if it fixes the first set of failure.
Still investigating the second cause of failures, which is not obvious.

Comment 32

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Sigh. The tryserver is failing to build... https://tbpl.mozilla.org/php/getParsedLog.php?id=33463283&tree=Try&full=1

Comment 33

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #32)
> Sigh. The tryserver is failing to build...
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33463283&tree=Try&full=1

I really can't run the try server for some reasons. I landed this who should disable asyncPanZoom in the test stuff by default if I have understood correctly how tests are ran.

https://hg.mozilla.org/integration/b2g-inbound/rev/a20fc7c8df1f

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #33)
> I really can't run the try server for some reasons. I landed this who should
> disable asyncPanZoom in the test stuff by default if I have understood
> correctly how tests are ran.
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/a20fc7c8df1f

Presumably this starts off the pref disabled and then as soon as gaia loads it sets it to true because of the force-enable?

Comment 36

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #35)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #33)
> > I really can't run the try server for some reasons. I landed this who should
> > disable asyncPanZoom in the test stuff by default if I have understood
> > correctly how tests are ran.
> > 
> > https://hg.mozilla.org/integration/b2g-inbound/rev/a20fc7c8df1f
> 
> Presumably this starts off the pref disabled and then as soon as gaia loads
> it sets it to true because of the force-enable?

Yes. It will also stay to true afterward. It may be a bit racy for the first time ever the device is turned on. I would prefer to set those prefs to true by default but we need to fix the 2 tests failures before.

Since I was unable to have a running build on try I choose to accelerate thing by making it |false| by default since the test runner is basically its own system app that does not use the Gaia settings.

Also the 2 tests failures does not seems related to that particular patch but seems like bugs that were lurking in the source.

Comment 37

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

Sounds reasonable to me. I'll file a follow-up bug for the tests and turning it on by default.

Comment 38

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

Comment on attachment 8363625 [details] [diff] [review]
activate.mozasyncpanzoom.patch

The gaia bit needs to land still.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f10161f83105

Please land the Gaia patch on both master and v1.3 and set status-b2g-v1.3 and status-b2g-v1.4 to fixed.

Comment 40

[:gerard-majax]

I fear bug 963584 might be an example of the race condition exposed by :vingtetun in comment 36 :(

Comment 41

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/362b3642076f176c2b780cd512b4110ce20e7eee
https://github.com/mozilla-b2g/gaia/commit/ffd4a72a6031461fe56cc16c89ca480a0efc1383

Comment 42

[Ryan VanderMeulen [:RyanVM]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #41)
> https://github.com/mozilla-b2g/gaia/commit/
> 362b3642076f176c2b780cd512b4110ce20e7eee
> https://github.com/mozilla-b2g/gaia/commit/
> ffd4a72a6031461fe56cc16c89ca480a0efc1383

This blew up B2G mochitests on TBPL. Reverted.
Master: b0cc15bf0d4369dad5c2806f66000c47b84fffba
v1.3: cd0647684b057a9a8d02b362fa72fa41aa70bacf

https://tbpl.mozilla.org/php/getParsedLog.php?id=33546421&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33546648&tree=B2g-Inbound

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Dammit, this got caught up with another busted commit. Re-landed, sorry for the churn :(

Master: 0534c383b4a219d4874717e905749cb8290aa209
v1.3: af4a42045a2128fced150ac3fcb99680ca94a4e9

Comment 44

[[:fabrice] Fabrice Desré]

Hiding the tests failures was really not a good idea. The race is real and is preventing us to land the Nuwa patches (see the movie at https://bugzilla.mozilla.org/show_bug.cgi?id=950266#c49).

I'm investigating but I may back out this one.