Determine whether to show "Add to Homescreen" dialog or WebAPK infobar

Determine whether to show "Add to Homescreen" dialog or WebAPK infobar

This CL:
- Introduces a new class, AddToHomescreenProcess, which manages the
add-to-homescreen process. AddToHomescreenProcess is largely copied from
AddToHomescreenDialogHelper
- The CL adds logic to AddToHomescreenProcess to determine whether to show
the add-WebAPK-to-homescreen infobar instead of the add-to-homescreen dialog.
- Changes the entry point for adding a page to the homescreen from
AddToHomescreenDialog#show() to AddToHomescreenProcess#start()

BUG=627250, 606430, 635933

[pkotwicz, 2016-08-15 12:57:09]

Description was changed from

==========
ABCD
==========

to

==========
Determine whether to show "Add to Homescreen" dialog or WebAPK infobar
==========

[pkotwicz, 2016-08-16 02:40:12]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-08-16 02:46:43]

Description was changed from

==========
Determine whether to show "Add to Homescreen" dialog or WebAPK infobar
==========

to

==========
Determine whether to show "Add to Homescreen" dialog or WebAPK infobar

This CL:
- Introduces a new class, AddToHomescreenProcess, which manages the
add-to-homescreen process.
- The CL adds logic to AddToHomescreenProcess to determine whether to show
the add-WebAPK-to-homescreen infobar instead of the add-to-homescreen dialog.
- Changes the entry point for adding a page to the homescreen from
AddToHomescreenDialog#show() to AddToHomescreenProcess#start()

BUG=627250,606430,635933
==========

[pkotwicz, 2016-08-16 02:46:43]

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

[pkotwicz, 2016-08-16 02:47:43]

Description was changed from

==========
Determine whether to show "Add to Homescreen" dialog or WebAPK infobar

This CL:
- Introduces a new class, AddToHomescreenProcess, which manages the
add-to-homescreen process.
- The CL adds logic to AddToHomescreenProcess to determine whether to show
the add-WebAPK-to-homescreen infobar instead of the add-to-homescreen dialog.
- Changes the entry point for adding a page to the homescreen from
AddToHomescreenDialog#show() to AddToHomescreenProcess#start()

BUG=627250,606430,635933
==========

to

==========
Determine whether to show "Add to Homescreen" dialog or WebAPK infobar

This CL:
- Introduces a new class, AddToHomescreenProcess, which manages the
add-to-homescreen process. AddToHomescreenProcess is largely copied from
AddToHomescreenDialogHelper
- The CL adds logic to AddToHomescreenProcess to determine whether to show
the add-WebAPK-to-homescreen infobar instead of the add-to-homescreen dialog.
- Changes the entry point for adding a page to the homescreen from
AddToHomescreenDialog#show() to AddToHomescreenProcess#start()

BUG=627250,606430,635933
==========

[pkotwicz, 2016-08-16 02:54:44]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2016-08-16 02:54:51]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2016-08-16 02:58:16]

Xi can you please take a look?

[pkotwicz, 2016-08-16 03:24:39]

Just realized that InstallableManager::IsManifestValidForWebApp() checks whether
the Web Manifest contains an icon which is too restrictive for WebAPKs

[dominickn, 2016-08-16 03:27:07]

On 2016/08/16 03:24:39, pkotwicz wrote:
> Just realized that InstallableManager::IsManifestValidForWebApp() checks
whether
> the Web Manifest contains an icon which is too restrictive for WebAPKs

I thought it was agreed that WebAPKs should only be minted for full PWAs - i.e.
they would have an icon in the manifest? I made comments to that effect in
crrev.com/2206493002.

[pkotwicz, 2016-08-16 15:46:37]

I discussed with Dominick offline. I filed a bug at http://crbug.com/638247 to
figure out whether we want to generate WebAPKs for Web Manifests with no icon
URL long term.

Dominick mentioned (and I agree) that not generating WebAPKs for Web Manifests
with no icon URL makes things easier for us in the short term

[pkotwicz, 2016-08-17 02:34:23]

As an update, I am putting this CL on ice. We might want to display the
add-to-homescreen dialog for WebAPKs after all

[pkotwicz, 2016-08-17 23:52:34]

Xi can you please take a look?

I pinged Sam and he said that security is OK with not showing the
add-to-homescreen dialog for now

[pkotwicz, 2016-08-19 00:51:48]

Xi Ping!

[Xi Han, 2016-08-19 14:11:06]

Sorry for the late reply. Mostly nits.

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

