Closed Bug 961047 Opened 6 years ago Closed 6 years ago

Ensure all mozbrowser iframes use APZ when APZ is enabled.

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- unaffected
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: kats, Assigned: vingtetun)

References

Details

(Keywords: regression, smoketest)

Attachments

(2 files, 2 obsolete files)

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');
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 ?
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?
Blocks: 942267
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #8361803 - Flags: review?(bugmail.mozilla)
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.
Attachment #8361809 - Flags: review?(alive)
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.
Attachment #8361803 - Flags: review?(bugmail.mozilla) → review+
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
1.3+ depends to QC bug: 959198
blocking-b2g: --- → 1.3+
After thinking about that again I realize that this will enable/disable asyncpanzoom for the browser app too :/
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.
Attachment #8361809 - Flags: review?(alive)
Blocks: 959198
No longer depends on: 959198
(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?
Blocks: 958230
No longer depends on: 958230
Blocks: 959425
No longer depends on: 959425
(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 :(
(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.
Flags: needinfo?(21)
(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 :)
Flags: needinfo?(21)
(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.
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 ?
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
Vivien

ni so its on your radar.
Flags: needinfo?(21)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :)
Flags: needinfo?(21)
Attachment #8361803 - Attachment is obsolete: true
Attachment #8363623 - Flags: review?(fabrice)
Attachment #8363623 - Flags: review?(bugmail.mozilla)
Gaia clean up. I just need a stamp from Alive or Fabrice.
Attachment #8361809 - Attachment is obsolete: true
Attachment #8363625 - Flags: review?(fabrice)
Attachment #8363625 - Flags: review?(alive)
Attachment #8363623 - Flags: review?(bugmail.mozilla) → review+
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.
Yes this is urgent for 1.3 QC CS.
No longer blocks: 959425
Duplicate of this bug: 959425
Attachment #8363623 - Flags: review?(fabrice) → review+
Attachment #8363625 - Flags: review?(fabrice) → review+
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.
Status: NEW → ASSIGNED
Component: Gaia::System → Panning and Zooming
Product: Firefox OS → Core
Target Milestone: --- → mozilla28
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.
Attachment #8363625 - Flags: review?(alive) → review+
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/a20fc7c8df1f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
(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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Sounds reasonable to me. I'll file a follow-up bug for the tests and turning it on by default.
Comment on attachment 8363625 [details] [diff] [review]
activate.mozasyncpanzoom.patch

The gaia bit needs to land still.
Attachment #8363625 - Flags: checkin?(21)
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.
I fear bug 963584 might be an example of the race condition exposed by :vingtetun in comment 36 :(
(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
Dammit, this got caught up with another busted commit. Re-landed, sorry for the churn :(

Master: 0534c383b4a219d4874717e905749cb8290aa209
v1.3: af4a42045a2128fced150ac3fcb99680ca94a4e9
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.
No longer blocks: 963278
Depends on: 963278
You need to log in before you can comment on or make changes to this bug.