PaymentRequest: The currencySystem should be non-nullable.

PaymentRequest: The currencySystem should be non-nullable.

The currencySystem attribute is defined in PaymentCurrencyAmount interface
to indicate the currency system that the currency identifier belongs to.
The spec[1] defines it as non-nullable, and our current IDL implementation
follows it well. However, its mapping value for mojom is nullable. It looks
strange. As a result, in browser side, we are unnecessarily checking whether
the value is null or not everytime.
Therefore, This CL is fixing the value as non-nullable and remove unncessary
checks. This is initiated from Tommy's comment(tommyt@opera.com) on the code
review[2].

[1] https://w3c.github.io/browser-payment-api/#paymentcurrencyamount
[2] https://codereview.chromium.org/2646313002/#msg5

BUG=none

Review-Url: https://codereview.chromium.org/2649143003
Cr-Commit-Position: refs/heads/master@{#445725}
Committed: https://chromium.googlesource.com/chromium/src/+/427645407dc20735347e185b9875b04a26c92f14

[zino, 2017-01-23 13:43:53]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 13:44:04]

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

[zino, 2017-01-23 13:44:15]

Description was changed from

==========
PaymentRequest: The currencySystem should be non-nullable.

The currencySystem attribute is defined in PaymentCurrencyAmount interface
to indicate the currency system that the currency identifier belongs to.
The spec[1] defines it as non-nullable, and our current IDL implementation
follows it well. However, its mapping value for mojom is nullable. It looks
strange. As a result, in browser side, we are unnecessarily checking whether
the value is null or not everytime.
Therefore, This CL is fixing the value as non-nullable and remove unncessary
checks. This is initiated from Tommy's comment(tommyt@opera.com) on the code
review[2].

[1] https://w3c.github.io/browser-payment-api/#paymentcurrencyamount
[2] https://codereview.chromium.org/2646313002/#msg5

BUG=none
==========

to

==========
PaymentRequest: The currencySystem should be non-nullable.

The currencySystem attribute is defined in PaymentCurrencyAmount interface
to indicate the currency system that the currency identifier belongs to.
The spec[1] defines it as non-nullable, and our current IDL implementation
follows it well. However, its mapping value for mojom is nullable. It looks
strange. As a result, in browser side, we are unnecessarily checking whether
the value is null or not everytime.
Therefore, This CL is fixing the value as non-nullable and remove unncessary
checks. This is initiated from Tommy's comment(tommyt@opera.com) on the code
review[2].

[1] https://w3c.github.io/browser-payment-api/#paymentcurrencyamount
[2] https://codereview.chromium.org/2646313002/#msg5

BUG=none
==========

[zino, 2017-01-23 13:44:15]

jinho.bang@samsung.com changed reviewers:
+ rouslan@chromium.org, tommyt@opera.com

[zino, 2017-01-23 13:44:41]

PTAL.

[commit-bot: I haz the power, 2017-01-23 13:45:15]

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

[commit-bot: I haz the power, 2017-01-23 13:45:16]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, no
build URL)

[please use gerrit instead, 2017-01-23 13:49:26]

Hold on, how is this value non-nullable? :-D I thought a developer can write
currencySystem:null or currencySystem:undefined in their JavaScript and that
would be valid... Here's the spec IDL and language.

------------------------------------------------
dictionary PaymentCurrencyAmount {
    required DOMString currency;
    required DOMString value;
             DOMString currencySystem = "urn:iso:std:iso:4217";
};

A PaymentCurrencyAmount dictionary is used to supply monetary amounts.

The following fields are required:

currencySystem - A URL that indicates the currency system that the currency
identifier belongs to. By default, the value is urn:iso:std:iso:4217 indicating
that currency is defined by [ISO4217] (for example, USD for US Dollars).
------------------------------------------------

The text "The following fields are required" is confusing here, because
"currencySystem" does not have the "required" keyword in WebIDL. I'll file a bug
about this. Is that what confused you as well?

[please use gerrit instead, 2017-01-23 13:52:45]

Filed https://github.com/w3c/browser-payment-api/issues/408 to discuss this with
the working group.

[tommyt, 2017-01-23 13:52:47]

I agree with Rouslan. It makes sense to keep this field optional, but the string
"The following fields are required:" should be deleted from the specification.

[please use gerrit instead, 2017-01-23 14:30:53]

LGTM given the discussion in W3C github. Looks like only "DOMString?" type
allows null values, while "undefined" values result in the default value being
used.

[zino, 2017-01-23 14:39:33]

On 2017/01/23 13:49:26, rouslan wrote:
> Hold on, how is this value non-nullable? :-D I thought a developer can write
> currencySystem:null or currencySystem:undefined in their JavaScript and that
> would be valid... Here's the spec IDL and language.
> 
> ------------------------------------------------
> dictionary PaymentCurrencyAmount {
>     required DOMString currency;
>     required DOMString value;
>              DOMString currencySystem = "urn:iso:std:iso:4217";
> };
> 
> A PaymentCurrencyAmount dictionary is used to supply monetary amounts.
> 
> The following fields are required:
> 
> currencySystem - A URL that indicates the currency system that the currency
> identifier belongs to. By default, the value is urn:iso:std:iso:4217
indicating
> that currency is defined by [ISO4217] (for example, USD for US Dollars).
> ------------------------------------------------
> 
> The text "The following fields are required" is confusing here, because
> "currencySystem" does not have the "required" keyword in WebIDL. I'll file a
bug
> about this. Is that what confused you as well?

