Homescreen fixes #4283

Merged
merged 25 commits into from Sep 17, 2012

Projects

None yet

6 participants

Contributor

Save icons for newly installed applications and generate a shadow for homescreen icons.

Contributor

@crdlc review?

Contributor
crdlc commented Sep 4, 2012

ok

@crdlc crdlc commented on the diff Sep 4, 2012
apps/homescreen/js/grid.js
appsInDB = appsInDB.concat(apps);
+
+ for (var app in apps) {
+ Applications.cacheIcon(apps[app].origin, apps[app].icon);
crdlc
crdlc Sep 4, 2012 Contributor

Why is this done whenever the homescreen starts?

vingtetun
vingtetun Sep 4, 2012 Contributor

This is to bypass the Icon code that normally reads the image from the manifest and tries to load it with a URL instead of the data: url version. So instead of trying to render the icons from a remote source it uses the locale database result.

@crdlc crdlc commented on the diff Sep 4, 2012
apps/homescreen/js/appmanager.js
@@ -131,8 +157,6 @@ var Applications = (function() {
return app;
}
- // XXX We are affected by the port!
-
// Trailing '/'
var trimmedOrigin = origin.slice(0, origin.length - 1);
crdlc
crdlc Sep 4, 2012 Contributor

we have always the '/' character at the end of the origin URI?

vingtetun
vingtetun Sep 4, 2012 Contributor

Not always but in this case this is likley that the code that is a few lines above return |app|.

@crdlc crdlc commented on the diff Sep 4, 2012
apps/homescreen/style/dock.css
pointer-events: none;
}
-/* icon */
crdlc
crdlc Sep 4, 2012 Contributor

Could be defined the size of the canvas in CSS? I don't know it's only a question. In any way as far as I remember the icon size should be 60px when UX defined the layout and now you write 68 for the canvas? is it correct? thanks Vivien

vingtetun
vingtetun Sep 4, 2012 Contributor

The latest icons I receive are 64x64 icons and we need some additional space for the drop shadow. @patrykdesign Any opinion on that?

crdlc
crdlc Sep 4, 2012 Contributor

@sergivila, I was searching the wireframe in Dropbox OWD_Moz_Share which defines the app grid (in pixels) but I cannot find, do you know where it is? thanks Sergi (AFAIR it was in our Jira)

sergivila
sergivila Sep 4, 2012 Contributor

The grid was never designed in a wireframe. I created some guidelines for the homescreen, but i'm afraid they're quite old. Originally the proposed size for the icons was 60x60px including the shadow. You can find a screenshot here explaining how the home screen grid was originally built: https://www.dropbox.com/s/w1rwbf9w1fc1g7c/homescreen_guides.png

The reason behind using that size was because we were designing over a 5px grid, so it was strange to use the 64px size, and using 65px left a really small horizontal space between icons.

Anyway this is quite old, and the newest icons have been redesigned by @patrykdesign so maybe he can help you on clarifying this.

dcoloma
dcoloma Sep 4, 2012

@sergivila @patrykdesign @authoritaire could you please check if the icons are OK with current size. Do we really need to generate the shadow via software when it could be part of the image itself?

sergivila
sergivila Sep 4, 2012 Contributor

I suggest to create the shadow in CSS if it doesn't have a huge impact in performance. It's the best way to ensure we always have a shadow applied to the icon. Regarding the icon size, i'm sure @patrykdesign can help you.

vingtetun
vingtetun Sep 6, 2012 Contributor

We cannot create it with CSS sadly. CSS will make a shadow on the box that contains the image, not the content itself. That's the main reason why this code use canvas.

vingtetun
vingtetun Sep 6, 2012 Contributor

@dcoloma It could not be part of the image itself if you want to always have a shadown on every icons on the homescreen. The icons on the marketplace will be of all different types :)

