1054268 - Show search icon in about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Instead of the Firefox logo.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Show search icon in about:home (r=margaret)

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8473689 [details] [diff] [review]
Show search icon in about:home (r=margaret)

Predefine a different icon for about:home. Using ab_search as it seems to better match antlam's design.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8473689 [details] [diff] [review]
Show search icon in about:home (r=margaret)

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

r+ with comments addressed.

::: mobile/android/base/AboutPages.java
@@ +77,5 @@
>          }
>  
> +        if (isAboutHome(url)) {
> +            return true;
> +        }

Looking at the changes in this patch, I was initially confused as to how this changed the behavior, but I see now that removing HOME from DEFAULT_ICON_PAGES changes what pages get stored with the default icon in the favicon cache.

I think we should update the name of this isDefaultIconPage method to something more like isSpecialIconPage, since it doesn't actually check if a url is a default icon page anymore. Where it's used in Tabs, we really just want to know if we should be updating the tab with a special icon from the cache.

You can also add a comment above this isAboutHome check to explain that about:home uses a special search icon.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8473689 [details] [diff] [review]
> Show search icon in about:home (r=margaret)
> 
> Review of attachment 8473689 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed.
> 
> ::: mobile/android/base/AboutPages.java
> @@ +77,5 @@
> >          }
> >  
> > +        if (isAboutHome(url)) {
> > +            return true;
> > +        }
> 
> Looking at the changes in this patch, I was initially confused as to how
> this changed the behavior, but I see now that removing HOME from
> DEFAULT_ICON_PAGES changes what pages get stored with the default icon in
> the favicon cache.
> 
> I think we should update the name of this isDefaultIconPage method to
> something more like isSpecialIconPage, since it doesn't actually check if a
> url is a default icon page anymore. Where it's used in Tabs, we really just
> want to know if we should be updating the tab with a special icon from the
> cache.
> 
> You can also add a comment above this isAboutHome check to explain that
> about:home uses a special search icon.

Good points, I'll go with isBuiltinIconPage() and add the suggested comment.

Comment 5

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/31d4832196f6

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out along with the rest of the patch stack for robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/d18d3e14ac3e

Comment 7

[Lucas Rocha (:lucasr)]

The string changes in bug 1010741 caused the failures. Forgot to update the tests accordingly. Try build looks nice and green now, pushed:

https://hg.mozilla.org/integration/fx-team/rev/f2c47e9e45e7

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f2c47e9e45e7

Comment 9

[Cristina Madaras, QA [:CristinaM]]

Search icon is displayed in about:home instead of the Firefox logo, so verified as fixed in
Build: Firefox for Android 34.0a1 (2014-08-19)
Devices: 
Asus Transformer Pad TF300T (Android 4.2.1)
Samsung S3 (Android 4.3)