TopBookmarksView should not have rounded borders

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(1 attachment, 1 obsolete attachment)

No rounded borders on dominant colored backgrounds.
Posted 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.
Posted 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.

Updated

6 years ago
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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.