Closed Bug 895423 Opened 7 years ago Closed 7 years ago

Search engine provider's associated icon in the search pane is too small

Categories

(Firefox for Android :: General, defect)

25 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox25 --- verified
firefox26 --- verified

People

(Reporter: aaronmt, Assigned: ckitching)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 21 obsolete files)

91.43 KB, image/png
Details
75.54 KB, image/png
Details
21.99 KB, image/png
Details
82.21 KB, image/png
Details
40.85 KB, image/png
Details
15.15 KB, patch
liuche
: review+
Details | Diff | Splinter Review
Mentioned this yesterday in channel but didn't see a bug filed. Bug 892094 landed [1] with a dimension set at 32dp for each installed engine. The current dimensions are very small and difficult to view (at least on my Samsung Galaxy SIV and HTC One at 1920x1080).

[1] https://hg.mozilla.org/mozilla-central/rev/f1028e173331
A quick solution might be to use a FaviconView for the icon, which will create a background with the dominant color, like we do in the awesomescreen.

We could also explore including larger icons in the search plugin XML files, but I don't think the spec supports multiple sizes. Although the docs here do mention a 64x64 icon that can be included:
https://developer.mozilla.org/en/docs/Creating_OpenSearch_plugins_for_Firefox#OpenSearch_description_file

