Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace setManifest()/getManifest() with set()/get()/keys()/has()/delete() #95

Closed
adamroach opened this issue Jan 23, 2017 · 24 comments
Closed

Comments

@adamroach
Copy link
Contributor

When a payment app is being set up in the current spec, it does so by (a) registering as a service worker, and then (b) installing a monolithic object via PaymentAppManager.setManifest(). Subsequent updates are performed through calling PaymentAppManager.getManifest(), mutating the object, and setting it again.

@marcoscaceres has proposed that this be replaced with a decomposed interface that allows for payment apps to modify their characteristics on a more granular level using get/set/delete/etc. operations.

As a slight modification of the proposal he made in his payment-request-handler document, I propose concretely:

  1. Removing the setManifest() and setManifest() methods from PaymentAppManager.
  2. Adding new setName() and setIcon() methods to PaymentAppManager.
  3. Adding a new options attribute to PaymentAppManager, of type PaymentMethodOption, having the following definition (taken from Marcos' proposal):
[Exposed=(Window,Worker)]
interface PaymentMethodOptions {
    [NewObject]
    Promise<boolean>              delete(DOMString methodKey);
    [NewObject]
    Promise<PaymentMethodDetails> get(DOMString methodKey);
    [NewObject]
    Promise<sequence<DOMString>>  keys();
    [NewObject]
    Promise<boolean>              has(DOMString methodKey);
    [NewObject]
    Promise<void>                 set(DOMString methodKey,
                                                      PaymentMethodDetails details);
};
@tommythorsen
Copy link
Member

I think this looks like a good change. +1 from me

@marcoscaceres
Copy link
Member

Adding new setName() and setIcon() methods to PaymentAppManager.

I still don't understand the use case for these? Can you clarify?

@jakearchibald
Copy link

@marcoscaceres for the payment app to be represented to the user, I believe. You have the name & icons per method, the current spec has one name & one set of icons for the app.

@marcoscaceres
Copy link
Member

Isn't that just duplicating HTML (link rel=icon and meta application-name) and web manifest? Why do we need YA another way of doing this? (That's a question for the group, not Jake).

@marcoscaceres
Copy link
Member

Also, we need multiple icons for per payment method: those actually matter, but should still be optional.

The app should get it's icon, name, etc. like any other PWA.

@ianbjacobs
Copy link
Contributor

@marcoscaceres wrote:

"Isn't that just duplicating HTML (link rel=icon and meta application-name) and web manifest? "

That's the issue that triggered all this. :) We would like to use Web Manifest. Web Manifest is partly implemented and there are statements of intention to implement. We have been wondering how to manage a transition period where we want people to be able to create payment apps (and use icons, etc.) even before Web Manifest is widely deployed. I'd like to figure out how to do that as cleanly as possible.

Ian

@marcoscaceres
Copy link
Member

marcoscaceres commented Jan 24, 2017 via email

@ianbjacobs
Copy link
Contributor

@marcoscaceres wrote:

"Again, you don't need to "manage" that. All the browser vendors know about it and each browser vendor provides a fallback. ... Cleanest solution is to do nothing. Focus just on solving responding to payment requests."

Will you go bungee jumping with me?

Ian

@marcoscaceres
Copy link
Member

marcoscaceres commented Jan 24, 2017 via email

@adamroach
Copy link
Contributor Author

adamroach commented Jan 24, 2017

@marcoscaceres --
I think you're missing an important part of the overall flow here.

Imagine:

Now, I go to a merchant site and click the "Pay" button. I, as a user, need to decide, at that moment, which of these four payment apps I want to use to send a basic card number to the merchant.

Without a registered icon and name, all you have is the URL these payment apps came from. So you can either show the user:

  1. www.bankofamerica.com
  2. www.citibank.com
  3. www.paypal.com
  4. www.paypal.com

(Leaving the user no way to distinguish between the two different paypal apps), or you can show the user:

  1. https://www.bankofamerica.com/payments/v1/cards.js
  2. https://www.citibank.com/assets/cc-payment-app.js
  3. https://www.paypal.org/eboxapps/js/0d/d056209362e0d90abada2b39c6846b5ddc8953.js
  4. https://www.paypal.org/eboxapps/js/68/68102ba47925b528684486c62f26ca750df08ec1.js

(at which point they close their browser, hop in the car, and drive to the store, on the basis that the web has simply become too confusing for them to use any longer.)

The reason we need these names (and, ideally, icons) is to allow this app selection process to be rendered to the user in some sensible fashion. If you think this is a solved problem, please be explicit in describing precisely where the browser will get these names and icons from, if not provided by the payment app itself during its registration.

@madmath
Copy link

madmath commented Jan 24, 2017

Agree on icons and names.

To come back to the question of fine-grained control of payment methods through get()/set()/delete()/etc. I worry that we are introducing a vector for bugs. The Payment App is now responsible for deleting stale payment methods from the list, whereas before it could just say "here's all the payment methods I currently support for this user," which is more consistent with how a server views the world.

Under the proposed change the payment app can of course iterate over all currently set keys, mutating them or deleting them as needed. But I worry that if they don't, or if there is a bug in their implementation, we are left with stale payment methods that the browser has to show, which leads to bad UX.

I will defer to you all on the name, but I am effectively advocating for an API to set all supported payment methods at once, overwriting the previous state, and suggesting it should be the only way to control payment methods for an App.

@adrianhopebailie
Copy link
Contributor

@marcoscaceres I am confused about what you are saying wrt manifests. On one hand you are asking why we are obsessed with manifests and on the other saying, just use web app manifest.

The reason we can't use Web manifest as it stands today is that it explicitly excludes a mechanism for triggering installation of the web app from script. An app developer has to advertise the app through link relations and then hope the browser feels kind on the day and prompts the user to install it on their homescreen.

@marcoscaceres
Copy link
Member

@adamroach, clearly, I'm failing at using English - let me try some code and pictures instead.

A web application need only include:

<!-- if the browser supports web manifest -->
<link rel=manifest href="manifest.json">

<!-- worst case, browser supports link rel icon and application-name -->
<link rel=icon href="some-icons.ico" sizes="16x16 32x32 64x64 128x128 256x256">
<meta name="application-name" content="PayPal">

<!-- or proprietary icons --> 
<link rel="apple-touch-icon-precomposed" sizes="144x144" href="icons">

Look at PayPal - they already have all this information:

screenshot_2017-01-25_10_51_35

Also, the "multi-paypal" example is totally bogus. If there are two totally different things, then they would be in totally different origins (hence different apps). In such a case, you would just get different icons:

screenshot 2017-01-25 11 15 23

@madmath, wrote "To come back to the question of fine-grained control of payment methods through get()/set()/delete()/etc. I worry that we are introducing a vector for bugs. The Payment App is now responsible for deleting stale payment methods from the list, whereas before it could just say "here's all the payment methods I currently support for this user," which is more consistent with how a server views the world.

This is not the server. That would be like saying, "here are the files for offline"... and that is exactly what AppCache did. And that broke everything. It's exactly the same here and we should not repeat those mistakes.

Hence the fine grained proposal for control over managing payment methods. Additionally, developers should be able to add, remove, etc. whatever payment methods they want in coordination with the end-user.

But I worry that if they don't, or if there is a bug in their implementation, we are left with stale payment methods that the browser has to show, which leads to bad UX.

That's not our problem. You can't stop developers doing bad things. The Cache API doesn't automatically hit the network if it can't find the Response it's looking for. Same here. We expect developers to do the right thing.

Additionally, the API, given it's asynchronous nature, allows implementers to keep relevant UIs in sync with the web application. So even if the user decides to remove a payment method from another tab, then we can update our own UI.

I will defer to you all on the name, but I am effectively advocating for an API to set all supported payment methods at once, overwriting the previous state, and suggesting it should be the only way to control payment methods for an App.

I'm strongly against this position. Developers should be able to add/remove/modify payment methods whenever they want (so long as the user has given them permission to do so) - without affecting other payment methods.

@marcoscaceres
Copy link
Member

marcoscaceres commented Jan 25, 2017

@adrianhopebailie, wrote:

@marcoscaceres I am confused about what you are saying wrt manifests. On one hand you are asking why we are obsessed with manifests and on the other saying, just use web app manifest.

I'm not saying that... I'm saying, the web platform provides everything you need already. You don't need to worry about icons and names when it comes to web applications. It's a solved problem (more below).

The reason we can't use Web manifest as it stands today is that it explicitly excludes a mechanism for triggering installation of the web app from script.

I don't understand what this has to do with anything. This again comes back to the whole "app" thing - which this proves is causing confusion - and why "app" should be dropped from our the nomenclature immediately.

Firstly, we need to stop pretending these things are "apps". This is a feature, just another API. There is nothing special about this API. It's just another API amongst other APIs: how browser vendors choose to present the API, based on the available metadata (link rel icon, web manifest, meta application-name), is up to us.

An app developer has to advertise the app through link relations and then hope the browser feels kind on the day and prompts the user to install it on their homescreen.

No. Absolutely not! you seem to be confused, or hold incorrect assumptions, about how web manifests work. Let me clarify: it's just metadata, it's not linked to PWA installation! A browser can use a web manifest wherever it wants, for whatever it wants.

Thus, when navigator.serviceWorker.paymentManager.register() is called, the web manifest can get fetched (if not already fetched!) used. Otherwise, if the manifest is missing, the metadata in HTML I listed above gets used.

Hope that makes more sense.

@adamroach
Copy link
Contributor Author

@marcoscaceres --

Also, the "multi-paypal" example is totally bogus. If there are two totally different things, then they would be in totally different origins (hence different apps).

I'm so very confused. Just 19 hours ago, you said that we could have more than one per origin. Now you're saying that doing so is "totally bogus." My head is spinning. Which one do you actually mean?

I have some other comments and questions, but I think we need to be talking about the same thing before we can come to any reasonable conclusions. It's clear that's not yet the case.

@marcoscaceres
Copy link
Member

It's what I said there: "I was not proposing 1 [service worker] per origin. What I was trying to say is, you can register as many service workers as you like, but the "feature" is controlled per origin (like any other powerful feature)."

Again, in pictures, I want this on "bank.com":

screenshot 2017-01-25 15 15 36

And I want this on "merchant.com" when I go to pay:

screenshot 2017-01-25 11 15 23

And this is what is "totally bogus" (apologies if I misread and you were not proposing the below):

const reg1 = await serviceWorker.register("app1.sw");
const reg2 = await serviceWorker.register("app2.sw");

reg1.paymentManager.setName("SUPER COOL CARD MANAGER");
reg1.paymentManager.setIcon("super-app1.png");

reg2.paymentManager.setName("I'M A TOTALLY DIFFERENT CARD MANAGER");
reg2.paymentManager.setIcon("TOTALY_DIFFERENT_APP.jpeg");

await Promise.all([reg1.paymentManager.register(), reg2.paymentManager.register()];

Which is why the manifest (or HTML things) should set the name and the icon of the web application.

Also, calling reg1's .register() should grant permission to reg2, because ".register()" is a permission grant. I think you agree with that, no?

@marcoscaceres
Copy link
Member

marcoscaceres commented Jan 25, 2017

Again, because my english is no good - the following "no es bueno"*:

screenshot_2017-01-25_15_43_57

No multiple "apps" per origin. Hopefully that's more clear.

*I'm Argentinian, so I'm allowed to say this 😉

@adamroach
Copy link
Contributor Author

@marcoscaceres -- yes, that's exactly what I was proposing. It is explicitly the question that is being called in Issue #98. What you're proposing here is exactly the thing that @tommythorsen calls a "very artificial limitation," which is -- I suspect -- a view that is shared by most of the working group.

I think it's now crystal clear that you think otherwise, although you don't seem to have explained why.

@marcoscaceres
Copy link
Member

marcoscaceres commented Jan 25, 2017

why...

  • Unnecessary complexity: it's a super edge case, and can be solved using a different origin.
  • Each requires its own name/icon(s): requires differentiating between different type of "apps" - adds more complexity; introduces yet more icons, names, etc. to the platform.
  • Confusing: revoking payment handling permission for "PayPal.com" disables multiple payment handlers.

@marcoscaceres
Copy link
Member

(argh, we should be having this discussion in the other bug...)

@marcoscaceres
Copy link
Member

Moved the discussion back to the other bug...

@adrianhopebailie
Copy link
Contributor

@marcoscaceres said:

Additionally, developers should be able to add, remove, etc. whatever payment methods they want in coordination with the end-user.

I think this is the crux of the confusion. Let me attempt to unpack a bit and see if we can all get on the same page...

I hear @marcoscaceres suggesting something like the following.

A user, Bob, browses to his bank's website: https://bigbank.com. That website has a <link rel='manifest'> tag in the page he visits which is picked up by the browser which follows the steps to register a web app as defined in the app manifest spec.

Part of that process is installing a Service Worker which, when it registers itself attempts to add event listeners for canMakePayment and paymentRequest events.

When it does this the browser asks Bob if he grants permission for https://bigbank.com to Handle Payment Requests (or some more user friendly string).

Bob grants permission and the Service Worker registration completes but has not registered any payment methods.

Bob log's into https://bigbank.com so he now has an authenticated session. At this point the Service Worker is able to see that Bob has two visa cards linked to his account and it attempts to add basic-card as a supported payment methods by calling some method on the new API.

Q1: Does the Bob need to consent to this or is the fact that Bob gave https://bigbank.com permission to handle payments allow it to:

  • Install as many Service Workers as it wants that have event handlers for canMakePayment and paymentRequest
  • Add and remove supported payment methods at will

Bob is also a customer of Big Bank's business banking. He logs out of his personal online banking profile and logs into his business banking profile. (or he just goes to the other site and the banks SSO system keeps him logged in, it's not really relevant, the point is there are two different apps from the same origin)

A new app manifest is linked in this new page which points to a new Service Worker with different scope to the first but still the origin https://bigbank.com. The browser installs this without prompting Bob because this origin already has the required permissions.

This new Service Worker sees that Bob has a China Union Pay business banking card on his profile and also registers the fact that it can handle the basic-card payment method.

Next Bob goes shopping online and when he checks out the website calls the PaymentRequest API with basic-card support listed in it's accepted methods.

Our goal is that Bob is prompted with 3 options here, his two personal cards and his business card however his browser only processed two manifests and so only has icons and labels for the apps not for the instruments/options.

Q2: When the Service Worker adds or removes methods does it provide an icon and label? If so how can we do this without duplicating what is already in the manifest of the app.

@ianbjacobs
Copy link
Contributor

Update: AdamR plans to write a proposal regarding finer-grain get/set functions (and to include filter capabilities in the proposal (cf. issue #96)

adamroach added a commit to adamroach/webpayments-payment-apps-api that referenced this issue Feb 20, 2017
ianbjacobs pushed a commit that referenced this issue Feb 21, 2017
* Proposed resolution to #95 and #96

* Fixing a couple of nits
@ianbjacobs
Copy link
Contributor

Closing this as it has been approved through a pull request approval
adamroach@dc913b8

ianbjacobs pushed a commit that referenced this issue Mar 18, 2017
* Proposed resolution to #95 and #96

* Fixing a couple of nits

* Conflict merge

* Changes to resolve issues #99, #109, #111
and to flesh out Instrument/Wallet APIs.
These incorporate example code from Marcos'
Payment Handler thumbnail document.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants