[subresource_filter] Add a subtitle for Android Page Info

[subresource_filter] Add a subtitle for Android Page Info

This just uses a placeholder string for now.

BUG=689487
Review-Url: https://codereview.chromium.org/2879733002
Cr-Commit-Position: refs/heads/master@{#471339}
Committed: https://chromium.googlesource.com/chromium/src/+/cececce4d88ac1ca8b7a6e9f31597776ea151015

[Charlie Harrison, 2017-05-12 01:57:04]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 01:57:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-12 02:05:28]

csharrison@chromium.org changed reviewers:
+ bauerb@chromium.org

[Charlie Harrison, 2017-05-12 02:05:28]

bauerb: Could you PTAL?

I've added a screenshot of this change on the linked bug, along with a link to
the mocks I'm trying to reproduce.

[commit-bot: I haz the power, 2017-05-12 04:31:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 04:31:07]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-05-12 09:24:36]

lgtm

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

https://codereview.chromium.org/2879733002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/page_info_permission_row.xml:44:
android:lineSpacingExtra="6dp"
Where is this value coming from? The linked mocks didn't have any exact
measurements. In Material Design, spacing tends to be often defined in terms of
leading (the distance between subsequent baselines) instead of additional line
spacing, soo... if that is the case here, you could use a
org.chromium.ui.widget.TextViewWithLeading to automatically use the correct line
spacing.

[Charlie Harrison, 2017-05-12 12:31:32]

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

https://codereview.chromium.org/2879733002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/page_info_permission_row.xml:44:
android:lineSpacingExtra="6dp"
On 2017/05/12 09:24:36, Bernhard Bauer wrote:
> Where is this value coming from? The linked mocks didn't have any exact
> measurements. In Material Design, spacing tends to be often defined in terms
of
> leading (the distance between subsequent baselines) instead of additional line
> spacing, soo... if that is the case here, you could use a
> org.chromium.ui.widget.TextViewWithLeading to automatically use the correct
line
> spacing.

I used this value to align with the page_info_permission_unavailable_message,
but I don't think it's strictly necessary. Let me check out using
TextViewWithLeading.

[Charlie Harrison, 2017-05-12 15:44:49]

Thanks for the help with this Bernhard.

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

https://codereview.chromium.org/2879733002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/page_info_permission_row.xml:44:
android:lineSpacingExtra="6dp"
On 2017/05/12 12:31:32, Charlie Harrison wrote:
> On 2017/05/12 09:24:36, Bernhard Bauer wrote:
> > Where is this value coming from? The linked mocks didn't have any exact
> > measurements. In Material Design, spacing tends to be often defined in terms
> of
> > leading (the distance between subsequent baselines) instead of additional
line
> > spacing, soo... if that is the case here, you could use a
> > org.chromium.ui.widget.TextViewWithLeading to automatically use the correct
> line
> > spacing.
> 
> I used this value to align with the page_info_permission_unavailable_message,
> but I don't think it's strictly necessary. Let me check out using
> TextViewWithLeading.

Update: I've checked with UI who confirmed that we want:
Size: 14sp
Color: #8A000000 (secondary_text_default_material_light)
Leading: 20dp

I noticed that no other element here uses TextViewWithLeading, I assume just
using it here should be ok?

[Bernhard Bauer, 2017-05-12 15:51:00]

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

