Introduce scope in WebappInfo.

Introduce scope in WebappInfo.

Committed: https://crrev.com/1e032287661577637786613d91e52be99fe6dc65
Cr-Commit-Position: refs/heads/master@{#406272}

[Xi Han, 2016-07-14 21:58:01]

Hi Peter, could you please take a look? Thanks!

[Xi Han, 2016-07-14 21:58:43]

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

[Xi Han, 2016-07-14 21:58:44]

Hi Peter, could you please take a look? Thanks!

[Xi Han, 2016-07-15 13:46:03]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-07-15 14:19:56]

https://codereview.chromium.org/2147323002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2147323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:318:
ShortcutHelper.getScopeFromUrl(uri().toString()));
This needs to be updated?

The scope parsing in WebappInfo#create() should default to the return value of
ShortcutHelper.getScopeFromUrl(uri().toString())

[Xi Han, 2016-07-15 15:15:17]

Hi Peter, PTAL, thanks!

https://codereview.chromium.org/2147323002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2147323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:318:
ShortcutHelper.getScopeFromUrl(uri().toString()));
On 2016/07/15 14:19:56, pkotwicz wrote:
> This needs to be updated?
> 
> The scope parsing in WebappInfo#create() should default to the return value of
> ShortcutHelper.getScopeFromUrl(uri().toString())

Good catch, thanks!

[pkotwicz, 2016-07-15 15:38:31]

https://codereview.chromium.org/2147323002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2147323002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:331:
ShortcutHelper.getScopeFromUrl(uri().toString()));
Shouldn't this line be changed to scope().toString() ?

[Xi Han, 2016-07-15 17:23:38]

Hi Peter, PTAL, thanks!

https://codereview.chromium.org/2147323002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2147323002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:331:
ShortcutHelper.getScopeFromUrl(uri().toString()));
On 2016/07/15 15:38:31, pkotwicz wrote:
> Shouldn't this line be changed to scope().toString() ?

Thanks for catching this. I also updated the create() function and moved the
null check for scope to the create() function with many parameters.

[pkotwicz, 2016-07-15 17:28:14]

LGTM

Sorry that I did not do not add WebappInfo#mScopeUrl when I plumbed scope from
content

[Xi Han, 2016-07-15 17:33:00]

On 2016/07/15 17:28:14, pkotwicz wrote:
> LGTM
> 
> Sorry that I did not do not add WebappInfo#mScopeUrl when I plumbed scope from
> content

That is ok, scope is not the only one that we didn't add to finish CL
(https://codereview.chromium.org/2124513002/#ps260001):P

[Xi Han, 2016-07-15 17:34:55]

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

[Xi Han, 2016-07-15 17:34:56]

Hi Dominick,
this is another CL to prepare data for the
CL(https://codereview.chromium.org/2124513002/).
Could you please take a look? Thanks!

[dominickn, 2016-07-18 06:00:04]

Is there a need for the scope to be a Uri? E.g. the manifest_url was just a
plain string before.

https://codereview.chromium.org/2147323002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2147323002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:176:
Uri scopeUri = Uri.parse(scope != null && !scope.isEmpty() ? scope
This is rather cryptic. Can you take out the ternary and just use a conditional?

if (scope == null || scope.isEmpty())
  scope = ShortcutHelper.getScopeFromUrl(url);
Uri scopeUri = Uri.parse(scope);

[Xi Han, 2016-07-18 14:03:03]

I guess it is better to use the Uri for a Url String, since we can know it's
host or path if needed. So I changed the web_manifest_url to Uri in the other CL
to keep consistence too. PTAL, thanks!

https://codereview.chromium.org/2147323002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2147323002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:176:
Uri scopeUri = Uri.parse(scope != null && !scope.isEmpty() ? scope
On 2016/07/18 06:00:04, dominickn wrote:
> This is rather cryptic. Can you take out the ternary and just use a
conditional?
> 
> if (scope == null || scope.isEmpty())
>   scope = ShortcutHelper.getScopeFromUrl(url);
> Uri scopeUri = Uri.parse(scope);
>   

Done.

[dominickn, 2016-07-19 03:08:50]

lgtm

[Xi Han, 2016-07-19 14:01:24]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2016-07-19 14:01:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2147323002/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-07-19 14:02:16]

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-07-19 14:45:35]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-07-19 14:45:53]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 14:46:58]

Description was changed from

==========
Introduce scope in WebappInfo.
==========

to

==========
Introduce scope in WebappInfo.

Committed: https://crrev.com/1e032287661577637786613d91e52be99fe6dc65
Cr-Commit-Position: refs/heads/master@{#406272}
==========

[commit-bot: I haz the power, 2016-07-19 14:46:59]

Patchset 7 (id:??) landed as
https://crrev.com/1e032287661577637786613d91e52be99fe6dc65
Cr-Commit-Position: refs/heads/master@{#406272}