978065 - [Music] Favor scroll performance over thumbnails quality

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: music.favor.performance.over.quality.patch

Comment 1

[David Flanagan [:djf]]

Comment on attachment 8383626 [details] [diff] [review]
music.favor.performance.over.quality.patch

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

If you need to do this for performance reasons, I'm not going to object, I guess.

But this is a somewhat different case than gallery where users can just tap on a thumbnail to see a larger image.  I think the idea of that tiles view is that it is supposed to display the album art attractively.

Do you get good enough performance if you only set this property on the .sub-tile class?  That should at least leave the larger images at higher quality.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug978065.v2.patch

This version use the little helper made in bug 943845. It restores the quality at some point. I also fixed the height of the views element as they don't larger than the visible space for scrolling and the scrollbars appears under the bottom bar.

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8384265 [details] [diff] [review]
bug978065.v2.patch

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

This looks okay ot me, but consider the changes below. 

I'm passing the review on to dkuo since he knows this app much better than I do, and because I don't understand the CSS change to subtract 4rem from the height of the view containers.

Dominic: see bug 943845 for the scroll_listener.js file that this patch uses.

::: apps/music/js/music.js
@@ +1967,5 @@
>  };
> +
> +window.addEventListener('scrollstatechanged', function onScroll(e) {
> +  var views = document.getElementById('views');
> +  views.classList.toggle('scroll', e.detail);

As in gallery, I'd prefer "scrolling" to "scroll".

If you're not putting this class on the direct container of the tiles, would it be simpler to just put it on document.body so you don't have to call getElementById? (I'm not sure how that affects the CSS selector performance.)

::: apps/music/style/music.css
@@ +286,4 @@
>  #views-tiles,
>  #views-list,
>  #views-sublist {
> +  height: calc(100% - 4rem);

I don't understand where the 4rem is coming from, and would like Dominic to review.

@@ +445,5 @@
>    /* We set a background URL here when the thumbnail loads */
>  }
>  
> +/* Favor scrolling performance over quality while scrolling */
> +#views.scroll .tile {

Since the first part of the selector is not specific to the tiles view, should you just remove the .tile part?  Would this optimization be helpful in other views, such as when scrolling through the list of all songs?  Or does .tile and li.list-item cover all the relevant cases?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to David Flanagan [:djf] from comment #3)
> ::: apps/music/js/music.js
> @@ +1967,5 @@
> >  };
> > +
> > +window.addEventListener('scrollstatechanged', function onScroll(e) {
> > +  var views = document.getElementById('views');
> > +  views.classList.toggle('scroll', e.detail);
> 
> As in gallery, I'd prefer "scrolling" to "scroll".
> 
> If you're not putting this class on the direct container of the tiles, would
> it be simpler to just put it on document.body so you don't have to call
> getElementById? (I'm not sure how that affects the CSS selector performance.)
> 

I do like to be specific, just to make sure to avoid side effects.

> ::: apps/music/style/music.css
> @@ +286,4 @@
> >  #views-tiles,
> >  #views-list,
> >  #views-sublist {
> > +  height: calc(100% - 4rem);
> 
> I don't understand where the 4rem is coming from, and would like Dominic to
> review.
> 

4rem is the size of the bar at the bottom of the app.

> @@ +445,5 @@
> >    /* We set a background URL here when the thumbnail loads */
> >  }
> >  
> > +/* Favor scrolling performance over quality while scrolling */
> > +#views.scroll .tile {
> 
> Since the first part of the selector is not specific to the tiles view,
> should you just remove the .tile part?  Would this optimization be helpful
> in other views, such as when scrolling through the list of all songs?  Or
> does .tile and li.list-item cover all the relevant cases?

My understanding is that it should cover all cases. But let's wait for Dominic to confirm that, I may have missed some bits as I don't know this app too well.

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 8384265 [details] [diff] [review]
bug978065.v2.patch

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

Vivien, the js part looks good to me, but there are some issues in the css part and needs adjustment before landing, overall it looks good and please see my review comments, thanks.

::: apps/music/style/music.css
@@ +286,4 @@
>  #views-tiles,
>  #views-list,
>  #views-sublist {
> +  height: calc(100% - 4rem);

Actually the height of the tab bar is 4.5rem, but I guess it used to be 4rem, we have some changes on this recently.

And if we want to modify the height of the views like this, then we should remove the margin-bottom of the last-child in tiles, list and sublist views, which are in: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/style/music.css#L480 and https://github.com/mozilla-b2g/gaia/blob/master/apps/music/style/music.css#L943 , or you will see extra padding under the last element.

@@ +445,5 @@
>    /* We set a background URL here when the thumbnail loads */
>  }
>  
> +/* Favor scrolling performance over quality while scrolling */
> +#views.scroll .tile {

If this css is targeting the elements/children in the tiles view, I am okay with it since .tile is only used on the tiles elements. If not, then probably we can just use #views.scroll without .tile.

@@ +971,5 @@
>    background-repeat: no-repeat;
>  }
>  
> +/* Favor scrolling performance over quality while scrolling */
> +#views.scroll li.list-item {

I think we can combine this css with the one above(#views.scroll .tile) so we can write less css and easy to find them if we want to change them in the future.

@@ +973,5 @@
>  
> +/* Favor scrolling performance over quality while scrolling */
> +#views.scroll li.list-item {
> +  image-rendering: -moz-crisp-edges;
> +  text-rendering: optmizeSpeed;

Is the "optmizeSpeed" spelled wrong?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Thanks for the review. I fixed the review comments and merged the patch: https://github.com/vingtetun/gaia/commit/f4a68429ef060515737df2fb846fd6e71e1ac097