Deduplicate Physical Web results with matching group IDs

Deduplicate Physical Web results with matching group IDs

When multiple Physical Web URLs are nearby and the broadcast URLs resolve
to the same or similar pages, only notify clients about the closest
instance of that URL. We identify similar pages via group ID, which is
constructed from the resolved URL (omitting any fragment), page title,
and page description.

BUG=681151
Review-Url: https://codereview.chromium.org/2669603003
Cr-Commit-Position: refs/heads/master@{#448350}
Committed: https://chromium.googlesource.com/chromium/src/+/1033377d9030c718bfaa1ecda9cbc01129fe7e7a

[mattreynolds, 2017-02-01 00:20:37]

Description was changed from

==========
Deduplicate Physical Web results with matching group IDs

When multiple Physical Web URLs are nearby and the broadcast URLs resolve
to the same or similar pages, only notify clients about the closest
instance of that URL. We identify similar pages via group ID, which is
constructed from the resolved URL (omitting any fragment), page title,
and page description.

BUG=681151
==========

to

==========
Deduplicate Physical Web results with matching group IDs

When multiple Physical Web URLs are nearby and the broadcast URLs resolve
to the same or similar pages, only notify clients about the closest
instance of that URL. We identify similar pages via group ID, which is
constructed from the resolved URL (omitting any fragment), page title,
and page description.

BUG=681151
==========

[mattreynolds, 2017-02-01 00:20:38]

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

[mattreynolds, 2017-02-01 00:20:56]

Hi Conley and Ian, PTAL

[cco3, 2017-02-01 00:23:50]

lgtm

[iankc, 2017-02-01 00:36:21]

lgtm

[mattreynolds, 2017-02-01 00:54:54]

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

[mattreynolds, 2017-02-01 00:54:55]

Hi Tommy, PTAL

[nyquist, 2017-02-01 07:18:01]

looks great except for test coverage

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

https://codereview.chromium.org/2669603003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:226:
return deduplicateByGroupId(urlInfos);
Given this is a business logic change, and this being seemingly unit-testable,
do you think you could add a test for this?

https://codereview.chromium.org/2669603003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:310:
continue;
Nit: Move this to previous line in Java. C++ and Java style guide doesn't match
here. Sorry :-/

[mattreynolds, 2017-02-01 19:29:15]

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

https://codereview.chromium.org/2669603003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:226:
return deduplicateByGroupId(urlInfos);
On 2017/02/01 07:18:01, nyquist wrote:
> Given this is a business logic change, and this being seemingly unit-testable,
> do you think you could add a test for this?

Done.

https://codereview.chromium.org/2669603003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:310:
continue;
On 2017/02/01 07:18:01, nyquist wrote:
> Nit: Move this to previous line in Java. C++ and Java style guide doesn't
match
> here. Sorry :-/

Oops, writing too much C++ lately. I decided to add braces instead since that
better matches the rest of the file.

[cco3, 2017-02-02 19:18:41]

https://codereview.chromium.org/2669603003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java
(right):

https://codereview.chromium.org/2669603003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:346:
// Construct two PwsResults with different information but identical groupids.
IMO, it'd be better to just add this to `testGetUrlSorts()`.  Sorting two
elements, you have a 50/50 shot of getting it right.

[mattreynolds, 2017-02-03 00:17:10]

https://codereview.chromium.org/2669603003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java
(right):

https://codereview.chromium.org/2669603003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:346:
// Construct two PwsResults with different information but identical groupids.
On 2017/02/02 19:18:41, cco3 wrote:
> IMO, it'd be better to just add this to `testGetUrlSorts()`.  Sorting two
> elements, you have a 50/50 shot of getting it right.

Done.

[nyquist, 2017-02-06 18:49:24]

lgtm

[mattreynolds, 2017-02-06 18:50:06]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2017-02-06 18:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from iankc@google.com,
cco3@chromium.org
Link to the patchset: https://codereview.chromium.org/2669603003/#ps60001
(title: "add another URL")

[commit-bot: I haz the power, 2017-02-06 18:50:39]

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-06 19:34:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486407006414520,
"parent_rev": "f002ed5e3050d1936d914d18495fcb20905820a4", "commit_rev":
"1033377d9030c718bfaa1ecda9cbc01129fe7e7a"}

[commit-bot: I haz the power, 2017-02-06 19:34:51]

Description was changed from

==========
Deduplicate Physical Web results with matching group IDs

When multiple Physical Web URLs are nearby and the broadcast URLs resolve
to the same or similar pages, only notify clients about the closest
instance of that URL. We identify similar pages via group ID, which is
constructed from the resolved URL (omitting any fragment), page title,
and page description.

BUG=681151
==========

to

==========
Deduplicate Physical Web results with matching group IDs

When multiple Physical Web URLs are nearby and the broadcast URLs resolve
to the same or similar pages, only notify clients about the closest
instance of that URL. We identify similar pages via group ID, which is
constructed from the resolved URL (omitting any fragment), page title,
and page description.

BUG=681151

Review-Url: https://codereview.chromium.org/2669603003
Cr-Commit-Position: refs/heads/master@{#448350}
Committed:
https://chromium.googlesource.com/chromium/src/+/1033377d9030c718bfaa1ecda9cb...
==========

[commit-bot: I haz the power, 2017-02-06 19:34:52]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1033377d9030c718bfaa1ecda9cb...