Add a Learn More link to the Physical Web opt-in activity

Add a Learn More link to the Physical Web opt-in activity

BUG=598106
Committed: https://crrev.com/e00ccbd44a59caa3d56a4af2b5393ef9b288ad94
Cr-Commit-Position: refs/heads/master@{#391706}

[mattreynolds, 2016-05-03 20:05:12]

mattreynolds@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mmocny@chromium.org

[mattreynolds, 2016-05-03 20:05:13]

Hi Dan, thanks for the ChromeCustomTab suggestion.  PTAL

[gone, 2016-05-04 00:43:33]

dfalcantara@chromium.org changed reviewers:
+ ianwen@chromium.org

[gone, 2016-05-04 00:43:33]

+Ian for checking the Custom Tab triggering logic.

[gone, 2016-05-04 00:43:38]

+Ian for checking the Custom Tab triggering logic.

[gone, 2016-05-04 00:46:43]

Can you post a screenshot of what this looks like?

[mattreynolds, 2016-05-04 00:55:48]

On 2016/05/04 00:46:43, dfalcantara wrote:
> Can you post a screenshot of what this looks like?

Sure, I added screenshots to the bug

[gone, 2016-05-04 01:02:37]

Would it make sense to tack the "Learn more" link onto the other TextView, then
use a Spannable to allow clicking on it?  That's the pattern we use in infobars:

https://folio.googleplex.com/infobarmasterspecdraft#%2F02%20Button%20stacking...

Doing something like this also avoids trying to get the right spacing between
the Learn More link and the TextView above it; because the Learn More text would
already be a part of the TextView, it's automatically spaced correctly.

[mattreynolds, 2016-05-04 21:31:56]

On 2016/05/04 01:02:37, dfalcantara wrote:
> Would it make sense to tack the "Learn more" link onto the other TextView,
then
> use a Spannable to allow clicking on it?  That's the pattern we use in
infobars:
> 
>
https://folio.googleplex.com/infobarmasterspecdraft#%2F02%20Button%20stacking...
> 
> Doing something like this also avoids trying to get the right spacing between
> the Learn More link and the TextView above it; because the Learn More text
would
> already be a part of the TextView, it's automatically spaced correctly.

Thanks, this seems like a cleaner and more flexible solution.

[gone, 2016-05-04 21:41:46]

lgtm on my end.  Thanks!

[Ian Wen, 2016-05-04 22:20:26]

I noticed two nits. :)

https://chromiumcodereview.appspot.com/1947693002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java
(right):

https://chromiumcodereview.appspot.com/1947693002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:90:
intent.putExtras(extras);
You should use intent.putExtra(EXTRA_CUSTOM_TABS_SESSION, null) here.

https://chromiumcodereview.appspot.com/1947693002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:91:
PhysicalWebOptInActivity.this.startActivity(intent, null);
Why not calling startActivity(intent) without the null argument?

[mattreynolds, 2016-05-04 22:44:32]

Thanks Dan and Ian!

https://chromiumcodereview.appspot.com/1947693002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java
(right):

https://chromiumcodereview.appspot.com/1947693002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:90:
intent.putExtras(extras);
On 2016/05/04 22:20:26, Ian Wen wrote:
> You should use intent.putExtra(EXTRA_CUSTOM_TABS_SESSION, null) here.

Done. putExtras(String, null) is ambiguous so I had to declare session
explicitly.

https://chromiumcodereview.appspot.com/1947693002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:91:
PhysicalWebOptInActivity.this.startActivity(intent, null);
On 2016/05/04 22:20:26, Ian Wen wrote:
> Why not calling startActivity(intent) without the null argument?

Done.

[mattreynolds, 2016-05-04 22:45:40]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-05-04 22:45:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1947693002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2016-05-04 22:46:23]

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

[commit-bot: I haz the power, 2016-05-05 00:06:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-05 00:08:28]

Description was changed from

==========
Add a Learn More link to the Physical Web opt-in activity

BUG=598106
==========

to

==========
Add a Learn More link to the Physical Web opt-in activity

BUG=598106

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

[commit-bot: I haz the power, 2016-05-05 00:08:29]

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