EarlGrey tests for Payment Request (base CL)

EarlGrey tests for Payment Request (base CL)

BUG=602666
Review-Url: https://codereview.chromium.org/2588913002
Cr-Commit-Position: refs/heads/master@{#443319}
Committed: https://chromium.googlesource.com/chromium/src/+/07f18d68d972f8e49c93c699ff61f774fc0584ff

[Moe, 2016-12-19 21:33:37]

Description was changed from

==========
eg test for Payment Request

BUG=602666
==========

to

==========
EarlGrey tests for Payment Request (base CL)

BUG=602666
==========

[Moe, 2016-12-19 21:44:09]

mahmadi@chromium.org changed reviewers:
+ jdonnelly@chromium.org

[Moe, 2016-12-19 21:44:09]

Hi Justin, Please review this CL.

[Justin Donnelly, 2016-12-29 18:36:57]

jdonnelly@chromium.org changed reviewers:
+ lpromero@chromium.org

[Justin Donnelly, 2016-12-29 18:36:58]

Sorry again for the slow turnaround on this.

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:975: <message
name="IDS_IOS_ACCNAME_PAYMENT_REQUEST_PAY_BUTTON" desc="The accessible name for
the button to confirm that the user wishes to pay for a good or service [iOS
only].">
Is this used? I didn't see it when looking through this CL. Regardless, I don't
think we should need it. The button already has a text label which will be used
for accessibility.

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
I'm skeptical that this is ok. The web state has a rather complex life cycle I
believe based on navigations, redirects, going back, reloads, etc. I copied this
particular setup from another JS injected feature and I assume it was designed
carefully to work in all circumstances. Maybe lpromero knows or else we can
track down someone who can better answer than I can if this will work in all
cases.

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_utils.mm (right):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_utils.mm:16:
profile->GetInfo(autofill::AutofillType(autofill::NAME_FULL),
Is there a motivation for switching to GetInfo instead of GetRawInfo? I don't
think it makes any difference in this case?

[Moe, 2017-01-03 17:42:20]

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:975: <message
name="IDS_IOS_ACCNAME_PAYMENT_REQUEST_PAY_BUTTON" desc="The accessible name for
the button to confirm that the user wishes to pay for a good or service [iOS
only].">
On 2016/12/29 18:36:57, Justin Donnelly wrote:
> Is this used? I didn't see it when looking through this CL. Regardless, I
don't
> think we should need it. The button already has a text label which will be
used
> for accessibility.

Done. It was being used in payment_request_view_controller.mm, but you're right.
It's not needed.

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_utils.mm (right):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_utils.mm:16:
profile->GetInfo(autofill::AutofillType(autofill::NAME_FULL),
On 2016/12/29 18:36:57, Justin Donnelly wrote:
> Is there a motivation for switching to GetInfo instead of GetRawInfo? I don't
> think it makes any difference in this case?

There could be cases, like in the test data, that the name is stored as separate
first, middle, and last components rather than the full thing. GetInfo does the
patching if that's the case whereas GetRawInfo would return an empty string.

[Moe, 2017-01-05 05:44:27]

Patchset #4 (id:60001) has been deleted

[lpromero, 2017-01-05 10:50:12]

lgtm

[Moe, 2017-01-05 15:38:19]

mahmadi@chromium.org changed reviewers:
+ rohitrao@chromium.org

[Moe, 2017-01-05 15:38:19]

rohitrao@ please owners review ios/chrome/browser/ui/

[Moe, 2017-01-06 16:16:27]

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
On 2016/12/29 18:36:57, Justin Donnelly wrote:
> I'm skeptical that this is ok. The web state has a rather complex life cycle I
> believe based on navigations, redirects, going back, reloads, etc. I copied
this
> particular setup from another JS injected feature and I assume it was designed
> carefully to work in all circumstances. Maybe lpromero knows or else we can
> track down someone who can better answer than I can if this will work in all
> cases.

lpromero@ I was wondering if you had any feedback on the point justin made.

[lpromero, 2017-01-06 16:20:02]

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
On 2017/01/06 16:16:27, moe wrote:
> On 2016/12/29 18:36:57, Justin Donnelly wrote:
> > I'm skeptical that this is ok. The web state has a rather complex life cycle
I
> > believe based on navigations, redirects, going back, reloads, etc. I copied
> this
> > particular setup from another JS injected feature and I assume it was
designed
> > carefully to work in all circumstances. Maybe lpromero knows or else we can
> > track down someone who can better answer than I can if this will work in all
> > cases.
> 
> lpromero@ I was wondering if you had any feedback on the point justin made.

eugenebut@ would probably be the right person to ask.

[Moe, 2017-01-06 16:25:20]

mahmadi@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Moe, 2017-01-06 16:25:21]

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
On 2017/01/06 16:20:01, lpromero wrote:
> On 2017/01/06 16:16:27, moe wrote:
> > On 2016/12/29 18:36:57, Justin Donnelly wrote:
> > > I'm skeptical that this is ok. The web state has a rather complex life
cycle
> I
> > > believe based on navigations, redirects, going back, reloads, etc. I
copied
> > this
> > > particular setup from another JS injected feature and I assume it was
> designed
> > > carefully to work in all circumstances. Maybe lpromero knows or else we
can
> > > track down someone who can better answer than I can if this will work in
all
> > > cases.
> > 
> > lpromero@ I was wondering if you had any feedback on the point justin made.
> 
> eugenebut@ would probably be the right person to ask.

eugenebut@ could please chime in?

[Eugene But (OOO till 7-30), 2017-01-06 17:36:22]

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
On 2017/01/06 16:25:21, moe wrote:
> On 2017/01/06 16:20:01, lpromero wrote:
> > On 2017/01/06 16:16:27, moe wrote:
> > > On 2016/12/29 18:36:57, Justin Donnelly wrote:
> > > > I'm skeptical that this is ok. The web state has a rather complex life
> cycle
> > I
> > > > believe based on navigations, redirects, going back, reloads, etc. I
> copied
> > > this
> > > > particular setup from another JS injected feature and I assume it was
> > designed
> > > > carefully to work in all circumstances. Maybe lpromero knows or else we
> can
> > > > track down someone who can better answer than I can if this will work in
> all
> > > > cases.
> > > 
> > > lpromero@ I was wondering if you had any feedback on the point justin
made.
> > 
> > eugenebut@ would probably be the right person to ask.
> 
> eugenebut@ could please chime in?
I don't know what |enableCurrentWebState| does. This method has no comment.

[Justin Donnelly, 2017-01-06 18:21:07]

jdonnelly@chromium.org changed reviewers:
+ marq@chromium.org

[Justin Donnelly, 2017-01-06 18:21:08]

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
On 2017/01/06 17:36:22, Eugene But wrote:
> On 2017/01/06 16:25:21, moe wrote:
> > On 2017/01/06 16:20:01, lpromero wrote:
> > > On 2017/01/06 16:16:27, moe wrote:
> > > > On 2016/12/29 18:36:57, Justin Donnelly wrote:
> > > > > I'm skeptical that this is ok. The web state has a rather complex life
> > cycle
> > > I
> > > > > believe based on navigations, redirects, going back, reloads, etc. I
> > copied
> > > > this
> > > > > particular setup from another JS injected feature and I assume it was
> > > designed
> > > > > carefully to work in all circumstances. Maybe lpromero knows or else
we
> > can
> > > > > track down someone who can better answer than I can if this will work
in
> > all
> > > > > cases.
> > > > 
> > > > lpromero@ I was wondering if you had any feedback on the point justin
> made.
> > > 
> > > eugenebut@ would probably be the right person to ask.
> > 
> > eugenebut@ could please chime in?
> I don't know what |enableCurrentWebState| does. This method has no comment. 

I copied the existing pattern of calls and state transitions from
contextual_search_controller.mm. marq@, did you make that? Can you comment on
whether the new setup mahmadi@ is making here will work in all circumstances?

[marq (ping after 24h), 2017-01-09 17:55:38]

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
On 2017/01/06 18:21:07, Justin Donnelly wrote:
> On 2017/01/06 17:36:22, Eugene But wrote:
> > On 2017/01/06 16:25:21, moe wrote:
> > > On 2017/01/06 16:20:01, lpromero wrote:
> > > > On 2017/01/06 16:16:27, moe wrote:
> > > > > On 2016/12/29 18:36:57, Justin Donnelly wrote:
> > > > > > I'm skeptical that this is ok. The web state has a rather complex
life
> > > cycle
> > > > I
> > > > > > believe based on navigations, redirects, going back, reloads, etc. I
> > > copied
> > > > > this
> > > > > > particular setup from another JS injected feature and I assume it
was
> > > > designed
> > > > > > carefully to work in all circumstances. Maybe lpromero knows or else
> we
> > > can
> > > > > > track down someone who can better answer than I can if this will
work
> in
> > > all
> > > > > > cases.
> > > > > 
> > > > > lpromero@ I was wondering if you had any feedback on the point justin
> > made.
> > > > 
> > > > eugenebut@ would probably be the right person to ask.
> > > 
> > > eugenebut@ could please chime in?
> > I don't know what |enableCurrentWebState| does. This method has no comment. 
> 
> I copied the existing pattern of calls and state transitions from
> contextual_search_controller.mm. marq@, did you make that? Can you comment on
> whether the new setup mahmadi@ is making here will work in all circumstances?

I'll have to look over it carefully; the use cases for
ContextualSearchController are fairly specific.

[marq (ping after 24h), 2017-01-11 15:09:58]

I've commented on the impact of removing the -enableCurrentWebState calls; note
that ContextualSearchManager has to handle things like VoiecOver being enabled
or the user turning off the contextual search preference while a web page is
loaded. If those aren't use cases for payment requests, then the API for this
class can probably be simplified, and the some of the methods that are calling
-enableCurrentWebState can go away.

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
Removing this call means that the payment request JS won't be injected into
|webState| until the next time it calls the -webPageLoadedWithStatus: observer
method.

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:149: [self
enableCurrentWebState];
Removing this call means that if -enablePaymentRequest: is called, changing the
current value of _enabled, the change won't be reflected in a currently-loaded
web page.

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:169: if (![self webState]
|| !experimental_flags::IsPaymentRequestEnabled()) {
I would prefer that the experimental flag gates creating an instance of this
class (at all) rather than just stopping the JS injection.

[Moe, 2017-01-11 16:45:09]

On 2017/01/11 15:09:58, marq wrote:
> I've commented on the impact of removing the -enableCurrentWebState calls;
note
> that ContextualSearchManager has to handle things like VoiecOver being enabled
> or the user turning off the contextual search preference while a web page is
> loaded. If those aren't use cases for payment requests, then the API for this
> class can probably be simplified, and the some of the methods that are calling
> -enableCurrentWebState can go away.
> 
>
https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
> File ios/chrome/browser/payments/payment_request_manager.mm (left):
> 
>
https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/payment_request_manager.mm:126: [self
> enableCurrentWebState];
> Removing this call means that the payment request JS won't be injected into
> |webState| until the next time it calls the -webPageLoadedWithStatus: observer
> method.
> 
>
https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/payment_request_manager.mm:149: [self
> enableCurrentWebState];
> Removing this call means that if -enablePaymentRequest: is called, changing
the
> current value of _enabled, the change won't be reflected in a currently-loaded
> web page.
> 
>
https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
> File ios/chrome/browser/payments/payment_request_manager.mm (right):
> 
>
https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/payment_request_manager.mm:169: if (![self
webState]
> || !experimental_flags::IsPaymentRequestEnabled()) {
> I would prefer that the experimental flag gates creating an instance of this
> class (at all) rather than just stopping the JS injection.


AFIK Payment Request doesn't have to handle such complex scenarios. I don't even
think there are plans for having an explicit switch for the user to turn it off
since merchants may rely on it for their checkout flow to work at all. It
basically has to be available on secure web pages.
I initially removed the calls in setWebState: and doEnablePaymentRequest:
because, in the EarlGrey test, injecting the JS into the |webState| before a web
page was loaded was causing a one of a DCHECKs in [1] to fail. Putting all the
calls back, I'm quite surprised to see the EG tests pass! I don't know what's
changed between then and now but I'm perfectly fine with leaving these calls
alone.
[1]:
https://cs.chromium.org/chromium/src/ios/web/web_state/web_state_impl.mm?l=686

[Moe, 2017-01-11 16:45:19]

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (left):

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:126: [self
enableCurrentWebState];
On 2017/01/11 15:09:57, marq wrote:
> Removing this call means that the payment request JS won't be injected into
> |webState| until the next time it calls the -webPageLoadedWithStatus: observer
> method. 

does the JS really need to be injected when switching tabs or inserting a new
one, before a webpage is actually loaded? pageLoadedWithStatus: observer method
seems to work reliably with different reload/navigation scenarios.

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:149: [self
enableCurrentWebState];
On 2017/01/11 15:09:57, marq wrote:
> Removing this call means that if -enablePaymentRequest: is called, changing
the
> current value of _enabled, the change won't be reflected in a currently-loaded
> web page.

enablePaymentRequest: gets called in BrowserViewController's setActive:
What are the scenario's where bvc is toggled between active and inactive?

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2588913002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:169: if (![self webState]
|| !experimental_flags::IsPaymentRequestEnabled()) {
On 2017/01/11 15:09:57, marq wrote:
> I would prefer that the experimental flag gates creating an instance of this
> class (at all) rather than just stopping the JS injection.

I Agree. But I found no way of adding a command line switch to enable this
feature in the EG test before an instance of the BrowserViewController is
created.

[Justin Donnelly, 2017-01-11 16:50:07]

lgtm

marq - Thanks for looking at the lifecycle issues and explaining them, that was
very helpful.

mahmadi - thanks for taking the time to consider how these affect Payment
Request. Given that you'll be responsible for what works and doesn't work going
forward, I'm comfortable with whatever approach you feel is best. I just wanted
to make sure we all had a reasonable understanding of these issues.

[Moe, 2017-01-11 20:22:45]

Patchset #4 (id:80001) has been deleted

[marq (ping after 24h), 2017-01-12 17:46:36]

manager/BVC (which now have no changes ;-) LGTM.

[rohitrao (ping after 24h), 2017-01-12 17:51:17]

I don't think there's anything additional that I can give OWNERS approval for. 
You may still need someone to approve the strings changes.

[Moe, 2017-01-12 18:08:14]

mahmadi@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Moe, 2017-01-12 18:08:15]

sdefresne@ please owners review components/components_strings.grd

[sdefresne, 2017-01-12 18:17:33]

lgtm

[Moe, 2017-01-12 18:28:05]

On 2017/01/12 18:17:33, sdefresne wrote:
> lgtm

Thanks everyone!

[Moe, 2017-01-12 18:28:10]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-01-12 18:28:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdonnelly@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2588913002/#ps120001
(title: "Moved the experimental flag back to the BVC")

[commit-bot: I haz the power, 2017-01-12 18:28:47]

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, 2017-01-12 19:16:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484245690757520,
"parent_rev": "fc4c96b992fc256fe2bb8819ba6a600580b25900", "commit_rev":
"07f18d68d972f8e49c93c699ff61f774fc0584ff"}

[commit-bot: I haz the power, 2017-01-12 19:16:57]

Description was changed from

==========
EarlGrey tests for Payment Request (base CL)

BUG=602666
==========

to

==========
EarlGrey tests for Payment Request (base CL)

BUG=602666

Review-Url: https://codereview.chromium.org/2588913002
Cr-Commit-Position: refs/heads/master@{#443319}
Committed:
https://chromium.googlesource.com/chromium/src/+/07f18d68d972f8e49c93c699ff61...
==========

[commit-bot: I haz the power, 2017-01-12 19:17:01]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/07f18d68d972f8e49c93c699ff61...

[lpromero, 2017-01-16 17:47:16]

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

The reason for reverting is: This broke several ExternalURLSSLTestCase
downstream  with this DCHECK:
[0116/083204.532380:FATAL:payment_request_manager.mm(206)] Check failed:
_webStateEnabled.
 .