Update the shippingType option to not allow invalid strings.

Update the shippingType option to not allow invalid strings.

spec list:
https://w3c.github.io/browser-payment-api/#dom-paymentoptions
https://github.com/w3c/browser-payment-api/pull/416

BUG=690928
Review-Url: https://codereview.chromium.org/2685343002
Cr-Commit-Position: refs/heads/master@{#450379}
Committed: https://chromium.googlesource.com/chromium/src/+/6396f2506e7fa1793a7223f2a1a191fb1316f56e

[Hwanseung Lee, 2017-02-10 14:29:09]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 14:29:45]

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

[Hwanseung Lee, 2017-02-10 14:30:40]

Description was changed from

==========
Update the shippingType option to not allow invalid strings.

spec list:
https://w3c.github.io/browser-payment-api/#dom-paymentoptions
https://github.com/w3c/browser-payment-api/pull/416

BUG=690928
==========

to

==========
Update the shippingType option to not allow invalid strings.

spec list:
https://w3c.github.io/browser-payment-api/#dom-paymentoptions
https://github.com/w3c/browser-payment-api/pull/416

BUG=690928
==========

[commit-bot: I haz the power, 2017-02-10 16:00:04]

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

[commit-bot: I haz the power, 2017-02-10 16:00:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Hwanseung Lee, 2017-02-11 23:30:42]

The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 23:30:53]

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, 2017-02-12 01:22:39]

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

[commit-bot: I haz the power, 2017-02-12 01:22:40]

Dry run: This issue passed the CQ dry run.

[Hwanseung Lee, 2017-02-13 14:49:03]

hs1217.lee@samsung.com changed reviewers:
+ jinho.bang@samsung.com, rouslan@chromium.org

[Hwanseung Lee, 2017-02-13 14:49:44]

@rouslan, zino
PTAL, thank you.

[zino, 2017-02-13 15:15:06]

lgtm

[please use gerrit instead, 2017-02-13 15:36:14]

Since this will change the interface, please send out an intent to implement and
ship to blink-dev@chromium.org. The instructions are here:
https://www.chromium.org/blink/launching-features. Please let me know if you
need help with this.

[zino, 2017-02-13 22:28:52]

On 2017/02/13 15:36:14, rouslan wrote:
> Since this will change the interface, please send out an intent to implement
and
> ship to mailto:blink-dev@chromium.org. The instructions are here:
> https://www.chromium.org/blink/launching-features. Please let me know if you
> need help with this.

IMHO, this might be a trivial change as per webplatform change guideline (and my
past experiences).
The guide says that:
"Trivial platform changes do not need to meet the requirements above. For
example, changes to existing APIs to improve compliance with web standards or to
fix bugs are welcome."

The PaymentRequest interface has 'shippingType' attribute but this change
doesn't affect anything to it.
This only changes the shippingType member of input dictionary from DOMString to
enum (restricted DOMString).
Before and after this change, the allowed types of shippingType are the same
fully.

Also, if we still need a intent, we probably should send a intent to deprecate
(or/and remove) first. (and send a intent to implement and ship later)
But how..? We might investigate a use count in use PaymentRequest constructor
with invalid shippingType.

[zino, 2017-02-13 22:29:49]

jinho.bang@samsung.com changed reviewers:
+ foolip@chromium.org

[zino, 2017-02-13 22:31:14]

To clarify this, I'll add foolip@ as API owner. :)

Thank you.

[zino, 2017-02-13 23:15:14]

On 2017/02/13 22:31:14, zino wrote:
> To clarify this, I'll add foolip@ as API owner. :)
> 
> Thank you.

For foolip@ and other API owners,

This CL only changes the shippingType member of input dictionary from
DOMString to enum (restricted DOMString). Before this patch, we used
DOMString but only allowed restricted strings according to the following spec.

 - https://w3c.github.io/browser-payment-api/#constructor
 "If options.requestShipping is set to true, then set the value of the
shippingType
 attribute on request to options.shippingType. If options.shippingType is
 not a valid PaymentShippingType value then set the value of the shippingType
 attribute on request to "shipping"."

So, after this change, we only allow (the same) restricted strings as before.

