Show search icon in about:home

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 verified)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
Instead of the Firefox logo.
Assignee

Comment 2

5 years ago
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.
Attachment #8473689 - Flags: review?(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.
Attachment #8473689 - Flags: review?(margaret.leibovic) → review+
Assignee

Comment 4

5 years ago
(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.
Backed out along with the rest of the patch stack for robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/d18d3e14ac3e
Assignee

Comment 7

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/f2c47e9e45e7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.