https://codereview.chromium.org/2244223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenProcess.java:20:
public interface Ui {
UiObserver? I feel "Ui" is a concept way to wide.

https://codereview.chromium.org/2244223002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_process.cc (right):

https://codereview.chromium.org/2244223002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_process.cc:162:
java_ref_.obj(),
It seems ".obj()" has been removed in my recently reviewed CLs, please update
this CL if it is corrent.

https://codereview.chromium.org/2244223002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_process.cc:167: const SkBitmap&
icon) {
The indent of "const SkBitmap& icon" is wrong, please fix it.

[pkotwicz, 2016-08-20 00:16:29]

Xi can you please take another look?

[Xi Han, 2016-08-22 12:24:52]

lgtm

[pkotwicz, 2016-08-22 14:10:50]

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

[pkotwicz, 2016-08-22 14:10:51]

Dominick, can you please take a look?

[dominickn, 2016-08-22 14:37:18]

On 2016/08/22 14:10:51, pkotwicz wrote:
> Dominick, can you please take a look?

FYI: am at an offsite in SF for the next couple of days and this is a chunky
review. I'll try and get it done ASAP (< 2 days).

[dominickn, 2016-08-22 18:09:17]

Very preliminary comments, plus a question. Is this *renaming* add to homescreen
dialog helper? At first glance the new interfaces and structure added in this CL
seem unnecessary because fundamentally you just want a if (webAPK) { do that }
else { do dialog }?

Could you provide a bit more context for why there's all of this churn - I'm
definitely in favour of changing less if possible.

https://codereview.chromium.org/2244223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenProcess.java
(right):

https://codereview.chromium.org/2244223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenProcess.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
This is a new file, update to 2016.

https://codereview.chromium.org/2244223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenProcess.java:17:
public class AddToHomescreenProcess implements AddToHomescreenDelegate {
"Process" is not a great name because it implies that add to homescreen spins up
a new process. This feels like an AddToHomescreenManager perhaps.

[pkotwicz, 2016-08-22 19:37:13]

Dominick which of the interfaces added in this CL do you think are unnecessary?

The major change in this CL is that AddToHomescreenManager now owns
AddToHomescreenDialog. Formely AddToHomescreenDialog owned add
AddToHomescreenDialogHelper.
There are some slightly unrelated changes in add_to_homescreen_process.h because
I changed add_to_homescreen_process.h AddToHomescreenDataFetcher::Observer's
overrides to be private.

[pkotwicz, 2016-08-22 19:37:40]

https://codereview.chromium.org/2244223002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenProcess.java
(right):

https://codereview.chromium.org/2244223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenProcess.java:17:
public class AddToHomescreenProcess implements AddToHomescreenDelegate {
Ok, I can rename the file to AddToHomescreenManager

[dominickn, 2016-08-24 04:20:57]

On 2016/08/22 19:37:13, pkotwicz wrote:
> Dominick which of the interfaces added in this CL do you think are
unnecessary?
> 
> The major change in this CL is that AddToHomescreenManager now owns
> AddToHomescreenDialog. Formely AddToHomescreenDialog owned add
> AddToHomescreenDialogHelper.
> There are some slightly unrelated changes in add_to_homescreen_process.h
because
> I changed add_to_homescreen_process.h AddToHomescreenDataFetcher::Observer's
> overrides to be private.

Mainly the AddToHomescreenDelegate seems unnecessary. You could just extend the
existing one for testing and override its methods to be no-ops. There won't be
any other implements of the delegate in the future right?

[pkotwicz, 2016-08-24 22:16:34]

Dominick can you please take another look?

- This CL now depends on the refactor in
https://codereview.chromium.org/2278693002/
- I have renamed AddToHomescreenProcess to AddToHomescreenManager as you
suggested
- I now pass the AddToHomescreenManager directly to AddToHomescreenDialog
instead of passing passing the AddToHomescreenManager as a delegate

[pkotwicz, 2016-08-26 19:20:34]

Dominick Ping!

[dominickn, 2016-08-26 22:27:51]

On 2016/08/26 19:20:34, pkotwicz wrote:
> Dominick Ping!

Sorry, currently in transit back to Sydney. Will try and finish the review on
this along the way.

[dominickn, 2016-08-29 00:13:37]

This is looking better now. One more major design question.

https://codereview.chromium.org/2244223002/diff/140001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right):

https://codereview.chromium.org/2244223002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:100:
installable_manager->GetData(
This really should be in the data fetcher, not in the manager, because the
fetcher is in charge of retrieving information. This class really shouldn't have
a weak pointer factory et al. Can you move this back to the fetcher and perhaps
add a OnDidDetermineWebApkCompatibility method to the observer interface?

https://codereview.chromium.org/2244223002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:105: void
AddToHomescreenManager::OnGotWebApkCompatibilityData(
Nit: Please name this OnDidGetInstallableData (and similarly, everywhere with
"Got" should be "DidGet".

https://codereview.chromium.org/2244223002/diff/140001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.h (right):

https://codereview.chromium.org/2244223002/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:19: class Message;
Nit: Looks like this isn't used any more, delete.

[pkotwicz, 2016-09-09 23:43:26]

Abandoning this CL because it landed as part of
https://codereview.chromium.org/2292133003/