https://codereview.chromium.org/2879733002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/page_info_permission_row.xml:44:
android:lineSpacingExtra="6dp"
On 2017/05/12 15:44:48, Charlie Harrison wrote:
> On 2017/05/12 12:31:32, Charlie Harrison wrote:
> > On 2017/05/12 09:24:36, Bernhard Bauer wrote:
> > > Where is this value coming from? The linked mocks didn't have any exact
> > > measurements. In Material Design, spacing tends to be often defined in
terms
> > of
> > > leading (the distance between subsequent baselines) instead of additional
> line
> > > spacing, soo... if that is the case here, you could use a
> > > org.chromium.ui.widget.TextViewWithLeading to automatically use the
correct
> > line
> > > spacing.
> > 
> > I used this value to align with the
page_info_permission_unavailable_message,
> > but I don't think it's strictly necessary. Let me check out using
> > TextViewWithLeading.
> 
> Update: I've checked with UI who confirmed that we want:
> Size: 14sp
> Color: #8A000000 (secondary_text_default_material_light)
> Leading: 20dp
> 
> I noticed that no other element here uses TextViewWithLeading, I assume just
> using it here should be ok?

I think I would use it for the TextView above as well. TextViewWithLeading is
newer than this layout, so I think the only reason it's not used there is
because it wasn't around back then. For for purposes of consistency it would
probably be better to use it there as well.

[Charlie Harrison, 2017-05-12 15:52:30]

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

https://codereview.chromium.org/2879733002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/page_info_permission_row.xml:44:
android:lineSpacingExtra="6dp"
On 2017/05/12 15:51:00, Bernhard Bauer wrote:
> On 2017/05/12 15:44:48, Charlie Harrison wrote:
> > On 2017/05/12 12:31:32, Charlie Harrison wrote:
> > > On 2017/05/12 09:24:36, Bernhard Bauer wrote:
> > > > Where is this value coming from? The linked mocks didn't have any exact
> > > > measurements. In Material Design, spacing tends to be often defined in
> terms
> > > of
> > > > leading (the distance between subsequent baselines) instead of
additional
> > line
> > > > spacing, soo... if that is the case here, you could use a
> > > > org.chromium.ui.widget.TextViewWithLeading to automatically use the
> correct
> > > line
> > > > spacing.
> > > 
> > > I used this value to align with the
> page_info_permission_unavailable_message,
> > > but I don't think it's strictly necessary. Let me check out using
> > > TextViewWithLeading.
> > 
> > Update: I've checked with UI who confirmed that we want:
> > Size: 14sp
> > Color: #8A000000 (secondary_text_default_material_light)
> > Leading: 20dp
> > 
> > I noticed that no other element here uses TextViewWithLeading, I assume just
> > using it here should be ok?
> 
> I think I would use it for the TextView above as well. TextViewWithLeading is
> newer than this layout, so I think the only reason it's not used there is
> because it wasn't around back then. For for purposes of consistency it would
> probably be better to use it there as well.

Sounds good. Should that have leading 20dp as well?

[Bernhard Bauer, 2017-05-12 15:53:51]

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

https://codereview.chromium.org/2879733002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/page_info_permission_row.xml:44:
android:lineSpacingExtra="6dp"
On 2017/05/12 15:52:29, Charlie Harrison wrote:
> On 2017/05/12 15:51:00, Bernhard Bauer wrote:
> > On 2017/05/12 15:44:48, Charlie Harrison wrote:
> > > On 2017/05/12 12:31:32, Charlie Harrison wrote:
> > > > On 2017/05/12 09:24:36, Bernhard Bauer wrote:
> > > > > Where is this value coming from? The linked mocks didn't have any
exact
> > > > > measurements. In Material Design, spacing tends to be often defined in
> > terms
> > > > of
> > > > > leading (the distance between subsequent baselines) instead of
> additional
> > > line
> > > > > spacing, soo... if that is the case here, you could use a
> > > > > org.chromium.ui.widget.TextViewWithLeading to automatically use the
> > correct
> > > > line
> > > > > spacing.
> > > > 
> > > > I used this value to align with the
> > page_info_permission_unavailable_message,
> > > > but I don't think it's strictly necessary. Let me check out using
> > > > TextViewWithLeading.
> > > 
> > > Update: I've checked with UI who confirmed that we want:
> > > Size: 14sp
> > > Color: #8A000000 (secondary_text_default_material_light)
> > > Leading: 20dp
> > > 
> > > I noticed that no other element here uses TextViewWithLeading, I assume
just
> > > using it here should be ok?
> > 
> > I think I would use it for the TextView above as well. TextViewWithLeading
is
> > newer than this layout, so I think the only reason it's not used there is
> > because it wasn't around back then. For for purposes of consistency it would
> > probably be better to use it there as well.
> 
> Sounds good. Should that have leading 20dp as well?