But, it can be slightly different when we pass an invalid string.
Before this patch, the invalid string is just ignored (and use default value
like enum in interface attribute)
After this patch, throws TypeError as per WebIDL spec
(https://heycam.github.io/webidl/#es-enumeration).

I'm not sure if we should cover abnormal cases?
If we really need to cover it, we should do but it's a very edge case and an
abnormal case.

[foolip, 2017-02-14 13:32:05]

In PaymentRequest.cpp, can you also add a DCHECK in the final else branch, to
show that bindings in fact guarantees that no other strings are used?

https://codereview.chromium.org/2685343002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/payments/payment-request-interface.html
(right):

https://codereview.chromium.org/2685343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/payments/payment-request-interface.html:328:
['Undefined PaymentShippingType value for shppingType should throw a TypeError',
new TypeError(), function() {
This test does not test undefined, but an invalid value. FWIW, I think that a
single test will suffice, everything else will be stringified and just be
testing other invalid strings.

[foolip, 2017-02-14 13:32:05]

In PaymentRequest.cpp, can you also add a DCHECK in the final else branch, to
show that bindings in fact guarantees that no other strings are used?

[foolip, 2017-02-14 13:34:56]

On 2017/02/13 23:15:14, zino wrote:
> On 2017/02/13 22:31:14, zino wrote:
> > To clarify this, I'll add foolip@ as API owner. :)
> > 
> > Thank you.
> 
> For foolip@ and other API owners,
> 
> This CL only changes the shippingType member of input dictionary from
> DOMString to enum (restricted DOMString). Before this patch, we used
> DOMString but only allowed restricted strings according to the following spec.
> 
>  - https://w3c.github.io/browser-payment-api/#constructor
>  "If options.requestShipping is set to true, then set the value of the
> shippingType
>  attribute on request to options.shippingType. If options.shippingType is
>  not a valid PaymentShippingType value then set the value of the shippingType
>  attribute on request to "shipping"."
> 
> So, after this change, we only allow (the same) restricted strings as before.
> 
> But, it can be slightly different when we pass an invalid string.
> Before this patch, the invalid string is just ignored (and use default value
> like enum in interface attribute)
> After this patch, throws TypeError as per WebIDL spec
> (https://heycam.github.io/webidl/#es-enumeration).
> 
> I'm not sure if we should cover abnormal cases?
> If we really need to cover it, we should do but it's a very edge case and an
> abnormal case.

lgtm, if you have no reason to suspect that this change will break any existing
content, then just making the change without any other process is fine.

It would be nice if
https://github.com/w3c/web-platform-tests/tree/master/payment-request were
imported (see W3CImportExpectations) and trivial changes like this were tested
using web-platform-tests instead. There's no requirement to do so, just very
much appreciated.

[foolip, 2017-02-14 13:35:16]

On 2017/02/14 13:34:56, foolip wrote:
> On 2017/02/13 23:15:14, zino wrote:
> > On 2017/02/13 22:31:14, zino wrote:
> > > To clarify this, I'll add foolip@ as API owner. :)
> > > 
> > > Thank you.
> > 
> > For foolip@ and other API owners,
> > 
> > This CL only changes the shippingType member of input dictionary from
> > DOMString to enum (restricted DOMString). Before this patch, we used
> > DOMString but only allowed restricted strings according to the following
spec.
> > 
> >  - https://w3c.github.io/browser-payment-api/#constructor
> >  "If options.requestShipping is set to true, then set the value of the
> > shippingType
> >  attribute on request to options.shippingType. If options.shippingType is
> >  not a valid PaymentShippingType value then set the value of the
shippingType
> >  attribute on request to "shipping"."
> > 
> > So, after this change, we only allow (the same) restricted strings as
before.
> > 
> > But, it can be slightly different when we pass an invalid string.
> > Before this patch, the invalid string is just ignored (and use default value
> > like enum in interface attribute)
> > After this patch, throws TypeError as per WebIDL spec
> > (https://heycam.github.io/webidl/#es-enumeration).
> > 
> > I'm not sure if we should cover abnormal cases?
> > If we really need to cover it, we should do but it's a very edge case and an
> > abnormal case.
> 
> lgtm, if you have no reason to suspect that this change will break any
existing
> content, then just making the change without any other process is fine.
> 
> It would be nice if
> https://github.com/w3c/web-platform-tests/tree/master/payment-request were
> imported (see W3CImportExpectations) and trivial changes like this were tested
> using web-platform-tests instead. There's no requirement to do so, just very
> much appreciated.

See
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
for how all of that works.

[please use gerrit instead, 2017-02-14 14:57:20]

lgtm

[Hwanseung Lee, 2017-02-14 15:02:35]

The CQ bit was checked by hs1217.lee@samsung.com

[commit-bot: I haz the power, 2017-02-14 15:03:16]

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-02-14 16:35:21]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487084555068580,
"parent_rev": "0bc6a07e9b04a8885f9e206f035e7c2e011b132c", "commit_rev":
"6396f2506e7fa1793a7223f2a1a191fb1316f56e"}

[commit-bot: I haz the power, 2017-02-14 16:35:54]

Description was changed from

==========
Update the shippingType option to not allow invalid strings.

spec list:
https://w3c.github.io/browser-payment-api/#dom-paymentoptions
https://github.com/w3c/browser-payment-api/pull/416

BUG=690928
==========

to

==========
Update the shippingType option to not allow invalid strings.

spec list:
https://w3c.github.io/browser-payment-api/#dom-paymentoptions
https://github.com/w3c/browser-payment-api/pull/416

BUG=690928

Review-Url: https://codereview.chromium.org/2685343002
Cr-Commit-Position: refs/heads/master@{#450379}
Committed:
https://chromium.googlesource.com/chromium/src/+/6396f2506e7fa1793a7223f2a1a1...
==========

[commit-bot: I haz the power, 2017-02-14 16:35:55]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6396f2506e7fa1793a7223f2a1a1...