888034 - TopBookmarksView should not have rounded borders

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

No rounded borders on dominant colored backgrounds.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Comment 2

[:Margaret Leibovic]

Comment on attachment 768604 [details] [diff] [review]
Patch

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

r+ with some comments.

::: mobile/android/base/Makefile.in
@@ +1017,5 @@
>    mobile/android/base/resources/drawable/abouthome_logo.xml                     \
>    mobile/android/base/resources/drawable/abouthome_promo_box.xml                \
>    mobile/android/base/resources/drawable/action_bar_button.xml                  \
>    mobile/android/base/resources/drawable/action_bar_button_inverse.xml          \
> +  mobile/android/base/resources/drawable/bookmark_thumbnail_view_bg.xml         \

Nit: I feel like we should just call this bookmark_thumbnail_bg, to be more consistent with favicon_bg.

::: mobile/android/base/home/BookmarkThumbnailView.java
@@ +96,5 @@
>       * @param color the color filter to apply over the drawable.
>       */
>      @Override
>      public void setBackgroundColor(int color) {
>          int colorFilter = color == 0 ? DEFAULT_COLOR : color & COLOR_FILTER;

I guess I missed looking closely at the patch where BookmarkThumbnailView was added, but I see this implements basically the same logic as FaviconView, but differently. Is that why you filed bug 883337? I feel like it would be nice for these files to be more consistent if possible (or even pull the logic out into some utility place, but maybe that's overkill). Maybe we can file a follow-up for that.

::: mobile/android/base/resources/drawable/bookmark_thumbnail_view_bg.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<shape xmlns:android="http://schemas.android.com/apk/res/android"
> +       android:shape="rectangle">
> +     <size android:height="@dimen/favicon_bg"/>

Aren't the thumbnails bigger than the favicon? How does this work correctly?

Comment 3

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 768604 [details] [diff] [review]
> Patch
> 
> Review of attachment 768604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with some comments.
> 
> ::: mobile/android/base/Makefile.in
> @@ +1017,5 @@
> >    mobile/android/base/resources/drawable/abouthome_logo.xml                     \
> >    mobile/android/base/resources/drawable/abouthome_promo_box.xml                \
> >    mobile/android/base/resources/drawable/action_bar_button.xml                  \
> >    mobile/android/base/resources/drawable/action_bar_button_inverse.xml          \
> > +  mobile/android/base/resources/drawable/bookmark_thumbnail_view_bg.xml         \
> 
> Nit: I feel like we should just call this bookmark_thumbnail_bg, to be more
> consistent with favicon_bg.
> 

Will change it.

> 
> ::: mobile/android/base/resources/drawable/bookmark_thumbnail_view_bg.xml
> @@ +4,5 @@
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +
> > +<shape xmlns:android="http://schemas.android.com/apk/res/android"
> > +       android:shape="rectangle">
> > +     <size android:height="@dimen/favicon_bg"/>
> 
> Aren't the thumbnails bigger than the favicon? How does this work correctly?

ShapeDrawable scales itself like a 9png. The height is just for some scaling. It could be a 2dp too and will work fine.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Changed the height to 2dp (and added a width too) for more clarity. Renamed the file.

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 768604 [details] [diff] [review]
> Patch
> ::: mobile/android/base/home/BookmarkThumbnailView.java
> @@ +96,5 @@
> >       * @param color the color filter to apply over the drawable.
> >       */
> >      @Override
> >      public void setBackgroundColor(int color) {
> >          int colorFilter = color == 0 ? DEFAULT_COLOR : color & COLOR_FILTER;
> 
> I guess I missed looking closely at the patch where BookmarkThumbnailView
> was added, but I see this implements basically the same logic as
> FaviconView, but differently. Is that why you filed bug 883337? I feel like
> it would be nice for these files to be more consistent if possible (or even
> pull the logic out into some utility place, but maybe that's overkill).
> Maybe we can file a follow-up for that.

Yup. I was looking at FaviconView while working on BookmarkThumbnailView and thought I could optimize it too. Hence filed bug 883337. I thought of extending this view from FaviconView, but that's a bit of an overkill -- the scale type changes based on whether we show an image or a favicon. Also, wesj wants to extend this view from ThumbnailView. So, only either of those two can be done. Pulling the logic out is unwanted too. One has rounded corners, and the other doesn't.

Comment 6

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/9c2d41078e3b

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c2d41078e3b