Remove ListUrlsActivity

Remove ListUrlsActivity

This change removes the ListUrlsActivity from clank, which was used
for notifications for physical-web. Because notifications have been
phased out, we are removing this code because it is no longer needed.

BUG=680747
Review-Url: https://codereview.chromium.org/2651553005
Cr-Commit-Position: refs/heads/master@{#448145}
Committed: https://chromium.googlesource.com/chromium/src/+/f872c9f20b0d823b06e565c6e818b0f071e0fa37

[iankc, 2017-01-24 23:42:18]

Description was changed from

==========
Remove ListUrlsActivity

This change removes the ListUrlsActivity from clank, which was used
for notifications for physical-web. Because notifications have been
phased out, we are removing this code because it is no longer needed.

BUG=680747
==========

to

==========
Remove ListUrlsActivity

This change removes the ListUrlsActivity from clank, which was used
for notifications for physical-web. Because notifications have been
phased out, we are removing this code because it is no longer needed.

BUG=680747
==========

[iankc, 2017-01-24 23:42:18]

iankc@google.com changed reviewers:
+ cco3@chromium.org, mattreynolds@chromium.org

[cco3, 2017-01-24 23:51:10]

https://codereview.chromium.org/2651553005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):

https://codereview.chromium.org/2651553005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:68:
public static final int NOTIFICATION_REFERER = 1;
Let's put these above the private ones

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

https://codereview.chromium.org/2651553005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:554:
private void createNotification() {
You should sync...none of this stuff is here anymore.

[iankc, 2017-01-25 20:05:49]

Fixed comments

[mattreynolds, 2017-01-25 20:11:17]

lgtm

[cco3, 2017-01-25 20:12:20]

lgtm

[cco3, 2017-01-25 20:12:46]

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

[cco3, 2017-01-25 20:12:46]

Hi Tommy, this is another cleanup change removing a couple of classes.

[nyquist, 2017-01-25 21:13:01]

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

https://codereview.chromium.org/2651553005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:66:
return null;
This seems to be passed directly to startActivity(). Are we sure we want to
return null here? Should we instead remove the call to startActivity() or remove
the onClick-listener or even the button?

[iankc, 2017-01-25 22:51:29]

fixed nyquists comments!

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

https://codereview.chromium.org/2651553005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:66:
return null;
On 2017/01/25 21:13:00, nyquist wrote:
> This seems to be passed directly to startActivity(). Are we sure we want to
> return null here? Should we instead remove the call to startActivity() or
remove
> the onClick-listener or even the button?

Yeah, removing the call to 'startActivity()' would be good. I'll do that now.
Thanks!

https://codereview.chromium.org/2651553005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebOptInActivity.java:66:
return null;
On 2017/01/25 21:13:00, nyquist wrote:
> This seems to be passed directly to startActivity(). Are we sure we want to
> return null here? Should we instead remove the call to startActivity() or
remove
> the onClick-listener or even the button?

Done.

[nyquist, 2017-01-31 21:44:33]

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:29:
public static final int NOTIFICATION_REFERER = 1;
IS this and OPTIN_REFERER still in use?

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java_so...
File chrome/android/java_sources.gni (left):

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java_so...
chrome/android/java_sources.gni:737:
"java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java",
Isn't this also listed in the manifest?
//chrome/android/java/AndroidManifest.xml

[cco3, 2017-01-31 21:53:18]

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:29:
public static final int NOTIFICATION_REFERER = 1;
On 2017/01/31 21:44:33, nyquist wrote:
> IS this and OPTIN_REFERER still in use?

No, they aren't in use.  What is the right course of action here?  We should
keep the value assigned to the other referers the same, correct?  Do we need to
mark anything in histograms as deprecated?

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java_so...
File chrome/android/java_sources.gni (left):

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java_so...
chrome/android/java_sources.gni:737:
"java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java",
On 2017/01/31 21:44:33, nyquist wrote:
> Isn't this also listed in the manifest?
> //chrome/android/java/AndroidManifest.xml

Yes, good call.

[iankc, 2017-01-31 22:12:29]

Removed the activity from the Manifest

[nyquist, 2017-01-31 22:17:48]

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):

https://codereview.chromium.org/2651553005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:29:
public static final int NOTIFICATION_REFERER = 1;
On 2017/01/31 21:53:18, cco3 wrote:
> On 2017/01/31 21:44:33, nyquist wrote:
> > IS this and OPTIN_REFERER still in use?
> 
> No, they aren't in use.  What is the right course of action here?  We should
> keep the value assigned to the other referers the same, correct?  Do we need
to
> mark anything in histograms as deprecated?

There's a way for marking histograms obsolete. I don't know about specific
values though. I'd check with the team working on that.

If you want, it's OK with me for you to submit this and do that as a followup
though. Maybe just add a comment about it in this CL, and make them private?

[iankc, 2017-02-01 00:40:34]

Made the obsolete metrics private and added a summary in the histogram.xml

[nyquist, 2017-02-01 07:12:42]

I think you also need a review from an owner of histograms.

https://codereview.chromium.org/2651553005/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):

https://codereview.chromium.org/2651553005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:29:
private static final int NOTIFICATION_REFERER = 1;
Could you mention that these two are not in use anymore?
Also, are there bots or tools that fail on unused private fields?
I wonder whether we maybe should just have them as a comment instead?
See a similar example here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[iankc, 2017-02-01 18:28:48]

iankc@google.com changed reviewers:
+ rkaplow@chromium.org

[iankc, 2017-02-01 18:28:48]

Hello rkaplow,

We're cleaning up our codebase after phasing out a notification feature. We're
marking two of out metrics as obselete:

REFERER_NOTIFICATION
REFERER_OPTIN

Could you take a look? Thanks!

[rkaplow, 2017-02-01 20:25:24]

lgtm

https://codereview.chromium.org/2651553005/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2651553005/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:101457: +  <summary>
Instead, can you switch the labels to REFERER_X (obsolete)

[iankc, 2017-02-01 20:29:44]

Made the old fields comments instead. Got the LGTM from owners of Histogram.xml

https://codereview.chromium.org/2651553005/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):

https://codereview.chromium.org/2651553005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:29:
private static final int NOTIFICATION_REFERER = 1;
On 2017/02/01 07:12:42, nyquist wrote:
> Could you mention that these two are not in use anymore?
> Also, are there bots or tools that fail on unused private fields?
> I wonder whether we maybe should just have them as a comment instead?
> See a similar example here:
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Done.

[nyquist, 2017-02-01 21:06:08]

chrome lgtm

https://codereview.chromium.org/2651553005/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):

https://codereview.chromium.org/2651553005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:29:
//Obsolete; don't use:  NOTIFICATION_REFERER = 1;
Nit: Missing space after //

[iankc, 2017-02-01 21:26:06]

The CQ bit was checked by iankc@google.com

[iankc, 2017-02-01 21:26:07]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org,
mattreynolds@chromium.org, rkaplow@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2651553005/#ps200001
(title: "Fixed nit")

[commit-bot: I haz the power, 2017-02-01 21:26:28]

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-02-01 21:28:33]

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

