Added end-to-end tests for navigator.share.

Added end-to-end tests for navigator.share.

There are separate tests for pre-Lollipop and post-Lollipop Android
(which run completely different code paths in ShareHelper and exhibit
different behaviour). On pre-Lolliop devices, the post-Lollipop tests
are skipped. On post-Lollipop devices, both sets of tests are run.

Adds some new bits to ShareHelper to facilitate testing.

BUG=645006
Committed: https://crrev.com/03d2b0f7c783a91ffdb37942208ffa4ec79b6fb9
Cr-Commit-Position: refs/heads/master@{#421417}

[Matt Giuca, 2016-09-14 07:22:38]

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

[Matt Giuca, 2016-09-14 07:22:38]

sammc: Review of tests.
dtrainor: Owners, and detailed review of ShareHelper changes.

Note: Depends on https://codereview.chromium.org/2338683002/ which I also sent
to David for review.

[Matt Giuca, 2016-09-14 07:22:46]

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

[commit-bot: I haz the power, 2016-09-14 07:23:04]

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-14 07:25:12]

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

[commit-bot: I haz the power, 2016-09-14 07:25:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Matt Giuca, 2016-09-14 07:50:13]

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

[commit-bot: I haz the power, 2016-09-14 07:50:21]

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-14 09:19:23]

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

[commit-bot: I haz the power, 2016-09-14 09:19:24]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-19 00:50:03]

https://codereview.chromium.org/2338203002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java
(right):

https://codereview.chromium.org/2338203002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:96:
tab.removeObserver(updateWaiter);
This should go in tearDown() or a finally block.

https://codereview.chromium.org/2338203002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:138:
ShareHelper.setFakeIntentReceiverForTesting(new ShareHelper.FakeIntentReceiver()
{
Do you need to reset this?

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

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:89:
*/
This should be in the previous line.

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java
(right):

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:238:
// Immediately cancel the chooser dialog.
Is it important that the cancel be immediate?

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:276:
// Click on an app (it doesn't matter which, because we will hook the inten).
intent

[Matt Giuca, 2016-09-19 23:35:39]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2016-09-19 23:39:09]

Patchset #4 (id:80001) has been deleted

[Matt Giuca, 2016-09-19 23:43:23]

https://codereview.chromium.org/2338203002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java
(right):

https://codereview.chromium.org/2338203002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:96:
tab.removeObserver(updateWaiter);
On 2016/09/19 00:50:03, Sam McNally wrote:
> This should go in tearDown() or a finally block.

Done. Moved a lot of common logic to the setUp/tearDown.

https://codereview.chromium.org/2338203002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:138:
ShareHelper.setFakeIntentReceiverForTesting(new ShareHelper.FakeIntentReceiver()
{
On 2016/09/19 00:50:03, Sam McNally wrote:
> Do you need to reset this?

Done (in tearDown).

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

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:89:
*/
On 2016/09/19 00:50:03, Sam McNally wrote:
> This should be in the previous line.

Done (added a third line actually, which seems to be the style for methods).

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java
(right):

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:238:
// Immediately cancel the chooser dialog.
On 2016/09/19 00:50:03, Sam McNally wrote:
> Is it important that the cancel be immediate?

"immediately" meaning "without waiting or doing anything else first".

What wording do you suggest?

https://codereview.chromium.org/2338203002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/WebShareTest.java:276:
// Click on an app (it doesn't matter which, because we will hook the inten).
On 2016/09/19 00:50:03, Sam McNally wrote:
> intent

Done.

[Sam McNally, 2016-09-20 00:29:15]

lgtm

[Matt Giuca, 2016-09-21 03:10:58]

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

[commit-bot: I haz the power, 2016-09-21 03:11:12]

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-21 04:32:02]

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

[commit-bot: I haz the power, 2016-09-21 04:32:03]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2016-09-23 20:31:57]

lgtm % comment nits.  Had a suggestion about the FakeIntentReceiver interface,
but I'm fine either way.

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:87:
public static interface FakeIntentReceiver {
Move this out of the middle of the static variables?

Also haven't looked at the rest of the CL yet, but would it be cleaner to have a
default implementation of this that forwarded to the Activity?  That way you
wouldn't need test code in the main logic.

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:97: *
|context| is the context that will receive broadcasts from the simulated
activity.
@param context
@param intent

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:140:
* Pass null to reset back to the default behavior (really send intents).
@param receiver, describe behavior there

[Matt Giuca, 2016-09-26 01:54:31]

David: PTAL, I think you should decide between PS 7 and 8, because PS 8 changes
quite a bit.

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:87:
public static interface FakeIntentReceiver {
On 2016/09/23 20:31:56, David Trainor wrote:
> Move this out of the middle of the static variables?
> 
> Also haven't looked at the rest of the CL yet, but would it be cleaner to have
a
> default implementation of this that forwarded to the Activity?  That way you
> wouldn't need test code in the main logic.

Hmmmk, that makes sense. I did it (made it into a class with a default
implementation), but I have some problems:

1. It means this thing is no longer a "fake intent receiver", it's always used,
so I had to rename and redocument everything.
2. It felt OK having a "for testing only" class having this weird bodgy
interface with 3 random methods. It feels much worse having these on a "real"
class.
3. (Potentially big problem): The default implementation needs to be created in
a static initializer, because these are all static methods and there is no
"init" time. In C++ this would be banned. Not sure if there's a similar policy
in Java. If we can't do a static initializer, then we have to put back those
null checks anyway, which kind of defeats the purpose of this change.

I did the other changes you requested in Patch Set 7. Then I did this change in
Patch Set 8. Have a look at the delta between 7 and 8 and see which one you
prefer.

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:97: *
|context| is the context that will receive broadcasts from the simulated
activity.
On 2016/09/23 20:31:56, David Trainor wrote:
> @param context
> @param intent

Done.

https://codereview.chromium.org/2338203002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:140:
* Pass null to reset back to the default behavior (really send intents).
On 2016/09/23 20:31:56, David Trainor wrote:
> @param receiver, describe behavior there

Done.

[David Trainor- moved to gerrit, 2016-09-26 18:50:34]

Yeah I think you're right.  I'm ok without the merged interface :).

[Matt Giuca, 2016-09-28 00:07:36]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-09-28 00:07:37]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2338203002/#ps220001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-09-28 00:07:50]

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-28 01:27:40]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-09-28 01:30:56]

Description was changed from

==========
Added end-to-end tests for navigator.share.

There are separate tests for pre-Lollipop and post-Lollipop Android
(which run completely different code paths in ShareHelper and exhibit
different behaviour). On pre-Lolliop devices, the post-Lollipop tests
are skipped. On post-Lollipop devices, both sets of tests are run.

Adds some new bits to ShareHelper to facilitate testing.

BUG=645006
==========

to

==========
Added end-to-end tests for navigator.share.

There are separate tests for pre-Lollipop and post-Lollipop Android
(which run completely different code paths in ShareHelper and exhibit
different behaviour). On pre-Lolliop devices, the post-Lollipop tests
are skipped. On post-Lollipop devices, both sets of tests are run.

Adds some new bits to ShareHelper to facilitate testing.

BUG=645006

Committed: https://crrev.com/03d2b0f7c783a91ffdb37942208ffa4ec79b6fb9
Cr-Commit-Position: refs/heads/master@{#421417}
==========

[commit-bot: I haz the power, 2016-09-28 01:30:58]

Patchset 10 (id:??) landed as
https://crrev.com/03d2b0f7c783a91ffdb37942208ffa4ec79b6fb9
Cr-Commit-Position: refs/heads/master@{#421417}