1170944 - SetVisible to Browser iframe when closing

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

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

Gecko is trying to keep layer data in bug 1154231. Let's remove the workaround to give it a try in gaia side.

1. Moving call setVisible(false) from closed to closing.
2. Remove the settings db write in attention window manager to notify other apps that attention window is closing right now.

George could you help? Thanks!

Comment 1

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

Hi Alive,

I'm not sure if you can review it or not...
I've done what you said and removed commit of bug 1037880 and also have verified bug 1037880 would not be reproduced after removing.

Comment 2

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

Comment on attachment 8624020 [details] [review]
PR to master

Good work!

Comment 3

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks,
master: https://github.com/mozilla-b2g/gaia/commit/7626a37ee5e4ec63a5f383897b5226f7b14135ae

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Would you figure out what other things to do in bug 1175460, and dup the bug if there isn't anything left? I am still seeing the keyword in the code

$ grep -r attention_screen_opening apps/
apps//sharedtest/test/unit/stop_recording_event_test.js:  test('setting private.broadcast.attention_screen_opening triggers event',
apps//sharedtest/test/unit/stop_recording_event_test.js:           'private.broadcast.attention_screen_opening': true
apps//sharedtest/test/unit/stop_recording_event_test.js:           'private.broadcast.attention_screen_opening': false
apps//sharedtest/test/unit/stop_recording_event_test.js:      'private.broadcast.attention_screen_opening': true
apps//system/js/app_window_manager.js:     * private.broadcast.attention_screen_opening setting hack in

Comment 5

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master, remove attention_screen_opening

removing private.broadcast.attention_screen_opening from  app_window_manager.js and bug 1051172 is not reproduced.
removing private.broadcast.attention_screen_opening related tests from stop_recording_event_test.

Comment 6

[James Burke [:jrburke]]

After this commit, raptor filed bug 1176490 about a performance regression in email. Looking at the changeset between the two gaia hashes:

https://github.com/mozilla-b2g/gaia/compare/ad8a87ddd3d255f3...7626a37ee5e4ec63

This bug is the only gaia change. It could be something that changed in Gecko too, but thought I would ask first if this change is something that could affect the raptor numbers.

Comment 7

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8625248 [details] [review]
PR to master, remove attention_screen_opening

Hi Tim,
I've done comment 5.
could you kindly review this patch ? I will land it when all tests are green.

Comment 8

[(Inactive after June) George Duan [:gduan] [:喬智]]

(In reply to James Burke [:jrburke] from comment #6)
> After this commit, raptor filed bug 1176490 about a performance regression
> in email. Looking at the changeset between the two gaia hashes:
> 
> https://github.com/mozilla-b2g/gaia/compare/ad8a87ddd3d255f3...
> 7626a37ee5e4ec63
> 
> This bug is the only gaia change. It could be something that changed in
> Gecko too, but thought I would ask first if this change is something that
> could affect the raptor numbers.

I just run raptor on my local side and found it indeed cause the performance issue. still investigating how to fix it correctly...

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8625248 [details] [review]
PR to master, remove attention_screen_opening

You should not land two pull requests in one bug #.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to (Inactive after June) George Duan [:gduan] [:喬智] from comment #8)
> I just run raptor on my local side and found it indeed cause the performance
> issue. still investigating how to fix it correctly...

It's pretty interesting because this bug is about when the app closes, and I think Raptor is measuring app lunching. Maybe we changed the timing of when home screen app is being set to visible=hidden?

Anyway, George can help out on that. Thanks.

Comment 11

[(Inactive after June) George Duan [:gduan] [:喬智]]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #9)
> Comment on attachment 8625248 [details] [review]
> PR to master, remove attention_screen_opening
> 
> You should not land two pull requests in one bug #.

Thanks, file bug 1177006 for it and wait for tests passing

Comment 12

[(Inactive after June) George Duan [:gduan] [:喬智]]

As what Tim said, the measuring for launching time might be affected by homescreen's setVisible(false). I tried to put a delay for setVisible(false) when it's homescreen, and the performance became normal.

I thought of ways to fix it ... 
1. setVisible(false) only to homescreen in handle_closed and keep others in handle_closing
2. revert my patch
3. improve closing performance (not sure how, but still investigating)

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It looks like George did not finish comment 12 in time. I don't know if we should just accept that being the new normal or this need to be "fixed" with special timing to home screen window.

https://github.com/mozilla-b2g/gaia/pull/30641/files#diff-f70ce6c514b65fb6094cdde4d5382495R197

Eli, what do you think? I will file a bug for that if that needs to be fixed.

Comment 14

[:Eli Perelman]

I'm not really familiar with what the problem is, but I'll describe what Raptor does and maybe that will help. Raptor determines if there is a regression in launch time by measuring the delta between the Homescreen's appLaunch marker [1] and the application's visuallyLoaded marker, e.g. [2].

I'm not sure how setVisible plays into this problem, but if that method can somehow slow down the time it takes to get to visuallyLoaded in an app, then it will trigger a regression. Glancing at the code, I'm not really sure how that would happen.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/mozapp.js#L345
[2] https://github.com/mozilla-b2g/gaia/blob/9dfedd7d35da00fa9b08dcffc3ab4f47da54e4f0/apps/email/js/metrics.js#L45

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

This is just a guess. I'll need to do some tests to confirm.

It looks like bug 1137124. setVisible(false) causes priority changes and that blocks b2g main-thread for tens of milliseconds, which will in turn slow down visuallyLoaded because of some sync IPC. The change moved setVisible(false) to be called earlier might increase the chances for that to happen.