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

Commit this CL on behalf of pkotwicz@. The original CL is:
https://codereview.chromium.org/2244223002/

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
Committed: https://crrev.com/787d2c9027fbc34ae35eee586a24d59124266718
Cr-Commit-Position: refs/heads/master@{#416649}

[Xi Han, 2016-08-31 18:06:08]

Patchset #2 (id:20001) has been deleted

[Xi Han, 2016-08-31 18:06:27]

Description was changed from

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

Commit this CL on behalf of pkotwicz@.

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
==========

to

==========
Commit this CL on behalf of pkotwicz@.

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
==========

[Xi Han, 2016-08-31 18:08:00]

The CQ bit was checked by hanxi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 18:11:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xi Han, 2016-08-31 18:14:10]

The CQ bit was unchecked by hanxi@chromium.org

[Xi Han, 2016-08-31 18:15:35]

Patchset #2 (id:40001) has been deleted

[Xi Han, 2016-08-31 20:17:35]

Patchset #2 (id:60001) has been deleted

[Xi Han, 2016-08-31 20:29:24]

Patchset #2 (id:80001) has been deleted

[Xi Han, 2016-08-31 20:30:59]

Description was changed from

==========
Commit this CL on behalf of pkotwicz@.

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
==========

to

==========
Commit this CL on behalf of pkotwicz@. The original CL is:
https://codereview.chromium.org/2244223002/

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
==========

[Xi Han, 2016-08-31 20:32:50]

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

[Xi Han, 2016-08-31 20:32:51]

Hi Dominick, I take this CL from Peter since he is on vacation till next
Thursday. I have addressed all of your comments, PTAL, thanks.

[Xi Han, 2016-09-01 18:55:40]

Patchset #3 (id:120001) has been deleted

[Xi Han, 2016-09-01 18:55:45]

Patchset #2 (id:100001) has been deleted

[Xi Han, 2016-09-01 18:56:56]

The CQ bit was checked by hanxi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 18:57:41]

Dry run: 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-01 19:53:39]

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

