Canonicalize URLs prior to comparing in ManifestUpgradeDetector

Canonicalize URLs prior to comparing in ManifestUpgradeDetector

Chrome and the WebAPK server parse URLs differently. For instance, the WebAPK
server currently does not do URL escaping. The WebAPK server populates the
WebAPK's Android Manifest with the URLs that it has parsed. This CL makes
ManifestUpgradeDetector canonicalize URLs before comparing them.

BUG=638077
TEST=ManifestUpgradeDetectorTest.*

Committed: https://crrev.com/c0635b4ff44c7ccad4c9c94740dca27945af88cf
Cr-Commit-Position: refs/heads/master@{#419863}

[pkotwicz, 2016-09-15 22:14:39]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-09-15 22:14:52]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org

[pkotwicz, 2016-09-15 22:14:53]

Xi can you please take a look?

[pkotwicz, 2016-09-15 22:17:36]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2016-09-15 22:24:15]

Patchset #1 (id:40001) has been deleted

[Xi Han, 2016-09-16 17:06:01]

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:160:
"/chrome/test/data/banners/manifest_one_icon_%74%65%73%74_page.html");
What is "%74%65%73%74"?

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:181:
"/chrome/test/data/banners/manifest_one_icon_%63%75%74%65_page.html");
The only different thing of the above two tests are these % numbers, and both
CreateData.start_url looks the same as the WEBAPK_START_URL (except the
%numbers).  Why does one need to upgrade while the other one doesn't?

[pkotwicz, 2016-09-16 17:54:36]

Patchset #2 (id:80001) has been deleted

[pkotwicz, 2016-09-16 17:56:07]

Patchset #2 (id:100001) has been deleted

[pkotwicz, 2016-09-16 17:57:01]

I added some explanatory comments to the tests. Hopefully they make things
clearer

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:160:
"/chrome/test/data/banners/manifest_one_icon_%74%65%73%74_page.html");
URL canonicalizat

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:160:
"/chrome/test/data/banners/manifest_one_icon_%74%65%73%74_page.html");
When gurl.h canonicalizes a URL:
- it escapes characters such as space (%20).
  "first second.html" is canonicalized as "first%20.html".
- it undoes the escaping for characters which do not need to be escaped such as
't'.
  "%74.html" is canonicalized as "t.html"

"%74%65%73%74" is unescaped as "test"

[Xi Han, 2016-09-16 18:51:36]

lgtm with nits.

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2341163002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:160:
"/chrome/test/data/banners/manifest_one_icon_%74%65%73%74_page.html");
On 2016/09/16 17:57:01, pkotwicz wrote:
> When gurl.h canonicalizes a URL:
> - it escapes characters such as space (%20).
>   "first second.html" is canonicalized as "first%20.html".
> - it undoes the escaping for characters which do not need to be escaped such
as
> 't'.
>   "%74.html" is canonicalized as "t.html"
> 
> "%74%65%73%74" is unescaped as "test"

Thanks for the explanation. The example of (%20) is more common seeing, but I am
ok with the %74 for "t" as the test case.

https://codereview.chromium.org/2341163002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2341163002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:219:
protected boolean urlsMatchIgnoringFragments(String url1, String url2) {
Please add a comment that this function canonicalizes URLs prior to comparing.

[pkotwicz, 2016-09-19 23:42:24]

Patchset #3 (id:140001) has been deleted

[pkotwicz, 2016-09-19 23:42:41]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2016-09-19 23:42:42]

Dominick, can you please take a look?

[dominickn, 2016-09-20 00:25:02]

Looks pretty good - just some minor comments.

https://codereview.chromium.org/2341163002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2341163002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:182:
boolean needsUpgrade = needsUpgrade(fetchedData);
Don't have variables and methods with the same name - it's needlessly confusing.
Change either the name of the method or the name of the variable.