Gavin, do you know if there's a way for us to include multiple icon sizes in the search plugin XML?
Flags: needinfo?(gavin.sharp)
(In reply to :Margaret Leibovic from comment #1)
> We could also explore including larger icons in the search plugin XML files,
> but I don't think the spec supports multiple sizes. Although the docs here
> do mention a 64x64 icon that can be included:

I don't know where that note comes from - I suppose it may be a guideline to encourage people to include higher-res icons, but Firefox doesn't use them.

> Gavin, do you know if there's a way for us to include multiple icon sizes in
> the search plugin XML?

You can just specify multiple <Image> elements, but we'll need to fix the search service to expose them somehow. I.e. we'll need to add a largeIcon attribute (or an size:iconURI map or something) on nsISearchEngine, and then change the search service loading code to pay attention to more than just the 16x16 icons. We should do that in another bug.

Alternatively, you can use a trick like bug 795495 and just stuff the larger icon inside the 16x16 one in the XML. But that probably only works if you're displaying the image in Gecko, which I imagine isn't the case here. I think we should still improve the search service either way.
Flags: needinfo?(gavin.sharp)
We could use Android's resource directories to allow us to provide various different versions of the icons for the bundled search engines - one for each screen density - allowing us to always show a correctly sized icon for those engines.
This also has the advantage of not having to use the somewhat wasteful (if I remember correctly..) base64-encoding scheme currently being used to stick the images in the xml files.

Of course, that's no good for non-bundled engines - we could handle those by just doing what Margaret suggested in comment 1.

Does this seem a reasonable plan? Cheat and get pretty icons for the bundled engines and make the best of the (possibly tiny) favicons we're going to have available for the non-bundled engines, essentially.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)

> Alternatively, you can use a trick like bug 795495 and just stuff the larger
> icon inside the 16x16 one in the XML. But that probably only works if you're
> displaying the image in Gecko, which I imagine isn't the case here. I think
> we should still improve the search service either way.

That's currently what we're doing with 32x32 icons, we just want even larger ones than that for hi-res devices.
(In reply to Chris Kitching [:ckitching] from comment #3)
> We could use Android's resource directories to allow us to provide various
> different versions of the icons for the bundled search engines - one for
> each screen density - allowing us to always show a correctly sized icon for
> those engines.
> This also has the advantage of not having to use the somewhat wasteful (if I
> remember correctly..) base64-encoding scheme currently being used to stick
> the images in the xml files.
> 
> Of course, that's no good for non-bundled engines - we could handle those by
> just doing what Margaret suggested in comment 1.
> 
> Does this seem a reasonable plan? Cheat and get pretty icons for the bundled
> engines and make the best of the (possibly tiny) favicons we're going to
> have available for the non-bundled engines, essentially.

What would that solution look like? Currently the search service handles getting those icons for us, and we generally try to avoid adding mobile-only hacks to shared code. Also, locales and distributions can both bundle their own search plugins, so we should try to come up with a solution that works for them as well.

I think Gavin's suggestion to patch the search service would be the most robust solution. If you want to work on that, you could get some good experience working on toolkit code :) However, if you want to land something sooner than that, I think just using FaviconView with the dominant color would be a good compromise, since that's what we're already doing in the awesomescreen.
(In reply to :Margaret Leibovic from comment #5)
> (In reply to Chris Kitching [:ckitching] from comment #3)
> > We could use Android's resource directories to allow us to provide various
> > different versions of the icons for the bundled search engines - one for
> > each screen density - allowing us to always show a correctly sized icon for
> > those engines.
> > This also has the advantage of not having to use the somewhat wasteful (if I
> > remember correctly..) base64-encoding scheme currently being used to stick
> > the images in the xml files.
> > 
> > Of course, that's no good for non-bundled engines - we could handle those by
> > just doing what Margaret suggested in comment 1.
> > 
> > Does this seem a reasonable plan? Cheat and get pretty icons for the bundled
> > engines and make the best of the (possibly tiny) favicons we're going to
> > have available for the non-bundled engines, essentially.
> 
> What would that solution look like? Currently the search service handles
> getting those icons for us, and we generally try to avoid adding mobile-only
> hacks to shared code. Also, locales and distributions can both bundle their
> own search plugins, so we should try to come up with a solution that works
> for them as well.
> 
> I think Gavin's suggestion to patch the search service would be the most
> robust solution. If you want to work on that, you could get some good
> experience working on toolkit code :) However, if you want to land something
> sooner than that, I think just using FaviconView with the dominant color
> would be a good compromise, since that's what we're already doing in the
> awesomescreen.

If the message Gecko sends to Java providing the list of search engines were changed to include a flag marking which search engines are bundled then Java would be able to make the following decision:

If this engine is a bundled engine, get the appropriate icon resource from the Android resource system (And we have bundled icons of the appropriate size for each of the various screen densities for each of the bundled engines)

If this is not a bundled engine, use a FaviconView with the favicon Gecko gives us, instead.


If this message change counts as a "mobile specific hack", then we could instead bundle higher-res images in the xml files for bundled engines, and do similar logic (Engines with high res images just have those shown, others fall back to a FaviconView)

That said, do we want this discrepancy of image quality between the bundled/nonbundled engines? Does this matter enough that it's worth making the apk (Slightly) larger? Should we just do FaviconView anyway?
I wouldn't worry about a slightly larger APK, but I would worry about complex code paths. I think we should look at putting 64px images in the bundled XML files (in a followup bug) and using FaviconView for anything else.

This bug could be about using FaviconView.
Assignee: nobody → ckitching
tracking-fennec: --- → ?
Depends on: 900137
tracking-fennec: ? → 25+
A patch addressing this problem - icons are now rendered using FaviconView in the custom Preference object used to represent search engines.
Patch depends on that for Bug 898151.

As an added bonus, no longer depending on Preference.setIcon() means we now get to display icons on older versions of Android. (All of them, in fact).
Attachment #784660 - Flags: review?(liuche)
Attachment #784660 - Attachment description: largerSearchEngineIcons.patch → Enlarge the search engine icons using FaviconView
Attached image Screenshot from Nexus 4 (obsolete) —
Attached image Screenshot from an an ancient Droid 2 (obsolete) —
Attachment #784662 - Attachment description: Screenshot from an anccient Droid 2 → Screenshot from an an ancient Droid 2
Attached image Screenshot from a tablet (obsolete) —
Some screenshots using the patch.
Comment on attachment 784660 [details] [diff] [review]
Enlarge the search engine icons using FaviconView

Scratch that. This patch incorrectly causes the favicon to be upscaled in all cases.
Attachment #784660 - Attachment is obsolete: true
Attachment #784660 - Flags: review?(liuche)
Revised patch - this one does the trick.
Attachment #784661 - Attachment is obsolete: true
Attachment #784662 - Attachment is obsolete: true
Attachment #784663 - Attachment is obsolete: true
Attachment #784714 - Flags: review?(liuche)
Attached image Nexus 4 screenshot using patch (obsolete) —
Attached image Old phone using patch (Now has icons!) (obsolete) —
Attached image Tablet using patch (obsolete) —
No longer blocks: 892094
Depends on: 898151
Comment on attachment 784714 [details] [diff] [review]
Enlarge the icons V2: Actually using FaviconView now

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

r+ after addressing comment.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +198,5 @@
>  
> +        // Copy the icon from this object to the prompt we produce. We lazily create the drawable,
> +        // as the user may not ever actually tap this object.
> +        if (mPromptIcon == null) {
> +            mPromptIcon = new BitmapDrawable(mIconBitmap);

Nice.

::: mobile/android/base/resources/layout/preference_search_engine.xml
@@ +46,5 @@
> +
> +   </RelativeLayout>
> +
> +    <!-- Preference should place its actual preference widget here. -->
> +    <LinearLayout android:id="@+android:id/widget_frame"

Where is widget_frame used? And what is a preference widget?
Attachment #784714 - Flags: review?(liuche) → review+
Attachment #785198 - Attachment is obsolete: true
Pruning the madness from the XML - it was a tag defined in the default Preference layout of which this file is a modified version.
Attachment #784714 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: checkin to fx-team
Keywords: checkin-needed
Whiteboard: checkin to fx-team
Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/7c080e5f472b
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 25
tracking-fennec: 25+ → ---
Target Milestone: Firefox 25 → ---
No longer causing rc2 oranges:
https://tbpl.mozilla.org/?tree=Try&rev=bd3cf17da040
Attachment #785295 - Attachment is obsolete: true
Attachment #786567 - Flags: review?(liuche)
Attachment #786567 - Attachment description: nlarge the icons V5: No longer causing oranges. → Enlarge the icons V5: No longer causing oranges.
Comment on attachment 786567 [details] [diff] [review]
Enlarge the icons V5: No longer causing oranges.

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

A quick fly-by:
- It looks like the text is still too large in this patch. I remember you mentioning that the default Android text style didn't match our Fennec styles, so look into that and maybe use textAppearance to use our custom text styles.
- One small UI thing - the icons are positioned right at the edge of the highlight. This is polish, but I'd like to see a tiny margin between the edge of the Preference layout and the icon. Alternatively, I guess we could make the icon smaller but that might be awkward with handling the icons for different dpi screens.
Attachment #786567 - Flags: review?(liuche)
Corrected the text size, added the requested padding, and addressed a number of icon-size inconsistency issues across devices.
Attachment #786567 - Attachment is obsolete: true
Attachment #788448 - Flags: review?(liuche)
Attached image Tablet using V6 patch.
Attachment #784718 - Attachment is obsolete: true
This phone is mad and oversized the icons previously. It's now at least somewhat less broken.
Attachment #784716 - Attachment is obsolete: true
Attachment #784717 - Attachment is obsolete: true
Attachment #777799 - Attachment description: HTC One (Android 4.2.2) | Screenshot → HTC One (Android 4.2.2) | Screenshot without patch.
Comment on attachment 788448 [details] [diff] [review]
Enlarge the icons V6: Padding, text, and icon consistency

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

With the changes suggested, this should be good to go. There's a large bug where you don't actually set a bitmap that you've scaled.

Thanks for doing all the manual device testing!

In case this is getting old (my local build on a modern phone seems fine though), I pushed another try build here: https://tbpl.mozilla.org/?tree=Try&rev=71147f3e1255

::: mobile/android/base/Favicons.java
@@ +42,5 @@
>      public static final long FAILED_EXPIRY_NEVER = -1;
>      public static final int FLAG_PERSIST = 1;
>      public static final int FLAG_SCALE = 2;
>  
> +    public static int sFaviconSmallSize = -1;

Why is this public now? I don't see it used by anything else.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +31,5 @@
>  public class SearchEnginePreference extends Preference {
>      private static final String LOGTAG = "SearchEnginePreference";
>  
>      // Dimensions, in dp, of the icon to display for this engine.
>      public static int sIconSize;

Since we now have two types of icon vars, how about rename this to sListIconSize?

@@ +32,5 @@
>      private static final String LOGTAG = "SearchEnginePreference";
>  
>      // Dimensions, in dp, of the icon to display for this engine.
>      public static int sIconSize;
> +    public static int sDialogIconSize;

Are these used anywhere other than SearchEnginePreference? Any reason why don't we make them private?

@@ +98,5 @@
>                                        res.getString(R.string.pref_search_remove) };
>      }
>  
>      /**
> +    * Called by android when we're bound to the custom view. Allows us to set the custom properties

nits: capitalization of "Android", comment alignment.

@@ +105,5 @@
> +    */
> +    @Override
> +    protected void onBindView(View view) {
> +        super.onBindView(view);
> +        // Set the icon in the FaviconView

nit: comments as full sentences, w/ periods.

@@ +106,5 @@
> +    @Override
> +    protected void onBindView(View view) {
> +        super.onBindView(view);
> +        // Set the icon in the FaviconView
> +        ((FaviconView) view.findViewById(R.id.search_engine_icon)).updateImage(mIconBitmap, getTitle().toString());

For style clarity, split this out into getting the view, and then updating the image of the view. (this is a lucasr nit)

::: mobile/android/base/resources/layout/preference_search_engine.xml
@@ +14,5 @@
> +    <org.mozilla.gecko.widget.FaviconView
> +        android:id="@+id/search_engine_icon"
> +        android:layout_width="@dimen/favicon_bg"
> +        android:layout_height="@dimen/favicon_bg"
> +        android:layout_centerVertical="true"

Is this necessary? I think layout_centerVertical is a RelativeLayout param.

@@ +19,5 @@
> +        android:layout_gravity="center"
> +        android:minWidth="@dimen/favicon_bg"
> +        android:minHeight="@dimen/favicon_bg" />
> +
> +    <RelativeLayout

According to my reviews from sriram, RelativeLayout is to be avoided for its extra loading time. And this could be switched to LinearLayout pretty easily, correct?

::: mobile/android/base/widget/FaviconView.java
@@ +31,4 @@
>  
>      public FaviconView(Context context, AttributeSet attrs) {
>          super(context, attrs);
> +        setScaleType(ScaleType.CENTER);

I would actually prefer this to stay ImageView.ScaleType, since ScaleType isn't imported as a resource, so it can be a little confusing where this comes from.

@@ +48,5 @@
> +     * Updates the displayed image, if the prerequisite data are available. Upscales tiny Favicons to
> +     * normal sized ones, replaces null bitmaps with the default Favicon, and fills all remaining space
> +     * in this view with the coloured background.
> +     */
> +    private void refreshImage() {

I'd prefer to have this named something like formatImage, since refreshImage and updateImage are not clearly differentiated. Also update the comment, because "Updates the displayed image" seems like it should be referring to the method updateImage.

@@ +58,5 @@
> +            return;
> +        }
> +
> +        // If we're called before size set, abort.
> +        if (mActualWidth == 0 || mActualHeight == 0) {

Comment here that we won't display an image at all, and under what circumstances this would happen - presumably it's very unlikely? Might even want to add a Log.w here, to explain why there isn't an image.

@@ +65,5 @@
> +        }
> +
> +        // If there's enough extra space to upscale a tiny Favicon to a less tiny one, do it.
> +        if (mActualWidth > Favicons.sFaviconLargeSize && mIconBitmap.getWidth() < Favicons.sFaviconLargeSize) {
> +            Bitmap.createScaledBitmap(mIconBitmap, Favicons.sFaviconLargeSize, Favicons.sFaviconLargeSize, false);

You never assign this returned bitmap to anything.

@@ +76,5 @@
> +        setImageBitmap(mIconBitmap);
> +
> +        // But even if it claims to be a large Favicon, it may not be sufficiently large to occupy
> +        // the space consumed by this view. If there's too much space around the edges, show the
> +        // background anyway...

Maybe this is a problem from not setting the scaled bitmap from above?

@@ +93,5 @@
> +     */
> +    private void showBackground() {
> +        int color = Favicons.getInstance().getFaviconColor(mIconBitmap, mIconKey);
> +        color = Color.argb(70, Color.red(color), Color.green(color), Color.blue(color));
> +        Drawable drawable = getResources().getDrawable(R.drawable.favicon_bg);

Might as well make the Drawable final.

@@ +105,5 @@
> +    private void hideBackground() {
> +        setBackgroundResource(0);
> +    }
> +
> +    /**

Missing comment body.
Attachment #788448 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #30)
> ::: mobile/android/base/Favicons.java
> @@ +42,5 @@
> >      public static final long FAILED_EXPIRY_NEVER = -1;
> >      public static final int FLAG_PERSIST = 1;
> >      public static final int FLAG_SCALE = 2;
> >  
> > +    public static int sFaviconSmallSize = -1;
> 
> Why is this public now? I don't see it used by anything else.

Just for neatness, largely. sFaviconLargeSize is used by this patch - it didn't seem to be nice to have only one of this pair of related values accessible to the world. I can make it so, if you'd prefer.

> ::: mobile/android/base/preferences/SearchEnginePreference.java
> @@ +31,5 @@
> >  public class SearchEnginePreference extends Preference {
> >      private static final String LOGTAG = "SearchEnginePreference";
> >  
> >      // Dimensions, in dp, of the icon to display for this engine.
> >      public static int sIconSize;
> 
> Since we now have two types of icon vars, how about rename this to
> sListIconSize?

Actually, this shouldn't exist any more - its uses should instead be references to the identically-valued value in Favicons.

> @@ +58,5 @@
> > +            return;
> > +        }
> > +
> > +        // If we're called before size set, abort.
> > +        if (mActualWidth == 0 || mActualHeight == 0) {
> 
> Comment here that we won't display an image at all, and under what
> circumstances this would happen - presumably it's very unlikely? Might even
> want to add a Log.w here, to explain why there isn't an image.

This should never result in an image not being displayed.
The onSizeChanged method will be called by Android sometime during the layout calculations. The user of the FaviconView class will call updateImage at some point, too. It's not defined which one is called first.
Both of these methods call into refreshImage - the first one will certainly fail, but the second one will succeed. Even if you omit to ever call updateImage, you'll still at least be shown the default favicon (Possibly incorrectly scaled).
 
> @@ +65,5 @@
> > +        }
> > +
> > +        // If there's enough extra space to upscale a tiny Favicon to a less tiny one, do it.
> > +        if (mActualWidth > Favicons.sFaviconLargeSize && mIconBitmap.getWidth() < Favicons.sFaviconLargeSize) {
> > +            Bitmap.createScaledBitmap(mIconBitmap, Favicons.sFaviconLargeSize, Favicons.sFaviconLargeSize, false);
> 
> You never assign this returned bitmap to anything.

AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.

> @@ +76,5 @@
> > +        setImageBitmap(mIconBitmap);
> > +
> > +        // But even if it claims to be a large Favicon, it may not be sufficiently large to occupy
> > +        // the space consumed by this view. If there's too much space around the edges, show the
> > +        // background anyway...
> 
> Maybe this is a problem from not setting the scaled bitmap from above?

Often in the case of search preferences, yes. In the more general case of the use of FaviconViews, no. The FaviconView is used to display a Favicon in a place where the Favicon itself may not be sufficiently large. If it can't be scaled without horrid pixelation, we need to fill up the extra space in the view with solid colour (That's sort of the purpose of the class).
It just now does so slightly more intelligently than it once did.
Attachment #791057 - Flags: review?(liuche)
Attachment #788448 - Attachment is obsolete: true
Comment on attachment 791057 [details] [diff] [review]
Enlarge the icons V7: AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.

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

Looks good, some weirdness going on with overstretching favicons, and a few more nits - I think one more review round should do it.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +99,5 @@
> +    protected void onBindView(View view) {
> +        super.onBindView(view);
> +
> +        // Set the icon in the FaviconView.
> +        FaviconView targetView = ((FaviconView) view.findViewById(R.id.search_engine_icon));

Style nit: make this final.

::: mobile/android/base/resources/layout/preference_search_engine.xml
@@ +25,5 @@
> +        android:layout_marginLeft="15dip"
> +        android:layout_marginRight="6dip"
> +        android:layout_marginTop="6dip"
> +        android:layout_marginBottom="6dip"
> +        android:layout_weight="1"

According to sriram [1], when you set layout_weight, you should also set the android:layout_height to be 0dip, because layout_height is not longer relevant and this saves us some inflation calculations.

...did your IDE not complain with a warning, the way Android Lint does? </jest>

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=891953#c21 at the layout_weight part.

@@ +26,5 @@
> +        android:layout_marginRight="6dip"
> +        android:layout_marginTop="6dip"
> +        android:layout_marginBottom="6dip"
> +        android:layout_weight="1"
> +        android:orientation="vertical">

Tiny tiny nit: move orientation up, to be below layout_height. To me, it's more general than all the margin stuff.

::: mobile/android/base/widget/FaviconView.java
@@ +104,5 @@
> +        setBackgroundResource(0);
> +    }
> +
> +    /**
> +     *

Please add at least a token javadoc comment here for "updateImage".
Attachment #791057 - Flags: review?(liuche) → review-
(In reply to Chenxia Liu [:liuche] from comment #32)
> ::: mobile/android/base/resources/layout/preference_search_engine.xml
> @@ +25,5 @@
> > +        android:layout_marginLeft="15dip"
> > +        android:layout_marginRight="6dip"
> > +        android:layout_marginTop="6dip"
> > +        android:layout_marginBottom="6dip"
> > +        android:layout_weight="1"
> 
> According to sriram [1], when you set layout_weight, you should also set the
> android:layout_height to be 0dip, because layout_height is not longer
> relevant and this saves us some inflation calculations.

So it turned out that doing this made the text vanish. The error seemed to be to have declared layout_weight at all.

> ...did your IDE not complain with a warning, the way Android Lint does?
> </jest>

For reasons that are not entirely clear, my IDE hangs when I enable the Android Lint inspections (Which are literally implemented as "Run Android Lint and parse the results" :P



As for the stretchy strangeness - that is indeed strange. The logic of the revamped Favicon view is now as follows:
If the provided Favicon is larger than this FaviconView, downscale it to fit.
If the provided Favicon is smaller than this FaviconView, upscale it by a maximum of a factor of two to fit. If, after upscaling by a factor of two, the image is still smaller than the view, then fill the remaining space with the dominant colour.

We now get consistently large Favicons everywhere, and, due to no longer doing multiple scaling there, less horridly broken Favicons on the Galaxy Ch@t. (All five of our customers using such a device will be pleased.)
Attachment #791057 - Attachment is obsolete: true
Attachment #793692 - Flags: review?(liuche)
Comment on attachment 793692 [details] [diff] [review]
Enlarge the icons V8: Nits + No more scaling strangeness

Drive-by: Unfortunately, you're going to need to update the awesomesceen parts of this patch now that fig has merged back into m-c.

In other news, this relates to bug 888326, so you may want to comment there if you have any findings to share about ways we can improve how/where we scale favicons.
Attachment #793692 - Attachment is obsolete: true
Attachment #793692 - Flags: review?(liuche)
Setting to block bug 867371 to track this as part of the settings UI work.
Blocks: 867371
Simple patch to solve the problem - Depends on monster patch series from Bug 	888326.
Attachment #800492 - Flags: review?(liuche)
Depends on: 888326
Adding new dependancies - prior solutions only actually worked for bundled engines - were no good for, say, DuckDuckGo, since the underlying Favicon system would fetch a pathetically small Favicon and then we'd be, essentially, stuck.
So, having redone how we do Favicons in general, all that's needed in this patch is to make the preferences entries have a Favicon view and put Favicons in it. Simple!

Previously:
https://www.dropbox.com/s/b1z6423rpintus3/PrefBefore.png?m

Now:
https://www.dropbox.com/s/dmdapt411l5agr4/PrefAfter.png

The duck is looking excellent, for a change.
Depends on: 855911, ICODecoder
No longer depends on: 888326
Patches below this one were changed. Unbitrotting.
Attachment #800492 - Attachment is obsolete: true
Attachment #800492 - Flags: review?(liuche)
Attachment #802081 - Flags: review?(liuche)
Attachment #802081 - Attachment is obsolete: true
Attachment #802081 - Flags: review?(liuche)
Attachment #802106 - Flags: review?(liuche)
Comment on attachment 802106 [details] [diff] [review]
Enlarge the icons V11: Unborking.

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

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +53,5 @@
> +    private BitmapDrawable mPromptIcon;
> +    // The bitmap backing the drawable above - needed separately for the FaviconView.
> +    private Bitmap mIconBitmap;
> +
> +    private FaviconView aFaviconView;

Drive-by: Name this mFaviconView. By convention, the a prefix is usually used for method parameters (and as I mentioned in another bug, we've been moving away from doing that).
(In reply to :Margaret Leibovic from comment #40)
> Drive-by: Name this mFaviconView. By convention, the a prefix is usually
> used for method parameters (and as I mentioned in another bug, we've been
> moving away from doing that).

Of course. Moment of madness.

Not refactoring this file away from the naming convention in this patch - it doesn't touch enough of it.
Attachment #802840 - Flags: review?(liuche)
Fixing a merge conflict. Things work better when the patch compiles.
Attachment #802106 - Attachment is obsolete: true
Attachment #802840 - Attachment is obsolete: true
Attachment #802106 - Flags: review?(liuche)
Attachment #802840 - Flags: review?(liuche)
Attachment #803123 - Flags: review?(liuche)
Comment on attachment 803123 [details] [diff] [review]
Enlarge the icons V13: Unbreaking

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

Try push here: https://tbpl.mozilla.org/?tree=Try&rev=bfce77dfc580

Only a couple of small problems!

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +68,5 @@
>          mParentCategory = parentCategory;
>  
>          Resources res = getContext().getResources();
>  
> +        // Set the layout resource for this preference - includes a FaviconView

Nit: trailing period.

::: mobile/android/base/resources/layout/preference_search_engine.xml
@@ +38,5 @@
> +
> +       <TextView android:id="@+android:id/summary"
> +            android:layout_width="wrap_content"
> +            android:layout_height="wrap_content"
> +            android:layout_below="@android:id/title"

LinearLayout doesn't use android:layout_below/align*. Maybe this is left over from a RelativeLayout?

::: mobile/android/base/widget/FaviconView.java
@@ +162,5 @@
>          updateImageInternal(bitmap, key, false);
>      }
> +
> +    public Bitmap getBitmap() {
> +        return iconBitmap;

Should be mIconBitmap, right? Doesn't build.
Attachment #803123 - Flags: review?(liuche) → review-
(In reply to Chenxia Liu [:liuche] from comment #43)
> ::: mobile/android/base/widget/FaviconView.java
> @@ +162,5 @@
> >          updateImageInternal(bitmap, key, false);
> >      }
> > +
> > +    public Bitmap getBitmap() {
> > +        return iconBitmap;
> 
> Should be mIconBitmap, right? Doesn't build.

Oh the hilarity.
Attachment #803123 - Attachment is obsolete: true
Attachment #804106 - Flags: review?(liuche)
Comment on attachment 804106 [details] [diff] [review]
Enlarge the icons V13: One of these days it'll work.

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

r+ with comment.

::: mobile/android/base/home/SearchEngineRow.java
@@ +165,5 @@
>          // Update search engine reference
>          mSearchEngine = searchEngine;
>  
>          // Set the search engine icon (e.g., Google) for the row
> +        mIconView.updateAndScaleImage(mSearchEngine.icon, mSearchEngine.name);

Last thing: this method isn't very clear (when do you scale?) so can you add a comment on the definition of this method, and reference the bug to auto-add bundled search icons to the Favicon cache?
Attachment #804106 - Flags: review?(liuche) → review+
Thought I'd save you the weekend review before I pass out on the floor.
Attachment #804106 - Attachment is obsolete: true
Attachment #804803 - Flags: review?(liuche)
Comment on attachment 804803 [details] [diff] [review]
Enlarge the icons V14: Adding comment.

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

Thank you!
Attachment #804803 - Flags: review?(liuche) → review+
https://hg.mozilla.org/mozilla-central/rev/e256831552b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Status: RESOLVED → VERIFIED
No longer depends on: 900137
You need to log in before you can comment on or make changes to this bug.