[commit-bot: I haz the power, 2016-09-01 19:53:40]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-02 01:17:46]

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

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:16: #include
"chrome/browser/installable/installable_manager.h"
Nit: I don't think you need this include any more.

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:70: void
AddToHomescreenManager::Start(content::WebContents* web_contents) {
Start(), StartWithoutInstallableCheck() CheckWebApkCompatible(), and
StartFetching() seem a bit complicated. How about this:

void AddToHomescreenManager::Start(content::WebContents* web_contents) {
  if (ChromeWebApkHost::AreWebApkEnabled() &&
      content::IsOriginSecure(web_contents->GetLastCommittedURL()) {
    // WebAPKs show a banner infobar once we have fetched the installable data.
    check_installable = true
  } else {
    ShowDialog();
  }

  data_fetcher_ = new AddToHomescreenDataFetcher(
      web_contents, ShortcutHelper::GetIdealHomescreenIconSizeInDp(),
      ShortcutHelper::GetMinimumHomescreenIconSizeInDp(),
      ShortcutHelper::GetIdealSplashImageSizeInDp(),
      ShortcutHelper::GetMinimumSplashImageSizeInDp(),
      check_installable, this);
}

I think that logic is right, and it replaces 4 methods with one.

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

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:11: #include
"base/memory/weak_ptr.h"
This isn't needed any more.

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:91:
base::WeakPtrFactory<AddToHomescreenManager> weak_ptr_factory_;
This isn't needed any more.

[Xi Han, 2016-09-02 14:25:15]

Patchset #3 (id:160001) has been deleted

[Xi Han, 2016-09-02 14:25:22]

Patchset #3 (id:180001) has been deleted

[Xi Han, 2016-09-02 14:29:00]

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

[Xi Han, 2016-09-02 14:29:01]

@donminickn: PTAL.
+ dfalcantara@ for OWNERS's review.

Thanks!

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

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:16: #include
"chrome/browser/installable/installable_manager.h"
On 2016/09/02 01:17:45, dominickn wrote:
> Nit: I don't think you need this include any more.

Done.

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:70: void
AddToHomescreenManager::Start(content::WebContents* web_contents) {
On 2016/09/02 01:17:45, dominickn wrote:
> Start(), StartWithoutInstallableCheck() CheckWebApkCompatible(), and
> StartFetching() seem a bit complicated. How about this:
> 
> void AddToHomescreenManager::Start(content::WebContents* web_contents) {
>   if (ChromeWebApkHost::AreWebApkEnabled() &&
>       content::IsOriginSecure(web_contents->GetLastCommittedURL()) {
>     // WebAPKs show a banner infobar once we have fetched the installable
data.
>     check_installable = true
>   } else {
>     ShowDialog();
>   }
> 
>   data_fetcher_ = new AddToHomescreenDataFetcher(
>       web_contents, ShortcutHelper::GetIdealHomescreenIconSizeInDp(),
>       ShortcutHelper::GetMinimumHomescreenIconSizeInDp(),
>       ShortcutHelper::GetIdealSplashImageSizeInDp(),
>       ShortcutHelper::GetMinimumSplashImageSizeInDp(),
>       check_installable, this);
> }
> 
> I think that logic is right, and it replaces 4 methods with one.

That is brilliant, thanks!

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

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:11: #include
"base/memory/weak_ptr.h"
On 2016/09/02 01:17:45, dominickn wrote:
> This isn't needed any more.

Done.

https://codereview.chromium.org/2292133003/diff/140001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:91:
base::WeakPtrFactory<AddToHomescreenManager> weak_ptr_factory_;
On 2016/09/02 01:17:46, dominickn wrote:
> This isn't needed any more.

Good catch, removed.

[dominickn, 2016-09-02 14:35:20]

lgtm

[gone, 2016-09-02 18:28:56]

lgtm

[Xi Han, 2016-09-02 20:13:57]

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

[Xi Han, 2016-09-02 20:13:58]

cpu@chromium.org: I need OWNERS review for:
-  chrome/browser/BUILD.gn

Could you please take a look? Thanks!

[Xi Han, 2016-09-02 20:22:05]

The CQ bit was checked by hanxi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 20:22:36]

Dry run: 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-02 22:28:23]

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

[commit-bot: I haz the power, 2016-09-02 22:28:23]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-05 07:18:42]

On 2016/09/02 20:13:58, Xi Han wrote:
> mailto:cpu@chromium.org: I need OWNERS review for:
> -  chrome/browser/BUILD.gn
> 
> Could you please take a look? Thanks!

Not sure if cpu@ works on chromium any more - you might want to try thestig@.

[Xi Han, 2016-09-05 13:52:14]

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

[Xi Han, 2016-09-05 13:52:15]

+thestig@: I need OWNERS review for:
-  chrome/browser/BUILD.gn

Could you please take a look? Thanks!

[Lei Zhang, 2016-09-06 15:59:01]

On 2016/09/05 13:52:15, Xi Han wrote:
> +thestig@: I need OWNERS review for:
> -  chrome/browser/BUILD.gn
> 
> Could you please take a look? Thanks!

lgtm

[Xi Han, 2016-09-06 16:23:44]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2016-09-06 16:23: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-06 17:41:59]

Description was changed from

==========
Commit this CL on behalf of pkotwicz@. The original CL is:
https://codereview.chromium.org/2244223002/

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
==========

to

==========
Commit this CL on behalf of pkotwicz@. The original CL is:
https://codereview.chromium.org/2244223002/

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
==========

[commit-bot: I haz the power, 2016-09-06 17:42:01]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2016-09-06 17:44:02]

Description was changed from

==========
Commit this CL on behalf of pkotwicz@. The original CL is:
https://codereview.chromium.org/2244223002/

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
==========

to

==========
Commit this CL on behalf of pkotwicz@. The original CL is:
https://codereview.chromium.org/2244223002/

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

Committed: https://crrev.com/787d2c9027fbc34ae35eee586a24d59124266718
Cr-Commit-Position: refs/heads/master@{#416649}
==========

[commit-bot: I haz the power, 2016-09-06 17:44:03]

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