@crdlc crdlc and 1 other commented on an outdated diff Sep 4, 2012
apps/homescreen/js/page.js
- img.onerror = function imgError() {
+ function generateShadow() {
crdlc
crdlc Sep 4, 2012 Contributor

What happen when the image has predefined the shadow? The result could be ugly

vingtetun
vingtetun Sep 4, 2012 Contributor

Tell that to the UI folks :)

@crdlc crdlc commented on an outdated diff Sep 4, 2012
apps/homescreen/js/page.js
- img.onerror = function imgError() {
+ function generateShadow() {
+ var ctx = canvas.getContext('2d');
+ ctx.shadowColor = 'rgba(0,0,0,0.8)';
+ ctx.shadowBlur = 2;
+ ctx.shadowOffsetY = 2;
+
+ var width = Math.min(img.width, canvas.width - 4);
+ var height = Math.min(img.width, canvas.height - 4);
+ ctx.drawImage(img,
+ (canvas.width - width) / 2,
+ (canvas.height - height) / 2,
+ width, height);
+ ctx.fill();
crdlc
crdlc Sep 4, 2012 Contributor

How is it rendered when the image is rounded?

crdlc
crdlc Sep 4, 2012 Contributor

it's ok

crdlc
crdlc Sep 4, 2012 Contributor

I check and it's OK

@crdlc crdlc commented on the diff Sep 4, 2012
apps/homescreen/js/page.js
@@ -50,13 +50,40 @@ Icon.prototype = {
var icon = this.icon = document.createElement('div');
// Image
- var img = document.createElement('img');
+ var canvas = document.createElement('canvas');
crdlc
crdlc Sep 4, 2012 Contributor

Are you checked the framerate using a canvas with shadow? Probably it is the same but I want to ask for it, thanks

Contributor
crdlc commented Sep 4, 2012

I'm going to checkout your branch but I cannot make install-gaia in my Mac, I have problems :( I need more time to check it, sorry

Contributor
crdlc commented Sep 4, 2012

Please review the edit mode because it's broken with this pr. Maybe you can keep the icon as an image instead of canvas by means of canvas.toDataURL() so everything should work as previously or if you keep the icon as a canvas you should review the edit mode in CSS basically

Contributor
crdlc commented Sep 4, 2012

Finally, please review this method [1] because it should use the mechanism to cache icons than when an app is installed. Thanks

[1] https://github.com/vingtetun/gaia/blob/homescreen-debug/apps/homescreen/js/appmanager.js#L317

Contributor

@crdlc Thanks for the review. I will address/answer your concern a bit after. I have a lot of crashes with this code while panning the homescreen so I will try to fix that first!

Contributor

@cgjones This pull request replaces |img| tag by |canvas| tag in order to draw a drop shadow below application icons. This result in a lof of crahes:

D/memalloc( 2010): /dev/pmem: Freeing buffer base:0x49546000 size:163840 offset:8056832 fd:995
D/memalloc( 2010): /dev/pmem: Freeing buffer base:0x494fa000 size:163840 offset:7745536 fd:955
D/memalloc( 2010): /dev/pmem: Allocated buffer base:0x48d97000 size:36864 offset:7745536 fd:955
D/memalloc( 2010): /dev/pmem: Allocated buffer base:0x48d97000 size:163840 offset:8056832 fd:995
D/memalloc( 2010): /dev/pmem: Allocated buffer base:0x48d97000 size:36864 offset:7782400 fd:1000
D/memalloc( 2010): /dev/pmem: Allocated buffer base:0x48d97000 size:36864 offset:7819264 fd:1005
D/memalloc( 2010): /dev/pmem: Allocated buffer base:0x48d97000 size:36864 offset:7856128 fd:1010
D/memalloc( 2010): /dev/pmem: Allocated buffer base:0x48d97000 size:36864 offset:8220672 fd:1015
D/memalloc( 2010): /dev/pmem: Allocated buffer base:0x48d97000 size:36864 offset:8257536 fd:1020
E/memalloc( 2010): /dev/pmem: Failed to initialize pmem sub-heap: -1
W/memalloc( 2010): Falling back to ashmem
E/memalloc( 2010): couldn't create ashmem (Too many open files)
E/msm7627a.gralloc( 2010): gralloc failed err=Too many open files
W/GraphicBufferAllocator( 2010): alloc(68, 68, 1, 00000133, ...) failed -24 (Too many open files)
E/Gecko ( 2010): ShmemAndroid::Create():open: Too many open files (24)
I/Gecko ( 2010): [Parent 2010] ###!!! ABORT: creating CanvasLayer back buffer failed!: file /home/vivien/Devel/B2G_OTORO_ICS/B2G/gecko/gfx/layers/basic/BasicCanvasLayer.cpp, line 440
E/profiler( 2129): Registering start signal

Could it be related to Canvas?

Contributor

Seems like the crashes are not related. My build is unstable. @crdlc Are you ok to land?

Contributor
crdlc commented Sep 10, 2012

I guess this is not ready to land because the edit mode is broken with this pr

Contributor

@crdlc Thanks. I will look at it.

Contributor
crdlc commented Sep 11, 2012

please review my pr to your branch, there you can view some fixes for edit mode

Contributor

@crdlc Updated! I will fix the bookmark case in a followup.

Contributor
crdlc commented Sep 12, 2012

I've tested your branch and doing drag&drop sometimes icons disappear and b2g crashes (page with 16 icons). Furthermore, the re-arranged is less smooth than with images. Have you noticed this behavior?

Contributor
crdlc commented Sep 12, 2012

Logs say several times:

E/msm7627a.gralloc(15435): Could not mmap handle 0x446076a0, fd=849 (Out of memory)
E/msm7627a.gralloc(15435): gralloc_register_buffer: gralloc_map failed
W/GraphicBufferMapper(15435): registerBuffer(0x446076a0) failed -12 (Out of memory)
E/memalloc(15435): /dev/pmem: Failed to map buffer size:5189632 offset:5152768 fd:849 Error: Out of memory
E/msm7627a.gralloc(15435): Could not mmap handle 0x446076a0, fd=849 (Out of memory)
E/libgenlock(15435): perform_lock_unlock_operation: GENLOCK_IOC_LOCK failed (lockType0x1, err=Bad file number fd=849)
E/msm7627a.gralloc(15435): gralloc_lock: genlock_lock_buffer (lockType=0x2) failed
W/GraphicBufferMapper(15435): lock(...) failed -22 (Invalid argument)

Contributor

On 12/09/2012 13:17, Cristian Rodriguez wrote:

I've tested your branch and doing drag&drop sometimes icons disappear
and b2g crashes (page with 16 icons).

I've observed those crashes too. Icons starts to disappear and the
homescreen crash soon after that. This is a platform bug that needs to
be investigate. The crash reporter will be hooked up soon so I won't
worry too much about that.

Furthermore, the re-arranged is less smooth than with images. Have you
noticed this behavior?

I have not but I trust you. I would like to optimize panning/dragging
later. I have dome some experiments and removing all the intermediary
JavaScript steps while doing a regular pan save us more than 10 fps. I
think this can be the same for this particular case, optimizing dragging
by removing some of the intermediary steps. But this belongs to a
different bug imho.


Reply to this email directly or view it on GitHub
#4283 (comment).

Contributor

hold on. I would like to try something before landing.

Contributor
crdlc commented Sep 12, 2012

perfect

Contributor

I wanted to play with the filereader to save the data url but it require too much changes for this pull request. I will open 2 followups if that's fine for you?

Contributor
crdlc commented Sep 13, 2012

question, saving the data url would be a forbidden operation because images come from other origins, but I don't know if I'm wrong or when you speak about playing with the filereader is another thing.. in any way, you can split into two pr, no problem.

Contributor

I'm not sure too of the side efffect of the FileReader. But I would like to experiment in a different PR. Merge?

Contributor
crdlc commented Sep 13, 2012

I don't have problem with merging BUT we have to remember what there are crashes in b2g translating canvases

Contributor
crdlc commented Sep 13, 2012

Did you fix the bookmark case? thanks

dcoloma commented Sep 13, 2012

I would like also some feedback from the UX team about what happens if the app icons already have a shadow and we apply another one to them. @patrykdesign @jcarpenter?

Before adding a PR that gets (temporarily I know) worse performance and some crashes, I would like to be really sure UX is OK with the resultant UX.

Contributor

cc'ing @PeterLa for feedback also.

I would like also some feedback from the UX team about what happens if the app icons already have a shadow and we apply another one to them. @patrykdesign @jcarpenter?

That's a very fair point. IMO the doubling up should be largely imperceptible, especially when set against a background image. At worst, the icon would simply appear to have a slightly darker shadow.

Before adding a PR that gets (temporarily I know) worse performance and some crashes, I would like to be really sure UX is OK with the resultant UX.

We'd like to have it, but not at the expense of killing FPS. If the performance consequences are too severe, then we can retract it. FWIW, @PeterLa mocked up examples here. The shadow helps the icons pop from their background.

Contributor

"... then we can retract it"

Heh, easy for me to say, right? Let us know how risky this is. If we think we're going to drop 50% FPS, and that it would take 1 Vivien day to revert the change, then we might say it's not worth it for v1, given current timelines. We're reasonable designers ;)

Contributor

On 13/09/2012 11:14, Josh Carpenter wrote:

"... then we can retract it"

Heh, easy for me to say, right? Let us know how risky this is. If we
think we're going to drop 50% FPS, /and/ that it would take 1 Vivien
day to revert the change, then we might say it's not worth it for v1,
given current timelines. We're reasonable designers ;)

As far as I can tell, we loose FPS only in 'drag icons' mode and not
that much (i'm not sure why btw). vs should not change
anything once the view is computed as a texture and sent to the GPU.
Most of the laggyness comes from bugs in the homescreen impl and in Gecko.

Contributor

On 13/09/2012 10:59, Cristian Rodriguez wrote:

Did you fix the bookmark case? thanks

Nope I would like to generate a better shadow for bookmarks too and this
is out of the scope of this pull request. As I said previously I would
like to do that as a followup :)


Reply to this email directly or view it on GitHub
#4283 (comment).

Owner

@vingtetun As a workaround to crash-y home screen, and to save some CPU cycles, do could instead save the generated image with shadows as blobs into IndexedDB and rendered them with the same <img>.

Contributor
crdlc commented Sep 17, 2012

as far as I know the images belong to origins different than homescreen origin and it's impossible to get the image from the canvas. But I'm not sure. Am I wrong?

Contributor

The crash sounds like because Gecko leak at some point and it will be fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=791364 .

As @crdlc images belongs to different origins that can not use CORS so you can not canvas.toDataURL() here because of security restrictions. I'm actually using a systemXHR and a FileReader to retrieve the data: as base64 so I guess it can be done with additional work. What I don't like in this solution is the fact that if you switch language then you need to update the icon, if the resolution is different then you need to change the icon, etc... All thoses seems more work for something that should just work out of the box but does not probably because of a Gecko issue that needs to be fixed!

@crdlc Are you ok to land as if with 1 followup for bookmark?

Contributor
crdlc commented Sep 17, 2012

We need to keep in our mind that there are many problems with it and it should be solved in gecko. So please merge it if you want Vivien

@vingtetun vingtetun merged commit b4836ae into mozilla-b2g:master Sep 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment