1432079 - Implement support for PaymentItem type member

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement, P1)

Description

[Marcos Caceres [:marcosc]]

The `PaymentItemType.type` member, together with the `PaymentItemType` enum, allows developers to explicitly identify a `PaymentItem` of being of a particular type (e.g., tax).

Comment 1

[Marcos Caceres [:marcosc]]

Sorry @overholt, could you help me get this triaged/prioritized?

Comment 2

[Andrew Overholt [:overholt]]

Does this require the accompanying frontend work to be complete first? (Just trying to understand urgency)

Comment 3

[Marcos Caceres [:marcosc]]

(In reply to Andrew Overholt [:overholt] from comment #2)
> Does this require the accompanying frontend work to be complete first? (Just
> trying to understand urgency)

The other way around, I’m afraid (DOM implemention needed first, so we can show the type in the Payment Sheet UI). However, it’s not super-duper high priority, but something the front end team would like to have for the first release. 

It’s a very small addition to the API. Just adds a dictionary member and an enum value.

Comment 4

[Andrew Overholt [:overholt]]

OK I'll find someone.

Comment 5

[Andrew Overholt [:overholt]]

Henri said he could help with a few Web Payments things here and there while we figure out longer-term ownership.

Comment 6

[Marcos Caceres [:marcosc]]

I'd be happy to help with implementing if someone would mentor me a little bit. I'd like to at least be able to get to the point where I can do simple fixes like this one, but without having to fight with the IDL binding layer for days.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1432079 - Implement PaymentItemType.

Review commit: https://reviewboard.mozilla.org/r/223838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223838/

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Marcos Caceres [:marcosc] from comment #6)
> I'd be happy to help with implementing if someone would mentor me a little
> bit. I'd like to at least be able to get to the point where I can do simple
> fixes like this one, but without having to fight with the IDL binding layer
> for days.

I'm unfamiliar with this code, too. I just cargo culted my way though by imitating the implementation of PaymentShippingType.

Patch still untested.

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 8954713 [details]
Bug 1432079 - Implement PaymentItemType.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223838/diff/1-2/

Comment 10

[Henri Sivonen (:hsivonen)]

In ConvertItem(), aItem.mType.WasPassed() is always false even when the test case passes "tax". Why might that be?

Comment 11

[Andrea Marchesini [:baku]]

It seems that the test is touching PaymentCurrencyAmount instead of PaymentItem.
currency is not an attribute of PaymentItem, but it's an attribute of PaymentCurrencyAmount.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Andrea Marchesini [:baku] from comment #11)
> It seems that the test is touching PaymentCurrencyAmount instead of
> PaymentItem.

Oops. Thanks.

Comment 13

[Henri Sivonen (:hsivonen)]

Comment on attachment 8954713 [details]
Bug 1432079 - Implement PaymentItemType.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223838/diff/2-3/

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8954713 [details]
Bug 1432079 - Implement PaymentItemType.

https://reviewboard.mozilla.org/r/223838/#review230050

Comment 15

[Marcos Caceres [:marcosc]]

I think we've made a mistake with how this was specified. tl;dr: type should just be a DOMString, not an enum. 

We should then just have a registry of "well known" types in a wiki somewhere. The rationale for the change is for backwards compatibility, introp, and greater experimentation: The problem is that if we add a new type in the future, and another browser (or an older version of firefox) doesn't support it, then the unknown value will throw. At best, developers will need to feature detect for support of a new type - which seems tedious.   

If necessary, we can put restrictions on the format (e.g., only lower-case ascii)... but I don't fell particularly strongly about that.

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to Marcos Caceres [:marcosc] from comment #15)
> I think we've made a mistake with how this was specified. tl;dr: type should
> just be a DOMString, not an enum. 

Should I not land this while that's being sorted out on the spec side?

Comment 17

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #16)
> (In reply to Marcos Caceres [:marcosc] from comment #15)
> > I think we've made a mistake with how this was specified. tl;dr: type should
> > just be a DOMString, not an enum. 
> 
> Should I not land this while that's being sorted out on the spec side?

We can land changes. We told the working group that we are experimenting with this feature to work out the best solution, so we are free come up with a suitable solution. 

However, if others agree that the enum is not the right solution, then we should just change to using the DOMString now. I can update the spec simultaneously and provide replacement web platform tests.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6804cac955f4
Implement PaymentItemType. r=baku

Comment 19

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/6804cac955f4