navigator.share: Resolve promise only after user chooses a target.

navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).

BUG=595608, 637194
Committed: https://crrev.com/906820b686d31b48d8c75e89ec3fe67b5c9f36b0
Cr-Commit-Position: refs/heads/master@{#412175}

[Matt Giuca, 2016-08-03 07:29:25]

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

[commit-bot: I haz the power, 2016-08-03 07:29:32]

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-08-03 07:51:34]

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

[commit-bot: I haz the power, 2016-08-03 07:51:34]

Dry run: 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...)

[Matt Giuca, 2016-08-03 08:19:44]

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

[commit-bot: I haz the power, 2016-08-03 08:20:00]

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

[Matt Giuca, 2016-08-03 08:31:30]

mgiuca@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Matt Giuca, 2016-08-03 08:31:31]

Hi David,

(+cc Ted who has done recent work on ShareHelper; Dan who reviewed my previous
CL)

I want to add new share functionality (triggered from the navigator.share API).
In my previous CL I just wrote my own intent sender (as you can see in the
deleted code for this CL). But now I want a lot more functionality that overlaps
with ShareHelper so I've decided to reuse ShareHelper.

This means adding a bunch more arguments and features to ShareHelper:
1. Ability to say "please don't save my choice in the last-choice pref."
2. Ability to supply an arbitrary text string, not just a URL.
3. Ability to pass a callback and get notified when chosen.

If you think these changes to ShareHelper are too severe, I could just copy the
code and implement my own, but I think the code reuse benefit outweighs the
extra arguments. I'm happy to help further refactor ShareHelper in the future.
WDYT?

https://codereview.chromium.org/2203273002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2203273002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:498:
text = text + " " + url;
On reflection, this seems a bit specific to my use case. It might be best to
remove the |url| argument entirely and just have text. Move the call to
DomDistillerUrlUtils out to ChromeActivity (where it is specific to the existing
Share menu use case).

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

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

[commit-bot: I haz the power, 2016-08-03 09:06:46]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2016-08-04 08:56:07]

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

[commit-bot: I haz the power, 2016-08-04 08:56:15]

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-08-04 09:27:42]

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

[commit-bot: I haz the power, 2016-08-04 09:27:43]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2016-08-05 16:06:39]

On 2016/08/03 08:31:31, Matt Giuca wrote:
> Hi David,
> 
> (+cc Ted who has done recent work on ShareHelper; Dan who reviewed my previous
> CL)
> 
> I want to add new share functionality (triggered from the navigator.share
API).
> In my previous CL I just wrote my own intent sender (as you can see in the
> deleted code for this CL). But now I want a lot more functionality that
overlaps
> with ShareHelper so I've decided to reuse ShareHelper.
> 
> This means adding a bunch more arguments and features to ShareHelper:
> 1. Ability to say "please don't save my choice in the last-choice pref."
> 2. Ability to supply an arbitrary text string, not just a URL.
> 3. Ability to pass a callback and get notified when chosen.
> 
> If you think these changes to ShareHelper are too severe, I could just copy
the
> code and implement my own, but I think the code reuse benefit outweighs the
> extra arguments. I'm happy to help further refactor ShareHelper in the future.
> WDYT?
> 
>
https://codereview.chromium.org/2203273002/diff/20001/chrome/android/java/src...
> File
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
> (right):
> 
>
https://codereview.chromium.org/2203273002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:498:
> text = text + " " + url;
> On reflection, this seems a bit specific to my use case. It might be best to
> remove the |url| argument entirely and just have text. Move the call to
> DomDistillerUrlUtils out to ChromeActivity (where it is specific to the
existing
> Share menu use case).

Posted some relatively small nits.  I'm fine with you sharing the code.  Yeah I
think making share just take an arbitrary string and allowing you to supply what
you want makes more sense too :).

[David Trainor- moved to gerrit, 2016-08-05 16:06:54]

gah forgot to publish

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:498:
if (url != null && url.length() > 0) {
!TextUtils.isEmpty(url)?  Same for below?

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:23:
private final WindowAndroid mWindow;
Do we still need to save mWindow now?

[Matt Giuca, 2016-08-08 04:54:43]

Thanks.

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:498:
if (url != null && url.length() > 0) {
On 2016/08/05 16:06:54, David Trainor wrote:
> !TextUtils.isEmpty(url)?  Same for below?

Done.

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2203273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:23:
private final WindowAndroid mWindow;
On 2016/08/05 16:06:54, David Trainor wrote:
> Do we still need to save mWindow now?

Huh, I guess not. Done.

[Matt Giuca, 2016-08-08 05:24:52]

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

[commit-bot: I haz the power, 2016-08-08 05:24:59]

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-08-08 06:08:56]

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

[commit-bot: I haz the power, 2016-08-08 06:08:57]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2016-08-08 23:03:31]

lgtm.  I'd still support pulling out url and text and making that the choice of
the caller though!

[Matt Giuca, 2016-08-09 00:11:14]

On 2016/08/08 23:03:31, David Trainor wrote:
> lgtm.  I'd still support pulling out url and text and making that the choice
of
> the caller though!

Indeed; I'll do that in a follow-up if you don't mind.

[Matt Giuca, 2016-08-09 00:11:23]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-08-09 00:12:00]

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-08-09 00:17:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-09 00:20:03]

Description was changed from

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

BUG=595608
==========

to

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

BUG=595608

Committed: https://crrev.com/610f5e1ea1b1b38926263489d3d235fc834686de
Cr-Commit-Position: refs/heads/master@{#410511}
==========

[commit-bot: I haz the power, 2016-08-09 00:20:04]

Patchset 5 (id:??) landed as
https://crrev.com/610f5e1ea1b1b38926263489d3d235fc834686de
Cr-Commit-Position: refs/heads/master@{#410511}

[amineer, 2016-08-15 16:14:59]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2241303002/ by amineer@chromium.org.

The reason for reverting is: Looks to be causing a large number of crashes, see
crbug.com/637194..

[Matt Giuca, 2016-08-16 05:17:05]

Description was changed from

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

BUG=595608

Committed: https://crrev.com/610f5e1ea1b1b38926263489d3d235fc834686de
Cr-Commit-Position: refs/heads/master@{#410511}
==========

to

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

BUG=595608,637194

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).
==========

[Matt Giuca, 2016-08-16 05:17:40]

Description was changed from

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

BUG=595608,637194

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).
==========

to

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).

BUG=595608,637194
==========

[Matt Giuca, 2016-08-16 05:18:56]

Patchset #6 (id:100001) has been deleted

[Matt Giuca, 2016-08-16 05:20:41]

Thanks, Alex.

Re-opening with a new patch set that fixes the crash.

[Matt Giuca, 2016-08-16 05:20:51]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-08-16 05:20:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2203273002/#ps140001
(title: "Null-check callback. Fixes crbug.com/637194.")

[commit-bot: I haz the power, 2016-08-16 05:21:01]

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-08-16 05:57:59]

Description was changed from

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).

BUG=595608,637194
==========

to

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).

BUG=595608,637194
==========

[commit-bot: I haz the power, 2016-08-16 05:58:02]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-08-16 05:59:49]

Description was changed from

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).

BUG=595608,637194
==========

to

==========
navigator.share: Resolve promise only after user chooses a target.

Previously, would immediately resolve as soon as the share dialog was
shown. This introduces a new issue where the promise *never* resolves
or rejects if the user cancels the share picker (I'm not sure how to do
this or if it's even possible).

Introduces a bunch of new functionality in the ShareHelper class to
support this new use case.

Reland of https://crrev.com/410511 (to fix https://crbug.com/637194).

BUG=595608,637194

Committed: https://crrev.com/906820b686d31b48d8c75e89ec3fe67b5c9f36b0
Cr-Commit-Position: refs/heads/master@{#412175}
==========

[commit-bot: I haz the power, 2016-08-16 05:59:50]

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