This way we align our UI with desktop and cleanup the toolbar a bit. Toolbar would only show the security state (again, just like desktop). Thoughts?

Makes sense to me!

Agreed! The space for favicon on the tab strip can also be used for display loading indicator, like the throbber on desktop FX. And this will make even more sense when we support pinning a tab on the tab strip :)

I don't want to add a implementation-specific version for all of our files, if we can help it, so I opted for the simple solution which is to use ifs within the code. Let me know if you disagree with this approach. Lucas, do you want to deal with part 2 (adding the favicon to the tab strip), or shall I?

Doorhangers do look a bit fishy without the favicon though - Yuan, what do you think?

(In reply to Michael Comella (:mcomella) from comment #5) > Created attachment 8488379 [details] > Doorhanger, no favicon > > Doorhangers do look a bit fishy without the favicon though - Yuan, what do > you think? I think we'll need keep the security icon always visible and have 'default' state for when it doesn't have any special identity/security state i.e. the equivalent of the gray globe on desktop.

Comment on attachment 8488376 [details] [diff] [review] Part 1: Remove favicon from BrowserToolbar Review of attachment 8488376 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. But I think we should sort out the design for the security doorhanger as part of this bug. I think we should keep the security icon always visible and have a generic state for it (see comment #6). (In reply to Michael Comella (:mcomella) from comment #3) > Created attachment 8488376 [details] [diff] [review] > Part 1: Remove favicon from BrowserToolbar > > I don't want to add a implementation-specific version for all of our files, > if we can help it, so I opted for the simple solution which is to use ifs > within the code. Let me know if you disagree with this approach. Yeah, this approach is fine in this case. > Lucas, do you want to deal with part 2 (adding the favicon to the tab strip), > or shall I? Feel free to do it. Let me just land bug 1015447 so that you can write the patch on top of the latest tab strip code.

(In reply to Lucas Rocha (:lucasr) from comment #6) > I think we'll need keep the security icon always visible and have 'default' > state for when it doesn't have any special identity/security state i.e. the > equivalent of the gray globe on desktop. I assume phone would not be changing though, right? Thus we'd be adding more device specific code to ToolbarDisplayLayout?

(In reply to Michael Comella (:mcomella) from comment #8) > (In reply to Lucas Rocha (:lucasr) from comment #6) > > I think we'll need keep the security icon always visible and have 'default' > > state for when it doesn't have any special identity/security state i.e. the > > equivalent of the gray globe on desktop. > > I assume phone would not be changing though, right? Thus we'd be adding more > device specific code to ToolbarDisplayLayout? Yep. We can plan for a refactoring if ToolbarDisplayLayout starts to look too messy with these changes.

Note to self: try to adjust the padding around the site security icon (and placeholder), as per https://bug1058909.bugzilla.mozilla.org/attachment.cgi?id=8480237.

(In reply to Lucas Rocha (:lucasr) from comment #7) > Looks good. But I think we should sort out the design for the security > doorhanger as part of this bug. I think we should keep the security icon > always visible and have a generic state for it (see comment #6). Any ideas here, Anthony? Also, what should the state be for a website that doesn't have a favicon? According to [1], a rectangle should be fine. If so, I believe I can manage to draw that in code. [1]: https://bug1060413.bugzilla.mozilla.org/attachment.cgi?id=8490916

Also, do you have specs for the favicon size, margins, etc.?

After some eyeballing, I ended up with: * favicon: 20dp * favicon's right margin: 5dp Anthony, what do you think?

Lucas, the apple logo from the screenshot in comment 13 appears to have some (alpha?) aliasing issues - any idea why?

(In reply to Michael Comella (:mcomella) from comment #14) > Created attachment 8491920 [details] [diff] [review] > Part 2: Put favicons in the tab strip > > Lucas, the apple logo from the screenshot in comment 13 appears to have some > (alpha?) aliasing issues - any idea why? Maybe it's just a problem with the image itself? I suggest downloading the image to confirm. It might be a bug in the way our Favicons framework handle bitmaps though.

Comment on attachment 8491920 [details] [diff] [review] Part 2: Put favicons in the tab strip Review of attachment 8491920 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. ::: mobile/android/base/tabs/TabStripItemView.java @@ +19,5 @@ > import android.graphics.PorterDuff.Mode; > import android.graphics.PorterDuffXfermode; > import android.graphics.Region; > import android.util.AttributeSet; > +import android.util.Log; Unused? @@ -22,5 @@ > -import org.mozilla.gecko.Tab; > -import org.mozilla.gecko.Tabs; > -import org.mozilla.gecko.widget.ThemedImageButton; > -import org.mozilla.gecko.widget.ThemedLinearLayout; > -import org.mozilla.gecko.widget.ThemedTextView; Oopsie :-) @@ +191,5 @@ > } > > id = tab.getId(); > + // TODO: default if null. > + faviconView.setImageBitmap(tab.getFavicon()); You will probably do something similar to ToolbarDisplayLayout here: http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#353

hopefully this helps! I've tried to spec out the favicon a bit specifically around the left and right margins. I'll attach the empty state favicon (that I used here) after this too. Not sure if this is helpful, but the page titles are 14dp in the design and hard capped at 109dp width (where fading happens just before the length hits this cap). This allows for 9dp margins on either side of the 'x' as well for the active tab.

Comment on attachment 8491918 [details] Favicons just attached some specs, let's try that first. I think this looks like we could use more space to the right of the favicon.

Try these if you need some images for the "empty" favicon :)

Comment on attachment 8488376 [details] [diff] [review] Part 1: Remove favicon from BrowserToolbar Doorhanger can be part 3.

I can file a followup to draw empty favicon icons in code to cut down on apk size, if you think this might be useful.

Anthony, what do you think? I can only space a tab's favicon to the close button on tab to its left, not the curve - do you happen to know how many dps it is between those two? I used 27dp, as that's the distance to the screen edge for the first tab in the leftmost scrolled position.

Anthony, what should we do about the security doorhanger (comment 5)? Should I use the globe icon that's in the tree (for favicons) [1] in place of the security icon, like desktop does? [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xhdpi/favicon.png

Comment on attachment 8488376 [details] [diff] [review] Part 1: Remove favicon from BrowserToolbar Review of attachment 8488376 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 8492507 [details] [diff] [review] Part 2: Put favicons in the tab strip Review of attachment 8492507 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/resources/values-large-v11/dimens.xml @@ +9,5 @@ > <dimen name="browser_toolbar_button_padding">16dp</dimen> > <dimen name="tabs_counter_size">26sp</dimen> > <dimen name="panel_grid_view_column_width">200dp</dimen> > > + <dimen name="tab_strip_favicon_size">16dp</dimen> Dimensions provided by antlam?

(In reply to Lucas Rocha (:lucasr) from comment #25) > Dimensions provided by antlam? Size of the no-favicon favicon, provided by antlam. :D

Comment on attachment 8492510 [details] Favicons in tab strip This actually looks pretty close! I measured it on my file and it looks like it's 25dp. let me attach the spec again. :)

Specs for the favicon.

(In reply to Michael Comella (:mcomella) from comment #5) > Created attachment 8488379 [details] > Doorhanger, no favicon > > Doorhangers do look a bit fishy without the favicon though - Yuan, what do > you think? Yeah this does look a bit weird. Let me think about this. Seems to kind of defeat the purpose if we just have another icon in the URL bar.. but I could be wrong.

https://hg.mozilla.org/integration/fx-team/rev/298a2cd455fc https://hg.mozilla.org/integration/fx-team/rev/603dad513d7c

Oops - should have checked with Anthony before landing!

Comment on attachment 8488379 [details] Doorhanger, no favicon Let's do doorhangers in a followup: bug 1071267

Comment on attachment 8493366 [details] Favicon in tab strip v2 +!

https://hg.mozilla.org/mozilla-central/rev/298a2cd455fc https://hg.mozilla.org/mozilla-central/rev/603dad513d7c