[commit-bot: I haz the power, 2017-02-01 21:28:35]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iankc, 2017-02-03 20:18:12]

The CQ bit was checked by iankc@google.com

[iankc, 2017-02-03 20:18:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, rkaplow@chromium.org, cco3@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2651553005/#ps220001
(title: "Syncing with Master")

[commit-bot: I haz the power, 2017-02-03 20:18:58]

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-02-03 20:53:49]

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

[commit-bot: I haz the power, 2017-02-03 20:53:50]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[iankc, 2017-02-03 22:32:21]

The CQ bit was checked by iankc@google.com

[iankc, 2017-02-03 22:32:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, rkaplow@chromium.org, cco3@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2651553005/#ps240001
(title: "fixing patch error")

[commit-bot: I haz the power, 2017-02-03 22:33:45]

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-02-03 23:11:37]

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

[commit-bot: I haz the power, 2017-02-03 23:11:38]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[iankc, 2017-02-04 00:11:41]

The CQ bit was checked by iankc@google.com

[iankc, 2017-02-04 00:11:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, rkaplow@chromium.org, cco3@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2651553005/#ps260001
(title: "Removing ListUrlsActivity Tests")

[commit-bot: I haz the power, 2017-02-04 00:12:24]

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-02-04 02:11:33]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1486167101857600,
"parent_rev": "4eb5e9b236c7eb1fb015f69a34dc7890cc706979", "commit_rev":
"f872c9f20b0d823b06e565c6e818b0f071e0fa37"}

[commit-bot: I haz the power, 2017-02-04 02:12:30]

Description was changed from

==========
Remove ListUrlsActivity

This change removes the ListUrlsActivity from clank, which was used
for notifications for physical-web. Because notifications have been
phased out, we are removing this code because it is no longer needed.

BUG=680747
==========

to

==========
Remove ListUrlsActivity

This change removes the ListUrlsActivity from clank, which was used
for notifications for physical-web. Because notifications have been
phased out, we are removing this code because it is no longer needed.

BUG=680747

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

[commit-bot: I haz the power, 2017-02-04 02:12:33]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/f872c9f20b0d823b06e565c6e818...