962902 - Duplicate blob url wastes 125k in the system app.

Status: RESOLVED FIXED

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

Description

[[:fabrice] Fabrice Desré]

Both bootstrap.js and lockscreen.js are creating blobs and they show up in memory reports.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: wallpaper.patch

Centralize the wallpaper setting listener to create a single blob and redispatch the url using an event.

Comment 2

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

Comment on attachment 8364086 [details] [diff] [review]
wallpaper.patch

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

I don't see the new wallpaper.js ?

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: gaia PR

Sorry for the missing file... I sent a PR to get travis results too.

Comment 4

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

Comment on attachment 8364812 [details] [review]
gaia PR

See github comment for v1.4

Comment 5

[[:fabrice] Fabrice Desré]

damn, I totally forgot about this one. If someone has time to make it move forward, please do!

Comment 6

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

In my long queue...

Comment 7

[[:fabrice] Fabrice Desré]

Alive, maybe someone else in your team could take that?

Comment 8

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

John, could you steal this bug and finish the patch? Shouldn't be too hard.

Comment 9

[John Lu [:mnjul][Please NI or I'll miss]]

Yeah ok, lemme look into this.

Comment 10

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Screenshot from 2014-05-26 11:12:54.png

Hi Fabrice,

While your patch seems to work with real devices, it appears that it breaks the emulation on nightly. When I run your branch for testing gaia on (the latest) nightly, all I get is a blank screen (with sim & battery icons on the status bar, no more than that). Check out the attachment for screenshot. Could you confirm this? And if yes, could you see if you can fix that? Thanks a lot!

Comment 11

[[:fabrice] Fabrice Desré]

John, I never tested that on master, only on 1.3t. Which gaia branch are you using?

Comment 12

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Fabrice,

I tested your patch both on 1.3t (latest 1.3t on github with your two commits cherry-picked) and on master (again, commits are cherry-picked, but some conflicts needed to be resolved, please check: https://github.com/mnjul/gaia/tree/bug_962902_duplicate_blob/fabricedesre_original ). In both scenarios, the emulation on Nightly broke (as described in my comment 10 above) after your two commits were cherry-picked in.

To clarify: if we only want to land this patch on 1.3t and if 1.3t is never meant to work in Nightly emulation (big disclaimer: this is something I don't know, we might want to NI others to confirm/deny), then we can proceed. Otherwise, if we also want to land the patch on master/1.4, then based on my current observation of how it breaks Nightly emulation, I would say we need to solve the black screen problem before we give the patch a green light for safe landing. What do you think? Thanks a lot.

Comment 13

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

Well, I don't see the obvious reason to break desktop but work on device. Could you just steal the bug and do investigation?

Comment 14

[[:fabrice] Fabrice Desré]

What Alive said, it's all your John!

Comment 15

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: New Patch

Hi everyone,

The finished patch is here.
The blank screen issue as reported in comment 12 has been resolved. It was caused by the situation that if the new wallpaper.js was executed before lockscreen's initialization (such as during device boot), then lockscreen's wallpaper-change event listener would not be triggered ( as in https://github.com/mnjul/gaia/blob/cd2d8b502ce34d03cb07c88e7b67ff6c7a59d2af/apps/system/js/lockscreen.js#L385-L388 ) and the lockscreen would have its uninit class remained -- thus it looked blank.

The execution/initialization of wallpaper.js (now renamed to wallpaper_manager.js) is now controlled in bootstrap.js.

Comment 16

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

Comment on attachment 8430700 [details] [review]
New Patch

Well, since we are not rushing to land, it's a nice practice to write unit test for wallpaper manager.

Comment 17

[[:fabrice] Fabrice Desré]

We want that to land (at least on 1.3t) this week.

Comment 18

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

(In reply to Fabrice Desré [:fabrice] from comment #17)
> We want that to land (at least on 1.3t) this week.

Let's land the patch without tests if there's no conflict.

Comment 19

[John Lu [:mnjul][Please NI or I'll miss]]

I modified the patch to fix a minor issue Alive commented on my github PR. Its Travis is running now.

Comment 20

[[:fabrice] Fabrice Desré]

master: https://github.com/mozilla-b2g/gaia/commit/053de51fcb7d1cd0553d3dd092e1974fda62a1f3

Comment 21

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: 1.3t uplift

Hi John, I had some trouble uplifting your patch to 1.3t. Can you take a look at this pull request and make sure everything looks right?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures.
Master: https://github.com/mozilla-b2g/gaia/commit/e4ab23c961f3364af962617174b0b6c8cb832db2

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

Comment 23

[Joe Cheng [:jcheng] (please needinfo)]

Hi John, can you please provide some updateS? Thanks

Comment 24

[John Lu [:mnjul][Please NI or I'll miss]]

Yep, I found out why it didn't pass test in tbpl. I should be able to issue a PR tomorrow (Thursday) and  hopefully if things are OK we can land it soon

@michael: you might want to issue a new PR after my new fix lands. From what I can see in your old PR's Travis report, gjslint failed because of some unsupported JSDoc attributes. I will take care of it with my new fix too.

Comment 25

[John Lu [:mnjul][Please NI or I'll miss]]

Master: https://github.com/mozilla-b2g/gaia/commit/8bb67b73078ef056017582f57d8b25220804d615

@michael: Please try uplifting to v1.3t again. To avoid gjslint errors:
1. remove "/** @exports WallpaperManager */" from apps/system/js/wallpaper_manager.js around line 58 ( https://github.com/mozilla-b2g/gaia/commit/8bb67b73078ef056017582f57d8b25220804d615#diff-a876f04c62310f55eaa7ac7e28f89ba7R58 )
2. change "@returns" to "@return in apps/system/js/wallpaper_manager.js around line 51 ( https://github.com/mozilla-b2g/gaia/commit/8bb67b73078ef056017582f57d8b25220804d615#diff-a876f04c62310f55eaa7ac7e28f89ba7R51 )

I will help you with uplifting issues if any.

Comment 26

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

I recommend you to make a 1.3t patch instead of hand writing comments.

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #25)
> Master:
> https://github.com/mozilla-b2g/gaia/commit/
> 8bb67b73078ef056017582f57d8b25220804d615
> 
> @michael: Please try uplifting to v1.3t again. To avoid gjslint errors:
> 1. remove "/** @exports WallpaperManager */" from
> apps/system/js/wallpaper_manager.js around line 58 (
> https://github.com/mozilla-b2g/gaia/commit/
> 8bb67b73078ef056017582f57d8b25220804d615#diff-
> a876f04c62310f55eaa7ac7e28f89ba7R58 )
> 2. change "@returns" to "@return in apps/system/js/wallpaper_manager.js
> around line 51 (
> https://github.com/mozilla-b2g/gaia/commit/
> 8bb67b73078ef056017582f57d8b25220804d615#diff-
> a876f04c62310f55eaa7ac7e28f89ba7R51 )
> 
> I will help you with uplifting issues if any.

Comment 27

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: 1.3t uplift (again)

Hi Michael,

Here's the new 1.3t uplift (Travis-passing). Please review and merge :)

Comment 28

[Michael Henretty [:mikehenrty][:mhenretty]]

Thanks John!

1.3t: https://github.com/mozilla-b2g/gaia/commit/d44d2f9020e3729dc7e12fbdfaee876d090d5605

Comment 29

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8435658 [details] [review]
1.3t uplift (again)

For future reference, you generally don't need another review for an uplift, you just need approval (which in this case came with the 1.3T blocking flag). But it really helped that you created the Pull Request for me :)