Hmm, do we need to allow 'null' value? I'm not sure if we need it.
The currencySystem attribute has a default value and defines as non-nullable in
IDL.
In the current implementation, the value should not be null anyway.

Even if JS authors pass null or undefined, the currencySystem attribute is set
as some
string value(non-null) because its IDL definition is non-nullable.

Here are some examples (based on current implementation):
[1] { currency: '...', value: '...' } -> currencySystem ==
"urn:iso:std:iso:4217"
[2] { currency: '...', value: '...', currencySystem: null } -> currencySystem ==
"null"
[3] { currency: '...', value: '...', currencySystem: undefined } ->
currencySystem == "urn:iso:std:iso:4217"
[4] { currency: '...', value: '...', currencySystem: "something" } ->
currencySystem == "something"

Before the currencySystem value is sent to browser side, it must has some
string.
But mojom definition allows the value nullable even if it can not be null.

[zino, 2017-01-23 14:40:38]

Ah, sorry, I didn't see your previous comment.

Thanks! I'll fix this patch again.

[tommyt, 2017-01-23 14:46:39]

On 2017/01/23 14:30:53, rouslan wrote:
> LGTM given the discussion in W3C github. Looks like only "DOMString?" type
> allows null values, while "undefined" values result in the default value being
> used.

Aha, now I understand. In this case this change LGTM to me also. I guess the
confusing thing is the "required" keyword (and lack of such for the
currencySystem field) in the spec's WebIDL.

[zino, 2017-01-23 15:01:14]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 15:01:25]

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-01-23 15:02:07]

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

[commit-bot: I haz the power, 2017-01-23 15:02:08]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[zino, 2017-01-23 15:04:29]

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

[zino, 2017-01-23 15:04:53]

+Tom Sepez for mojom file.

[zino, 2017-01-23 16:20:47]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 16:21:05]

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-01-23 18:04:19]

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

[commit-bot: I haz the power, 2017-01-23 18:04:20]

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...)

[Tom Sepez, 2017-01-23 18:26:13]

lgtm

[zino, 2017-01-24 12:40:31]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 12:40:46]

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-01-24 13:11:40]

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

[commit-bot: I haz the power, 2017-01-24 13:11:40]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[zino, 2017-01-24 14:29:38]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2017-01-24 14:29:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, tommyt@opera.com, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2649143003/#ps60001
(title: "PaymentRequest: The currencySystem should be non-nullable.")

[commit-bot: I haz the power, 2017-01-24 14:29:59]

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-24 14:42:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485268178215330,
"parent_rev": "3a41b46b3f45c62b5d49296f985efd9118e65736", "commit_rev":
"427645407dc20735347e185b9875b04a26c92f14"}

[commit-bot: I haz the power, 2017-01-24 14:43:30]

Description was changed from

==========
PaymentRequest: The currencySystem should be non-nullable.

The currencySystem attribute is defined in PaymentCurrencyAmount interface
to indicate the currency system that the currency identifier belongs to.
The spec[1] defines it as non-nullable, and our current IDL implementation
follows it well. However, its mapping value for mojom is nullable. It looks
strange. As a result, in browser side, we are unnecessarily checking whether
the value is null or not everytime.
Therefore, This CL is fixing the value as non-nullable and remove unncessary
checks. This is initiated from Tommy's comment(tommyt@opera.com) on the code
review[2].

[1] https://w3c.github.io/browser-payment-api/#paymentcurrencyamount
[2] https://codereview.chromium.org/2646313002/#msg5

BUG=none
==========

to

==========
PaymentRequest: The currencySystem should be non-nullable.

The currencySystem attribute is defined in PaymentCurrencyAmount interface
to indicate the currency system that the currency identifier belongs to.
The spec[1] defines it as non-nullable, and our current IDL implementation
follows it well. However, its mapping value for mojom is nullable. It looks
strange. As a result, in browser side, we are unnecessarily checking whether
the value is null or not everytime.
Therefore, This CL is fixing the value as non-nullable and remove unncessary
checks. This is initiated from Tommy's comment(tommyt@opera.com) on the code
review[2].

[1] https://w3c.github.io/browser-payment-api/#paymentcurrencyamount
[2] https://codereview.chromium.org/2646313002/#msg5

BUG=none

Review-Url: https://codereview.chromium.org/2649143003
Cr-Commit-Position: refs/heads/master@{#445725}
Committed:
https://chromium.googlesource.com/chromium/src/+/427645407dc20735347e185b9875...
==========

[commit-bot: I haz the power, 2017-01-24 14:43:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/427645407dc20735347e185b9875...