Use Web Manifest when creating a shortcut on Chrome Android.

Use Web Manifest when creating a shortcut on Chrome Android.

This is using the short_name and the name set in the Manifest.

BUG=366145
Committed: https://crrev.com/b225ae560fea0648b61fad36b5972221a7f5c47d
Cr-Commit-Position: refs/heads/master@{#295256}

[mlamouri (slow - plz ping), 2014-09-16 21:05:37]

mlamouri@chromium.org changed reviewers:
+ dfalcantara@chromium.org, miguelg@chromium.org

[mlamouri (slow - plz ping), 2014-09-16 21:05:38]

This is a pretty simply change, all the logic is happeninig in the background:
if there is no manifest or if there is an error, the callback (OnGetManifest)
will be called with an empty manifest. If the manifest is correctly fetched and
parsed, the callback will be run with the manifest correctly sanitized (wrt to
IPC).

[gone, 2014-09-17 06:10:26]

On 2014/09/16 21:05:38, Mounir Lamouri wrote:
> This is a pretty simply change, all the logic is happeninig in the background:
> if there is no manifest or if there is an error, the callback (OnGetManifest)
> will be called with an empty manifest. If the manifest is correctly fetched
and
> parsed, the callback will be run with the manifest correctly sanitized (wrt to
> IPC).

lgtm

[Mike West, 2014-09-17 07:56:59]

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

[Mike West, 2014-09-17 07:57:00]

Drive-by LGTM with a question about potential future work.

https://codereview.chromium.org/573073002/diff/1/chrome/browser/android/short...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/573073002/diff/1/chrome/browser/android/short...
chrome/browser/android/shortcut_helper.cc:55: Send(new
ChromeViewMsg_GetWebApplicationInfo(routing_id()));
Does the application info need to be gathered before you can execute
GetManifest? If not, perhaps you could run the two async requests in parallel,
similar to what we do in 'BrowsingDataRemover::AllDone'.

https://codereview.chromium.org/573073002/diff/1/chrome/browser/android/short...
chrome/browser/android/shortcut_helper.cc:85: JNIEnv* env =
base::android::AttachCurrentThread();
Nit: For clarity, I'd suggest breaking this block out into something like an
"OnInitialized" method in this class. It's not conceptually part of getting the
manifest data, it's just the finalization of the initialization work that you
happen to be able to do only after getting the manifest data.

[mlamouri (slow - plz ping), 2014-09-17 08:47:11]

Comments applied.

I've also added support for |start_url|, given that it has landed.

Miguel, PTAL.

https://codereview.chromium.org/573073002/diff/1/chrome/browser/android/short...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/573073002/diff/1/chrome/browser/android/short...
chrome/browser/android/shortcut_helper.cc:55: Send(new
ChromeViewMsg_GetWebApplicationInfo(routing_id()));
On 2014/09/17 07:57:00, Mike West wrote:
> Does the application info need to be gathered before you can execute
> GetManifest? If not, perhaps you could run the two async requests in parallel,
> similar to what we do in 'BrowsingDataRemover::AllDone'.

We probably could. We want to make sure the manifest values overwrite whatever
we have from the meta tags and the meta tags overwrite whatever we got from the
WebContents but that should not be a big issue. This said, it would likely make
the code harder to read and I'm not certain it's worth it. Getting the
WebAppInformation is a basic bounce to the renderer process. Surely, those two
IPC messages could take a while but more than likely insignificant compared to
the Manifest work.

Though, there are a lot of improvement we could do with regards to the async
work (and the UI that comes with it). I will keep that in mind when I will
tackle that. Thanks for the pointer!

https://codereview.chromium.org/573073002/diff/1/chrome/browser/android/short...
chrome/browser/android/shortcut_helper.cc:85: JNIEnv* env =
base::android::AttachCurrentThread();
On 2014/09/17 07:57:00, Mike West wrote:
> Nit: For clarity, I'd suggest breaking this block out into something like an
> "OnInitialized" method in this class. It's not conceptually part of getting
the
> manifest data, it's just the finalization of the initialization work that you
> happen to be able to do only after getting the manifest data.

Done.

[Miguel Garcia, 2014-09-17 09:02:42]

lgtm with a question

https://codereview.chromium.org/573073002/diff/20001/chrome/browser/android/s...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/573073002/diff/20001/chrome/browser/android/s...
chrome/browser/android/shortcut_helper.cc:76: base::Unretained(this)));
why base::unretained? wouldn't it be better to use a WeakPtrFactory?

[mlamouri (slow - plz ping), 2014-09-17 10:51:04]

https://codereview.chromium.org/573073002/diff/20001/chrome/browser/android/s...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/573073002/diff/20001/chrome/browser/android/s...
chrome/browser/android/shortcut_helper.cc:76: base::Unretained(this)));
On 2014/09/17 09:02:42, Miguel Garcia wrote:
> why base::unretained? wouldn't it be better to use a WeakPtrFactory?

Very good point. I wonder if I should introduce a way to cancel a Manifest
request. Anyhow, fixed with weak_ptr for now.

[mlamouri (slow - plz ping), 2014-09-17 10:51:08]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-17 10:51:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/573073002/40001

[commit-bot: I haz the power, 2014-09-17 11:51:46]

Committed patchset #3 (id:40001) as 8d27da84c2afbd6022f4fcb8b7007b09c93bcae3

[commit-bot: I haz the power, 2014-09-17 11:52:30]

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