Add Physical Web opt-in dialog

Add Physical Web opt-in dialog

Display a dialog describing the Physical Web and inviting the user to
enable or decline the feature.

BUG=529962
Committed: https://crrev.com/f8d35b46aa73c475c62d2c1d74bc8f708c014def
Cr-Commit-Position: refs/heads/master@{#367027}

[cco3, 2015-12-21 23:34:04]

cco3@chromium.org changed reviewers:
+ cco3@chromium.org

[cco3, 2015-12-21 23:34:05]

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java
(right):

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:111:
int value = getOptInNotifyCount(context);
I would prefer you replace this with the direct operation (getInt)

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:275:
if (PhysicalWeb.featureIsEnabled() && PhysicalWeb.isOnboarding(mContext)) {
Under what circumstances would you need to check featureIsEnabled?

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:287:
if (PhysicalWeb.isPhysicalWebPreferenceEnabled(mContext)) {
Same for this check

[mattreynolds, 2015-12-22 00:21:56]

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java
(right):

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:111:
int value = getOptInNotifyCount(context);
On 2015/12/21 23:34:05, cco3 wrote:
> I would prefer you replace this with the direct operation (getInt)

Done (see issue 1526093003)

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:275:
if (PhysicalWeb.featureIsEnabled() && PhysicalWeb.isOnboarding(mContext)) {
On 2015/12/21 23:34:05, cco3 wrote:
> Under what circumstances would you need to check featureIsEnabled?

It's not needed, I'll remove it (see issue 1526093003).  It shouldn't be
possible to have a non-empty URL list when the feature is disabled.

https://codereview.chromium.org/1526323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:287:
if (PhysicalWeb.isPhysicalWebPreferenceEnabled(mContext)) {
On 2015/12/21 23:34:05, cco3 wrote:
> Same for this check

It's possible that the user disabled the physical web via privacy options at the
same time we received an onFound event, this check prevents us from displaying
the notification when the preference is disabled.

[mattreynolds, 2015-12-22 00:34:59]

mattreynolds@chromium.org changed reviewers:
+ newt@chromium.org

[mattreynolds, 2015-12-22 00:35:00]

Hi Newton, please take a look.

UX mocks: go/pw-user-stories (slide 3, second image)

Screenshots are linked in the bug.

This CL includes changes from issue 1526093003 and will need to be rebased once
that lands. Except where noted, the changes in UrlManager.java and
PhysicalWeb.java have already been reviewed.

I've modified the button bar style from the mocks as the original design didn't
work as well in landscape.

[mattreynolds, 2015-12-22 00:36:42]

https://codereview.chromium.org/1526323003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1526323003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:248:
Intent intent = new Intent(mContext, PhysicalWebOptInActivity.class);
Modified from issue 1526093003 (previously the intent would open the
ListUrlsActivity instead)

[gone, 2015-12-28 22:27:03]

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

[gone, 2015-12-28 22:27:03]

Preliminary comments.

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/physical_web_optin.xml (right):

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/physical_web_optin.xml:8: 
This looks broken because you're assuming that the bottom bar will always be
exactly 56dp high.  Have you tried looking at your layout in German, or some
other language that has really long strings?  Temporarily plug in long ones and
see how your layout deals with it.

That said, why not use a LinearLayout (e.g.)?

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java
(right):

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:20:
private static final String TAG = "PhysicalWeb";
Don't add TAGs unless you actually use them for logs.

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:53:
private Intent createListUrlsIntent() {
nit: static?

[mattreynolds, 2015-12-28 23:11:22]

Thanks Dan!

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/physical_web_optin.xml (right):

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/physical_web_optin.xml:8: 
On 2015/12/28 22:27:03, dfalcantara wrote:
> This looks broken because you're assuming that the bottom bar will always be
> exactly 56dp high.  Have you tried looking at your layout in German, or some
> other language that has really long strings?  Temporarily plug in long ones
and
> see how your layout deals with it.
> 
> That said, why not use a LinearLayout (e.g.)?

I couldn't find a way to use a LinearLayout that would center the content
correctly while also scrolling on small displays and aligning the button bar to
the bottom of the view. I borrowed heavily from fre_choose_account.xml which
uses the same hard-coded 56dp button bar height.

Plugging in longer strings for "No Thanks" and "Enable" (padded out to 32 chars)
causes the button label to wrap but otherwise renders fine. I linked some
screenshots from bug 529962.

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java
(right):

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:20:
private static final String TAG = "PhysicalWeb";
On 2015/12/28 22:27:03, dfalcantara wrote:
> Don't add TAGs unless you actually use them for logs.

Removed

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:53:
private Intent createListUrlsIntent() {
On 2015/12/28 22:27:03, dfalcantara wrote:
> nit: static?

Done.

[gone, 2015-12-28 23:31:41]

lgtm

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/physical_web_optin.xml (right):

https://codereview.chromium.org/1526323003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/physical_web_optin.xml:8: 
On 2015/12/28 23:11:22, mattreynolds wrote:
> On 2015/12/28 22:27:03, dfalcantara wrote:
> > This looks broken because you're assuming that the bottom bar will always be
> > exactly 56dp high.  Have you tried looking at your layout in German, or some
> > other language that has really long strings?  Temporarily plug in long ones
> and
> > see how your layout deals with it.
> > 
> > That said, why not use a LinearLayout (e.g.)?
> 
> I couldn't find a way to use a LinearLayout that would center the content
> correctly while also scrolling on small displays and aligning the button bar
to
> the bottom of the view. I borrowed heavily from fre_choose_account.xml which
> uses the same hard-coded 56dp button bar height.
> 
> Plugging in longer strings for "No Thanks" and "Enable" (padded out to 32
chars)
> causes the button label to wrap but otherwise renders fine. I linked some
> screenshots from bug 529962.

For future reference, the original CL that changed fre_choose_account.xml to
have the 57dp wonkiness:
https://chrome-internal-review.googlesource.com/#/c/166785/4/java/apps/chrome...

Given that there's precedence, and given that it works with really long strings
(thanks for checking), this part is fine.  I checked existing translations for
"No thanks" and they don't hit anywhere near 32 chars.

[mattreynolds, 2015-12-28 23:36:25]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2015-12-28 23:37:34]

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

[commit-bot: I haz the power, 2015-12-29 00:16:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-29 00:17:21]

Description was changed from

==========
Add Physical Web opt-in dialog

Display a dialog describing the Physical Web and inviting the user to
enable or decline the feature.

BUG=529962
==========

to

==========
Add Physical Web opt-in dialog

Display a dialog describing the Physical Web and inviting the user to
enable or decline the feature.

BUG=529962

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

[commit-bot: I haz the power, 2015-12-29 00:17:22]

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