Closed
Bug 1170944
Opened 10 years ago
Closed 9 years ago
SetVisible to Browser iframe when closing
Categories
(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: alive, Assigned: gduan)
References
Details
Attachments
(2 files)
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!
Flags: needinfo?(gduan)
Updated•9 years ago
|
Flags: needinfo?(timdream)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gduan
Flags: needinfo?(gduan)
Assignee | ||
Comment 1•9 years ago
|
||
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.
Attachment #8624020 -
Flags: review?(alegnadise+moz)
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8624020 [details] [review]
PR to master
Good work!
Attachment #8624020 -
Flags: review?(alegnadise+moz) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 4•9 years ago
|
||
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
Flags: needinfo?(timdream) → needinfo?(gduan)
Assignee | ||
Comment 5•9 years ago
|
||
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.
Flags: needinfo?(gduan)
Comment 6•9 years ago
|
||
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.
Flags: needinfo?(gduan)
Assignee | ||
Comment 7•9 years ago
|
||
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.
Flags: needinfo?(gduan)
Attachment #8625248 -
Flags: review?(timdream)
Assignee | ||
Comment 8•9 years ago
|
||
(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•9 years ago
|
||
Comment on attachment 8625248 [details] [review]
PR to master, remove attention_screen_opening
You should not land two pull requests in one bug #.
Attachment #8625248 -
Flags: review?(timdream) → review+
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
(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
Assignee | ||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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.
Flags: needinfo?(eperelman)
Comment 14•9 years ago
|
||
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
Flags: needinfo?(eperelman)
Comment 15•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•