PaymentRequest.abort() should return a promise.

PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentrequest-interface

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
Committed: https://crrev.com/e411df45423c6c1fd6acb338ce0ea48630784b18
Cr-Commit-Position: refs/heads/master@{#399854}

[please use gerrit instead, 2016-06-08 05:13:03]

Description was changed from

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request to introduce the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

BUG=617193
==========

to

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move MockFunction into MockFunction.h/cpp.
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Move abort() tests into AbortTest.cpp
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

[please use gerrit instead, 2016-06-08 21:27:33]

Patchset #2 (id:20001) has been deleted

[please use gerrit instead, 2016-06-08 21:28:21]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-06-08 22:25:10]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2016-06-08 22:27:43]

rouslan@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mek@chromium.org, tsepez@chromium.org

[please use gerrit instead, 2016-06-08 22:27:44]

mek@, ptal WebKit.

dfalcantara@, ptal Java.

tsepez@, ptal payment_request.mojom.

[Tom Sepez, 2016-06-08 22:29:04]

mojom lgtm

[Marijn Kruisselbrink, 2016-06-08 22:44:12]

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/MockFunction.h (right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/MockFunction.h:16: class MockFunction
: public ScriptFunction {
I'm a bit uncomfortable with having something named as generic as
blink::MockFunction in such a non-generic location. Couldn't this be part of
PaymentRequestTestBase somehow to not pollute the global blink:: namespace with
generic names?

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:260:
m_abortResolver = ScriptPromiseResolver::create(scriptState);
Presumably you want to do something special if m_abortResolver is already set?

The spec seems not quite meaningful if abort is called multiple times either
(it'll end up rejecting the show promise multiple times as well, if I'm reading
the spec correctly).

[haraken, 2016-06-09 01:57:58]

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

[haraken, 2016-06-09 01:57:59]

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/MockFunction.h (right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/MockFunction.h:16: class MockFunction
: public ScriptFunction {

It would be better to move this class to V8BindingForTesting.h so that others
can use it.

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:493: void
PaymentRequest::OnAbort(bool abortedSuccessfully)

OnAbort => onAbort

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:19: class
PaymentRequestTestBase : public testing::Test {

Can we use ScriptStateForTesting?

[gone, 2016-06-09 15:49:28]

https://chromiumcodereview.appspot.com/2048823004/diff/60001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java
(right):

https://chromiumcodereview.appspot.com/2048823004/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java:44:
clickNode("abort");
This will likely be flaky.  Any timing hiccups would mess up the test.

https://chromiumcodereview.appspot.com/2048823004/diff/60001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java
(right):

https://chromiumcodereview.appspot.com/2048823004/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:98:
TimeoutException {
nit: indentation is wonky

[please use gerrit instead, 2016-06-09 22:28:19]

Patchset #2 (id:80001) has been deleted

[please use gerrit instead, 2016-06-09 22:28:28]

Patchset #2 (id:100001) has been deleted

[please use gerrit instead, 2016-06-09 22:33:29]

Description was changed from

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move MockFunction into MockFunction.h/cpp.
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Move abort() tests into AbortTest.cpp
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

to

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

[please use gerrit instead, 2016-06-09 22:36:14]

haraken@, ptal WebKit.

mek@, if you're not out of office, ptal WebKit as well.

dfalcantara@, ptal Java. I added a testing callback for the abort() function. So
the chance of flakiness should be reduced. Curiously enough, I needed to link
the mojom Java targets into the chrome java test target in order for the tests
to build properly.

https://codereview.chromium.org/2048823004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java
(right):

https://codereview.chromium.org/2048823004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java:44:
clickNode("abort");
On 2016/06/09 15:49:28, dfalcantara wrote:
> This will likely be flaky.  Any timing hiccups would mess up the test.

Done.

https://codereview.chromium.org/2048823004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java
(right):

https://codereview.chromium.org/2048823004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:98:
TimeoutException {
On 2016/06/09 15:49:28, dfalcantara wrote:
> nit: indentation is wonky

Done.

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/MockFunction.h (right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/MockFunction.h:16: class MockFunction
: public ScriptFunction {
On 2016/06/09 01:57:58, haraken wrote:
> 
> It would be better to move this class to V8BindingForTesting.h so that others
> can use it.

Actually, turns out this implementation of MockFunction does not fail the test
if the expectations are not met. This is because expectations are checked when
the MockFunction is garbage-collected or when Mock::VerifyAndClear(&mockObject)
is called. The garbage collection happens when all tests finish. At that time,
there's a message printed in console, but no test failure.

Therefore, I am taking a different approach to MockFunction. I am keeping a
reference to every MockFunction that I create and then calling
Mock::VerifyAndClear() on these objects when a test case finishes. The best
place to store these references is in PaymentRequestTestBase persistently.
Persistence must be used because ScriptFunction is garbage collected, but unit
test cases are not.

See PaymentRequestTestBase.h/cpp for the new implementation.

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:260:
m_abortResolver = ScriptPromiseResolver::create(scriptState);
On 2016/06/08 22:44:12, Marijn Kruisselbrink wrote:
> Presumably you want to do something special if m_abortResolver is already set?
> 
> The spec seems not quite meaningful if abort is called multiple times either
> (it'll end up rejecting the show promise multiple times as well, if I'm
reading
> the spec correctly).

Rejecting the second abort() promise is more consistent with how show() and
complete() promises behave.

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:493: void
PaymentRequest::OnAbort(bool abortedSuccessfully)
On 2016/06/09 01:57:59, haraken wrote:
> 
> OnAbort => onAbort

This is an implementation of the interface defined mojom:

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/publ...

I thought mojom follows Chrome C++ standard of using the capital letters for the
first letter of a method name. Should I rename all of these into Blink style
instead?

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:19: class
PaymentRequestTestBase : public testing::Test {
On 2016/06/09 01:57:59, haraken wrote:
> 
> Can we use ScriptStateForTesting?

ScriptStateForTesting needs an isolate(), which I can get only from
DummyPageHolder. So replacing DummyPageHolder with ScriptStateForTesting seems
infeasible.

[please use gerrit instead, 2016-06-09 22:36:30]

patch 2.

[haraken, 2016-06-10 00:15:12]

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:19: class
PaymentRequestTestBase : public testing::Test {
On 2016/06/09 22:36:14, Rouslan (ツ) wrote:
> On 2016/06/09 01:57:59, haraken wrote:
> > 
> > Can we use ScriptStateForTesting?
> 
> ScriptStateForTesting needs an isolate(), which I can get only from
> DummyPageHolder. So replacing DummyPageHolder with ScriptStateForTesting seems
> infeasible.

You can use v8::Isolate::GetCurrent().

Or if you really need a dummy page, you can add DummyPageHolder to
ScriptStateForTesting.

[gone, 2016-06-10 01:25:23]

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:132:
/**
Are you doing this only to avoid having to set that observer?  If so, just make
one constructor call the other one.

public PaymentRequestImpl(WebContents webContents) {
    this(webContents, null);
}

private PaymentRequestImpl(WebContents webContents,
PaymentRequestServiceObserverForTest observerForTest) {
    ....
}

[please use gerrit instead, 2016-06-10 21:18:21]

haraken@, ptal patch 3.

dfalcantara@, see my response inline.

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:19: class
PaymentRequestTestBase : public testing::Test {
On 2016/06/10 00:15:12, haraken wrote:
> On 2016/06/09 22:36:14, Rouslan (ツ) wrote:
> > On 2016/06/09 01:57:59, haraken wrote:
> > > 
> > > Can we use ScriptStateForTesting?
> > 
> > ScriptStateForTesting needs an isolate(), which I can get only from
> > DummyPageHolder. So replacing DummyPageHolder with ScriptStateForTesting
seems
> > infeasible.
> 
> You can use v8::Isolate::GetCurrent().
> 
> Or if you really need a dummy page, you can add DummyPageHolder to
> ScriptStateForTesting.

After experimenting, turns out I need a document with a frame. PaymentRequest
calls frame->isMainFrame() and frame->getServiceRegstry(). It's simplest to use
DummyPageHolder. Placing DummyPageHolder in ScriptStateForTesting seems like an
overkill: most tests do not need this. I've placed it into
ScriptStateForTestingWithPage instead. What do you think?

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:132:
/**
On 2016/06/10 01:25:23, dfalcantara wrote:
> Are you doing this only to avoid having to set that observer?  If so, just
make
> one constructor call the other one.

Actually, it's a bit more tricky than that. An instance of PaymentRequestFactory
creates instances of PaymentRequestImpl in response to mojo IPC calls from
Blink. Somewhere in PaymentRequestFactory or PaymentRequestImpl we need a static
setObserverForTest() method that the test can call without referring to these
instances. The observer needs to be set before an instance is created. The
factory instance is created when the browser starts up, so that may be too early
for integration tests. Therefore, the observer should be set in
PaymentRequestImpl. Find-bugs tells us that static variables should not be
accessed from dynamic methods. Therefore, the best way to pass the osberver into
PaymentRequestImpl is via a static create() method. That's the reason for this
design. Sorry for the wall of text.

[gone, 2016-06-10 21:24:54]

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:132:
/**
On 2016/06/10 21:18:20, Rouslan (ツ) wrote:
> On 2016/06/10 01:25:23, dfalcantara wrote:
> > Are you doing this only to avoid having to set that observer?  If so, just
> make
> > one constructor call the other one.
> 
> Actually, it's a bit more tricky than that. An instance of
PaymentRequestFactory
> creates instances of PaymentRequestImpl in response to mojo IPC calls from
> Blink. Somewhere in PaymentRequestFactory or PaymentRequestImpl we need a
static
> setObserverForTest() method that the test can call without referring to these
> instances. The observer needs to be set before an instance is created. The
> factory instance is created when the browser starts up, so that may be too
early
> for integration tests. Therefore, the observer should be set in
> PaymentRequestImpl. Find-bugs tells us that static variables should not be
> accessed from dynamic methods. Therefore, the best way to pass the osberver
into
> PaymentRequestImpl is via a static create() method. That's the reason for this
> design. Sorry for the wall of text.

What about this:

/**
 * Monitors all created PaymentRequestImpls for tests.
 */
public interface PaymentRequestServiceObserverForTest {
    /**
     * Called when an abort request was denied.
     * @param paymentRequestImpl Which PaymentRequestImpl this was triggered
for.
     */
    void onPaymentRequestServiceUnableToAbort(PaymentRequestImpl
paymentRequestImpl);
}

public static void
setPaymentRequestServiceObserver(PaymentRequestServiceObserverForTest observer)
{
    sPaymentRequestServiceObserver = observer;
}

Constructor reverts to the original constructor.

You end up with a single Observer that can be statically set before any are
created, and none of the PaymentRequestImpl instances have to store (or care)
about having a member field Observer.

[please use gerrit instead, 2016-06-10 22:31:59]

dfalcantara@, ptal patch 5.

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:132:
/**
My bad. The "find-bugs" error that I was trying to avoid was always "don't write
to static from dynamic". Turns out that "using static from dynamic" is OK. Done
(here and in PaymentRequestUI and CardUnmaskPrompt).

[gone, 2016-06-10 22:37:33]

Java bits lgtm

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2048823004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:132:
/**
On 2016/06/10 22:31:58, Rouslan (ツ) wrote:
> My bad. The "find-bugs" error that I was trying to avoid was always "don't
write
> to static from dynamic". Turns out that "using static from dynamic" is OK.
Done
> (here and in PaymentRequestUI and CardUnmaskPrompt).

Yeah, IIRC the reason is that you can have multiple threads via multiple
instances setting the same static variable, meaning that whatever singleton you
thought you had is not actually one.  It complains a little less if you have an
explicit static function for modifying it, but you sometimes still have to
synchronously block on setting it.

[please use gerrit instead, 2016-06-11 00:57:12]

Description was changed from

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

to

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Define ScriptStateForTestingWithPage in V8BindingFroTest.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

[haraken, 2016-06-13 01:31:10]

Sorry for asking many things in this CL iteratively...

The idea is that I'd like to improve V8TestingScope so that all unit tests can
just use V8TestingScope to get mock objects required for binding tests (e.g.,
script state, execution context, exception state etc) instead of adding adding
the mock objects to PaymentRequestTestBase.

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.h (right):

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.h:31: class
ScriptStateForTestingWithPage {

Or can you just add DummyPageHolder to ScriptStateForTesting instead of adding a
new testing class?

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:45:
TrackExceptionState m_exceptionState;

V8TestingScope is an abstraction class that encapsulates ScriptStateForTesting
etc. Would it be possible to add TrackExceptionState to V8TestingScope and use
it for payment tests?

Ideally, tests that require bindings should be written as follows:

TEST_F(...) {
  V8TestingScope scope;
  scope.xxx();  // You can get whatever you want (e.g., page, execution context,
script state etc) from the scope.
}

[please use gerrit instead, 2016-06-14 05:12:02]

Description was changed from

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Define ScriptStateForTestingWithPage in V8BindingFroTest.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

to

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

[please use gerrit instead, 2016-06-14 05:12:40]

haraken@, ptal patch 6. V8TestingScope changes have been moved to
http://crrev.com/2061073003.

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.h (right):

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8BindingForTesting.h:31: class
ScriptStateForTestingWithPage {
On 2016/06/13 01:31:10, haraken wrote:
> 
> Or can you just add DummyPageHolder to ScriptStateForTesting instead of adding
a
> new testing class?

Done.

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:45:
TrackExceptionState m_exceptionState;
On 2016/06/13 01:31:10, haraken wrote:
> 
> V8TestingScope is an abstraction class that encapsulates ScriptStateForTesting
> etc. Would it be possible to add TrackExceptionState to V8TestingScope and use
> it for payment tests?
> 
> Ideally, tests that require bindings should be written as follows:
> 
> TEST_F(...) {
>   V8TestingScope scope;
>   scope.xxx();  // You can get whatever you want (e.g., page, execution
context,
> script state etc) from the scope.
> }

Done.

[please use gerrit instead, 2016-06-14 05:14:32]

On 2016/06/14 05:12:40, Rouslan (ツ) wrote:
> Done.

... in http://crrev.com/2061073003.

[haraken, 2016-06-14 05:24:36]

LGTM

https://codereview.chromium.org/2048823004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:45:
OwnPtr<DummyPageHolder> m_holder;

Remove this?

https://codereview.chromium.org/2048823004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:46:
TrackExceptionState m_exceptionState;

Remove this?

[please use gerrit instead, 2016-06-15 04:13:19]

Sending to cq.

https://codereview.chromium.org/2048823004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h
(right):

https://codereview.chromium.org/2048823004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:45:
OwnPtr<DummyPageHolder> m_holder;
On 2016/06/14 05:24:35, haraken wrote:
> 
> Remove this?

Done.

https://codereview.chromium.org/2048823004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTestBase.h:46:
TrackExceptionState m_exceptionState;
On 2016/06/14 05:24:35, haraken wrote:
> 
> Remove this?

Done.

[please use gerrit instead, 2016-06-15 04:13:27]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-06-15 04:13:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
dfalcantara@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2048823004/#ps220001
(title: "Remove DummyPageHolder and TrackExceptionState for haraken@ comment.")

[commit-bot: I haz the power, 2016-06-15 04:13:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2048823004/220001

[commit-bot: I haz the power, 2016-06-15 06:49:03]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-06-15 06:49:06]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 06:53:06]

Description was changed from

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193
==========

to

==========
PaymentRequest.abort() should return a promise.

The abort() method on an instance of PaymentRequest returns a promise
that is resolved when the browser has hidden its UI. If the browser is
unable to abort payment, then the abort() promise is rejected. This can
happen, for example, when the browser has launched a 3rd-party payment
app and is unable to abort it.

PaymentRequest interface:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...

The abort() method:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#abort

The pull request that introduced the promise:
https://github.com/w3c/browser-payment-api/pull/190/files

Additional changes in this patch:
  * Move abort() tests into AbortTest.cpp
  * Move common test fixture into PaymentRequestTestBase.h/cpp.
  * Actually fail tests if MockFunction is not called as expected.
  * Fix RejectShowPromiseWithRequestShippingFalseAndShippingAddressInResponse

BUG=617193

Committed: https://crrev.com/e411df45423c6c1fd6acb338ce0ea48630784b18
Cr-Commit-Position: refs/heads/master@{#399854}
==========

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

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