[Offline pages] Adding the status icon to the OIB

[Offline pages] Adding the status icon to the OIB

BUG=585209
Committed: https://crrev.com/f238ee15f949a4dd2998b2ff2ceadf0a9f952a1b
Cr-Commit-Position: refs/heads/master@{#378035}

[fgorski, 2016-02-26 01:57:26]

fgorski@chromium.org changed reviewers:
+ tedchoc@chromium.org

[fgorski, 2016-02-26 01:57:26]

There is one question related to offline page with latin URL shown on RTL UI,
that I posted to the designers, but please take a look.

[Ted C, 2016-02-26 20:57:37]

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/website_settings.xml:29: <ImageView
android:id="@+id/status_icon"
Instead of adding the ImageView, can you try calling:
ApiCompatibilityUtils#setCompoundDrawablesRelative

Along with:
http://developer.android.com/reference/android/widget/TextView.html#setCompou...

I suspect it would center the asset vs aligning it to the top, which is probably
not what you want.  But it would be better than adding the linearlayout.  But if
it doesn't work then this is fine.

Also, we should call this offline_icon for now.

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/website_settings.xml:36: android:alpha="0.54"
do you need the alpha or the new assets negate the need for this?

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/website_settings.xml:38:
android:contentDescription="@string/accessibility_toolbar_btn_site_info" />
we should have a specific accessibility string.  this should say something about
offline instead of a generic site info string.

[fgorski, 2016-02-26 22:33:26]

PTAL

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/website_settings.xml:29: <ImageView
android:id="@+id/status_icon"
On 2016/02/26 20:57:37, Ted C wrote:
> Instead of adding the ImageView, can you try calling:
> ApiCompatibilityUtils#setCompoundDrawablesRelative
> 
> Along with:
>
http://developer.android.com/reference/android/widget/TextView.html#setCompou...
> 
> I suspect it would center the asset vs aligning it to the top, which is
probably
> not what you want.  But it would be better than adding the linearlayout.  But
if
> it doesn't work then this is fine.
> 
> Also, we should call this offline_icon for now.

Renamed. I tried the content drawables, but that indeed centers the icon on the
left and does not allow to specify content description. 
We want to appear on top of the dialog. Linear Layout seems better for that.

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/website_settings.xml:36: android:alpha="0.54"
On 2016/02/26 20:57:37, Ted C wrote:
> do you need the alpha or the new assets negate the need for this?

Done.

https://codereview.chromium.org/1740063002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/website_settings.xml:38:
android:contentDescription="@string/accessibility_toolbar_btn_site_info" />
On 2016/02/26 20:57:37, Ted C wrote:
> we should have a specific accessibility string.  this should say something
about
> offline instead of a generic site info string.

Done.

[Ted C, 2016-02-26 22:34:08]

lgtm

https://codereview.chromium.org/1740063002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1740063002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1460: Offline content
icon
I would drop "icon".

[fgorski, 2016-02-26 22:36:18]

Updated

https://codereview.chromium.org/1740063002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1740063002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1460: Offline content
icon
On 2016/02/26 22:34:08, Ted C wrote:
> I would drop "icon".

Done.

[fgorski, 2016-02-26 22:36:30]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-02-26 22:36:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1740063002/#ps40001
(title: "Updating content description for the icon")

[commit-bot: I haz the power, 2016-02-26 22:38:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1740063002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1740063002/40001

[commit-bot: I haz the power, 2016-02-27 00:13:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-27 00:15:54]

Description was changed from

==========
[Offline pages] Adding the status icon to the OIB

BUG=585209
==========

to

==========
[Offline pages] Adding the status icon to the OIB

BUG=585209

Committed: https://crrev.com/f238ee15f949a4dd2998b2ff2ceadf0a9f952a1b
Cr-Commit-Position: refs/heads/master@{#378035}
==========

[commit-bot: I haz the power, 2016-02-27 00:15:55]

Patchset 3 (id:??) landed as
https://crrev.com/f238ee15f949a4dd2998b2ff2ceadf0a9f952a1b
Cr-Commit-Position: refs/heads/master@{#378035}