Closed Bug 888034 Opened 8 years ago Closed 8 years ago

TopBookmarksView should not have rounded borders

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: sriram, Assigned: sriram)

References

Details

(Whiteboard: fixed-fig)

Attachments

(1 file, 1 obsolete file)

No rounded borders on dominant colored backgrounds.
Attached patch Patch (obsolete) — Splinter Review
Attachment #768604 - Flags: review?(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?
Attachment #768604 - Flags: review?(margaret.leibovic) → review+
(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.
Attached patch PatchSplinter Review
Changed the height to 2dp (and added a width too) for more clarity. Renamed the file.
Attachment #768604 - Attachment is obsolete: true
Attachment #769054 - Flags: review?(margaret.leibovic)
(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.
Attachment #769054 - Flags: review?(margaret.leibovic) → review+
http://hg.mozilla.org/projects/fig/rev/9c2d41078e3b
Assignee: nobody → sriram
Blocks: 882365
Whiteboard: fixed-fig
https://hg.mozilla.org/mozilla-central/rev/9c2d41078e3b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.