Yes please.

[Charlie Harrison, 2017-05-12 15:58:24]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[Charlie Harrison, 2017-05-12 15:58:29]

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

https://codereview.chromium.org/2879733002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/page_info_permission_row.xml:44:
android:lineSpacingExtra="6dp"
On 2017/05/12 15:53:50, Bernhard Bauer wrote:
> On 2017/05/12 15:52:29, Charlie Harrison wrote:
> > On 2017/05/12 15:51:00, Bernhard Bauer wrote:
> > > On 2017/05/12 15:44:48, Charlie Harrison wrote:
> > > > On 2017/05/12 12:31:32, Charlie Harrison wrote:
> > > > > On 2017/05/12 09:24:36, Bernhard Bauer wrote:
> > > > > > Where is this value coming from? The linked mocks didn't have any
> exact
> > > > > > measurements. In Material Design, spacing tends to be often defined
in
> > > terms
> > > > > of
> > > > > > leading (the distance between subsequent baselines) instead of
> > additional
> > > > line
> > > > > > spacing, soo... if that is the case here, you could use a
> > > > > > org.chromium.ui.widget.TextViewWithLeading to automatically use the
> > > correct
> > > > > line
> > > > > > spacing.
> > > > > 
> > > > > I used this value to align with the
> > > page_info_permission_unavailable_message,
> > > > > but I don't think it's strictly necessary. Let me check out using
> > > > > TextViewWithLeading.
> > > > 
> > > > Update: I've checked with UI who confirmed that we want:
> > > > Size: 14sp
> > > > Color: #8A000000 (secondary_text_default_material_light)
> > > > Leading: 20dp
> > > > 
> > > > I noticed that no other element here uses TextViewWithLeading, I assume
> just
> > > > using it here should be ok?
> > > 
> > > I think I would use it for the TextView above as well. TextViewWithLeading
> is
> > > newer than this layout, so I think the only reason it's not used there is
> > > because it wasn't around back then. For for purposes of consistency it
would
> > > probably be better to use it there as well.
> > 
> > Sounds good. Should that have leading 20dp as well?
> 
> Yes please.

Done.

[commit-bot: I haz the power, 2017-05-12 15:59:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bernhard Bauer, 2017-05-12 16:05:17]

Thank you! LGTM.

[Charlie Harrison, 2017-05-12 16:09:45]

On 2017/05/12 16:05:17, Bernhard Bauer wrote:
> Thank you! LGTM.

No problem! Thanks again

[Charlie Harrison, 2017-05-12 16:09:49]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-05-12 16:09:49]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-05-12 16:10:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 16:51:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494605389616040,
"parent_rev": "d7585f5b58e656fe5e0902294b1ab6af2bca11f3", "commit_rev":
"cececce4d88ac1ca8b7a6e9f31597776ea151015"}

[commit-bot: I haz the power, 2017-05-12 16:51:37]

Description was changed from

==========
[subresource_filter] Add a subtitle for Android Page Info

This just uses a placeholder string for now.

BUG=689487
==========

to

==========
[subresource_filter] Add a subtitle for Android Page Info

This just uses a placeholder string for now.

BUG=689487

Review-Url: https://codereview.chromium.org/2879733002
Cr-Commit-Position: refs/heads/master@{#471339}
Committed:
https://chromium.googlesource.com/chromium/src/+/cececce4d88ac1ca8b7a6e9f3159...
==========

[commit-bot: I haz the power, 2017-05-12 16:51:38]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cececce4d88ac1ca8b7a6e9f3159...