https://codereview.chromium.org/2341163002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:195:
boolean scopeMatch = urlsMatchIgnoringFragments(
Nit: inline this calculation into the if condition so it's consistent with the
check above.

https://codereview.chromium.org/2341163002/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2341163002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:34:
private static final String WEBAPK_SCOPE_URL =
Nit: does this not fit on one line? Looking at the image-512x.png line below, it
should easily fit.

https://codereview.chromium.org/2341163002/diff/160001/chrome/test/data/banne...
File chrome/test/data/banners/manifest_one_icon.json (right):

https://codereview.chromium.org/2341163002/diff/160001/chrome/test/data/banne...
chrome/test/data/banners/manifest_one_icon.json:2: "name": "Manifest test app",
Instead of creating new test files, can you use
chrome/test/data/webapps/manifest_test_page.html and
chrome/test/data/webapps/manifest.json? They seem to be essentially identical
for your purposes here.

At some point I am going to go and consolidate all of these test files for
webapps and banners into one directory.

[pkotwicz, 2016-09-20 02:34:10]

Patchset #4 (id:180001) has been deleted

[pkotwicz, 2016-09-20 02:34:29]

Dominick, can you please take a look?

https://codereview.chromium.org/2341163002/diff/160001/chrome/test/data/banne...
File chrome/test/data/banners/manifest_one_icon.json (right):

https://codereview.chromium.org/2341163002/diff/160001/chrome/test/data/banne...
chrome/test/data/banners/manifest_one_icon.json:2: "name": "Manifest test app",
Thank you for the suggestion. Unfortunately,
chrome/test/data/webapps/manifest_test_page.html does not have a service worker
and the test page needs a service worker for my new tests.

[dominickn, 2016-09-20 02:41:25]

lgtm

https://codereview.chromium.org/2341163002/diff/160001/chrome/test/data/banne...
File chrome/test/data/banners/manifest_one_icon.json (right):

https://codereview.chromium.org/2341163002/diff/160001/chrome/test/data/banne...
chrome/test/data/banners/manifest_one_icon.json:2: "name": "Manifest test app",
On 2016/09/20 02:34:29, pkotwicz wrote:
> Thank you for the suggestion. Unfortunately,
> chrome/test/data/webapps/manifest_test_page.html does not have a service
worker
> and the test page needs a service worker for my new tests.

Acknowledged.

[pkotwicz, 2016-09-20 14:37:39]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-09-20 14:37:39]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org
Link to the patchset: https://codereview.chromium.org/2341163002/#ps200001
(title: "Merge branch 'master' into url_canonicalize")

[commit-bot: I haz the power, 2016-09-20 14:37:57]

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, 2016-09-20 16:16:30]

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

[commit-bot: I haz the power, 2016-09-20 16:16:31]

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...)

[pkotwicz, 2016-09-20 19:52:29]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-09-20 19:52:59]

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, 2016-09-20 21:57:20]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2016-09-20 22:00:25]

Description was changed from

==========
Canonicalize URLs prior to comparing in ManifestUpgradeDetector

Chrome and the WebAPK server parse URLs differently. For instance, the WebAPK
server currently does not do URL escaping. The WebAPK server populates the
WebAPK's Android Manifest with the URLs that it has parsed. This CL makes
ManifestUpgradeDetector canonicalize URLs before comparing them.

BUG=638077
TEST=ManifestUpgradeDetectorTest.*
==========

to

==========
Canonicalize URLs prior to comparing in ManifestUpgradeDetector

Chrome and the WebAPK server parse URLs differently. For instance, the WebAPK
server currently does not do URL escaping. The WebAPK server populates the
WebAPK's Android Manifest with the URLs that it has parsed. This CL makes
ManifestUpgradeDetector canonicalize URLs before comparing them.

BUG=638077
TEST=ManifestUpgradeDetectorTest.*

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

[commit-bot: I haz the power, 2016-09-20 22:00:26]

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