Web monetization prototype [chromium/src : main]
Alexander Surkov (Gerrit)
Attention is currently required from: Alexander Surkov.
Alexander Surkov uploaded patch set #8 to this change.
Web monetization prototype
This a WM skeleton with boilerplate code for WM implementation.
It consist of webmonetization blink module which is supposed to
implement a web monetization session flow and HTML link rel=monetization
extension which starts a web monetization session.
WM spec: https://webmonetization.org/specification/
Bug: 1031476
Change-Id: I6229515b594dc186cea87c9a1238e5f1614700ab
---
M third_party/blink/renderer/bindings/generated_in_modules.gni
M third_party/blink/renderer/bindings/idl_in_modules.gni
M third_party/blink/renderer/core/core_initializer.h
M third_party/blink/renderer/core/events/event_type_names.json5
M third_party/blink/renderer/core/frame/local_frame_client.h
M third_party/blink/renderer/core/frame/local_frame_client_impl.cc
M third_party/blink/renderer/core/frame/local_frame_client_impl.h
M third_party/blink/renderer/core/html/build.gni
M third_party/blink/renderer/core/html/html_link_element.cc
A third_party/blink/renderer/core/html/link_monetization.cc
A third_party/blink/renderer/core/html/link_monetization.h
M third_party/blink/renderer/core/html/link_resource.h
M third_party/blink/renderer/core/html/link_style.cc
M third_party/blink/renderer/core/html/rel_list.cc
M third_party/blink/renderer/core/loader/link_loader.cc
M third_party/blink/renderer/modules/BUILD.gn
M third_party/blink/renderer/modules/modules_initializer.cc
M third_party/blink/renderer/modules/modules_initializer.h
A third_party/blink/renderer/modules/webmonetization/BUILD.gn
A third_party/blink/renderer/modules/webmonetization/monetization_configuration.h
A third_party/blink/renderer/modules/webmonetization/monetization_currency_amount.cc
A third_party/blink/renderer/modules/webmonetization/monetization_currency_amount.h
A third_party/blink/renderer/modules/webmonetization/monetization_currency_amount.idl
A third_party/blink/renderer/modules/webmonetization/monetization_event.cc
A third_party/blink/renderer/modules/webmonetization/monetization_event.h
A third_party/blink/renderer/modules/webmonetization/monetization_event.idl
A third_party/blink/renderer/modules/webmonetization/monetization_event_init.idl
A third_party/blink/renderer/modules/webmonetization/monetization_event_test.cc
A third_party/blink/renderer/modules/webmonetization/monetization_provider.cc
A third_party/blink/renderer/modules/webmonetization/monetization_provider.h
A third_party/blink/renderer/modules/webmonetization/monetization_session.cc
A third_party/blink/renderer/modules/webmonetization/monetization_session.h
A third_party/blink/renderer/modules/webmonetization/open_payments_amount.cc
A third_party/blink/renderer/modules/webmonetization/open_payments_amount.h
A third_party/blink/renderer/modules/webmonetization/open_payments_complete_incoming_payment.h
A third_party/blink/renderer/modules/webmonetization/open_payments_fetcher.cc
A third_party/blink/renderer/modules/webmonetization/open_payments_fetcher.h
A third_party/blink/renderer/modules/webmonetization/open_payments_incoming_payment.cc
A third_party/blink/renderer/modules/webmonetization/open_payments_incoming_payment.h
A third_party/blink/renderer/modules/webmonetization/open_payments_outgoing_payment.h
A third_party/blink/renderer/modules/webmonetization/open_payments_quote.cc
A third_party/blink/renderer/modules/webmonetization/open_payments_quote.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
43 files changed, 1,253 insertions(+), 12 deletions(-)
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: Alexander Surkov.
Alexander Surkov uploaded patch set #9 to this change.
Web monetization prototype
This a WM skeleton with boilerplate code for WM implementation.
It consist of webmonetization blink module which is supposed to
implement a web monetization session flow and HTML link rel=monetization
extension which starts a web monetization session.
WM spec: https://webmonetization.org/specification/
Bug: 1031476
Change-Id: I6229515b594dc186cea87c9a1238e5f1614700ab
---
M third_party/blink/renderer/bindings/generated_in_modules.gni
M third_party/blink/renderer/bindings/idl_in_modules.gni
M third_party/blink/renderer/core/core_initializer.h
M third_party/blink/renderer/core/events/event_type_names.json5
M third_party/blink/renderer/core/frame/local_frame_client.h
M third_party/blink/renderer/core/frame/local_frame_client_impl.cc
M third_party/blink/renderer/core/frame/local_frame_client_impl.h
M third_party/blink/renderer/core/html/build.gni
M third_party/blink/renderer/core/html/html_link_element.cc
A third_party/blink/renderer/core/html/link_monetization.cc
A third_party/blink/renderer/core/html/link_monetization.h
M third_party/blink/renderer/core/html/link_resource.h
M third_party/blink/renderer/core/html/rel_list.cc
41 files changed, 1,249 insertions(+), 10 deletions(-)
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Rouslan Solomakhin (Gerrit)
Attention is currently required from: Alexander Surkov.
6 comments:
Patchset:
A few minor comments while I read the initial parts of the code. More to come later this week. 😄
File third_party/blink/renderer/modules/webmonetization/BUILD.gn:
deps = []Probably OK to omit the empty `deps`.
File third_party/blink/renderer/modules/webmonetization/monetization_currency_amount.idl:
File third_party/blink/renderer/modules/webmonetization/monetization_event.idl:
File third_party/blink/renderer/modules/webmonetization/monetization_event_init.idl:
Patch Set #9, Line 5:
// https://webmonetization-preview.netlify.app/specification/#dom-monetizationeventPlease link to https://webmonetization-preview.netlify.app/specification/#dom-monetizationeventinit instead.
required DOMString? amount;
required DOMString? assetCode;
required octet? assetScale;
required DOMString? receipt;The spec says:
The `amount`, `assetCode`, `assetScale` and `receipt` attributes are deprecated.
Is it OK to keep including these fields in the code?
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: Rouslan Solomakhin.
5 comments:
File third_party/blink/renderer/modules/webmonetization/BUILD.gn:
deps = []Probably OK to omit the empty `deps`.
Done
File third_party/blink/renderer/modules/webmonetization/monetization_currency_amount.idl:
Please link to https://webmonetization-preview.netlify. […]
Done
File third_party/blink/renderer/modules/webmonetization/monetization_event.idl:
Please link to https://webmonetization-preview.netlify. […]
Done
File third_party/blink/renderer/modules/webmonetization/monetization_event_init.idl:
Please link to https://webmonetization-preview.netlify. […]
Done
required DOMString? amount;
required DOMString? assetCode;
required octet? assetScale;
required DOMString? receipt;
The spec says: […]
Done
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: Rouslan Solomakhin.
Alexander Surkov uploaded patch set #10 to this change.
41 files changed, 1,244 insertions(+), 10 deletions(-)
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: Rouslan Solomakhin.
Alexander Surkov uploaded patch set #11 to this change.
Web monetization prototype
This a WM skeleton with boilerplate code for WM implementation.
It consist of webmonetization blink module which is supposed to
implement a web monetization session flow and HTML link rel=monetization
extension which starts a web monetization session.
WM spec: https://webmonetization.org/specification/
Bug: 1031476
Change-Id: I6229515b594dc186cea87c9a1238e5f1614700ab
---
M third_party/blink/renderer/bindings/generated_in_modules.gni
M third_party/blink/renderer/bindings/idl_in_modules.gni
M third_party/blink/renderer/core/core_initializer.h
M third_party/blink/renderer/core/events/event_type_names.json5
M third_party/blink/renderer/core/frame/local_frame_client.h
M third_party/blink/renderer/core/frame/local_frame_client_impl.cc
M third_party/blink/renderer/core/frame/local_frame_client_impl.h
M third_party/blink/renderer/core/html/build.gni
M third_party/blink/renderer/core/html/html_link_element.cc
M third_party/blink/renderer/core/html/html_link_element.h
42 files changed, 1,453 insertions(+), 10 deletions(-)
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: David Baron, Dmitry Gozman, Rouslan Solomakhin.
Alexander Surkov would like David Baron and Dmitry Gozman to review this change.
Alexander Surkov (Gerrit)
Attention is currently required from: David Baron, Dmitry Gozman, Rouslan Solomakhin.
1 comment:
Patchset:
David, Dmitriy. Could you please provide some initial feedback on the implementation of the WebMonetization module? Although it's not yet complete, I would greatly appreciate your general thoughts on the approach I've taken and whether it aligns with the right direction.
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
David Baron (Gerrit)
Attention is currently required from: Alexander Surkov, David Baron, Dmitry Gozman, Rouslan Solomakhin.
8 comments:
Patchset:
One question below that would help provide some context here, and also a few comments from skimming parts of the page. I still need to provide more feedback later.
Commit Message:
Patch Set #11, Line 7:
Web monetization prototypeIs there a chromestatus entry and intent to prototype for this?
(That would provide some context that would be helpful to give high-level feedback.)
Patch Set #11, Line 16:
Bug: 1031476This is an archived bug. It should probably have a different status if there's still active work.
File third_party/blink/renderer/core/html/html_link_element.cc:
Patch Set #11, Line 324:
printf("Link Load Event\n");Please remove this `printf`.
Patch Set #11, Line 332:
printf("Link Error Event\n");Please remove this `printf`.
File third_party/blink/renderer/modules/webmonetization/monetization_event.h:
Patch Set #11, Line 18:
public GarbageCollectedMixin {I don't think you should need to inherit from `GarbageCollectedMixin`.
File third_party/blink/renderer/modules/webmonetization/monetization_event.cc:
Patch Set #11, Line 18:
MonetizationEvent* MonetizationEvent::Create(This shouldn't ignore the string passed to it. (See other events for examples of how to deal with it; it probably needs a second constructor.)
File third_party/blink/renderer/modules/webmonetization/monetization_event.idl:
Patch Set #11, Line 5:
// https://webmonetization-preview.netlify.app/specification/#monetizationevent-interfaceSeems like this and similar links should point to the https://webmonetization.org/specification/ URL for the spec since that's the spec's own preferred URL.
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: David Baron, Dmitry Gozman, Rouslan Solomakhin.
6 comments:
Commit Message:
Patch Set #11, Line 7:
Web monetization prototype
Is there a chromestatus entry and intent to prototype for this? […]
We have a PRD https://docs.google.com/document/d/1TXXnoRNhlkVjeMX434ajUVQW3AB3k-iuznJTJbNdlis/edit?usp=sharing which is supposed to give an overview of the project. Is that something you look for?
Patch Set #11, Line 16:
Bug: 1031476This is an archived bug. It should probably have a different status if there's still active work.
the status was changed
File third_party/blink/renderer/core/html/html_link_element.cc:
Patch Set #11, Line 332:
printf("Link Error Event\n");
Please remove this `printf`.
I find them useful for debugging, so I would keep them for now if you're ok with it. I'll certainly will remove them once we get closer to the landing.
File third_party/blink/renderer/modules/webmonetization/monetization_event.h:
Patch Set #11, Line 18:
public GarbageCollectedMixin {I don't think you should need to inherit from `GarbageCollectedMixin`.
Acknowledged
File third_party/blink/renderer/modules/webmonetization/monetization_event.cc:
Patch Set #11, Line 18:
MonetizationEvent* MonetizationEvent::Create(
This shouldn't ignore the string passed to it. […]
There's a second constructor provided which takes the init object only. but ok, I will keep using the given string. I couldn't find a good example in the codebase, the closest thing was BeforeUnloadEvent (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/before_unload_event.h;l=34;drc=67d90538f11c6b232dbfd716075db52aeb34fd15) but it doesn't provide that constructor at all. Can you think of a more suitable example?
File third_party/blink/renderer/modules/webmonetization/monetization_event.idl:
Seems like this and similar links should point to the https://webmonetization. […]
Acknowledged
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: David Baron, Dmitry Gozman, Nate Chapin, Rouslan Solomakhin.
Alexander Surkov would like Nate Chapin to review this change.
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Attention is currently required from: David Baron, Dmitry Gozman, Nate Chapin, Rouslan Solomakhin.
1 comment:
Patchset:
Looping into Nate for review/feedback on OpenPaymentsFetcher implementation.
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
David Baron (Gerrit)
Attention is currently required from: Alexander Surkov, Dmitry Gozman, Nate Chapin, Rouslan Solomakhin.
13 comments:
Patchset:
I took a somewhat more detailed look this time, although still not (given what you asked for) a full code review.
Comments below -- but one theme that came up a few times is that it would probably be useful for the code to have comments pointing to the parts of the spec that it's implementing. When specs are reasonably algorithmic (as this one is, at least in parts), it often makes sense for the code to resemble the spec and have comments pointing to the parts of the spec that the code is implementing.
Commit Message:
Patch Set #11, Line 7:
Web monetization prototype
We have a PRD https://docs.google. […]
That's useful -- but if you're prototyping something you should also be creating a chromestatus entry and sending an intent to prototype -- that's a part of our process and it helps the community understand what work is happening and gives folks a chance to comment early if they want to.
File third_party/blink/renderer/core/html/link_monetization.cc:
if (LocalFrame* frame = owner_->GetDocument().GetFrame()) {
frame->Client()->StartMonetizationSession(*owner_);
}At least with the current code, I don't see any reason for this to go through `LocalFrame`. (It looks like it's modeled on `DidChangeManifest` / `DispatchDidChangeManifest`, but in that case the code in `LocalFrameClientImpl` attaches additional information to it.) Could this just call `CoreInitializer::GetInstance` directly?
(That said, maybe there's a reason you would want to go through `LocalFrame` in the future?)
Patch Set #11, Line 45:
owner_->ScheduleEvent();Does this match the spec's description of when to fire load/error events at the link element? At the very least, it's hard for me to confirm that it does -- it might be good if the code looked more like the spec's description of how things should work.
File third_party/blink/renderer/core/html/rel_list.cc:
Patch Set #11, Line 43:
tokens.insert(AtomicString("monetization"));It might be nice to find a way to do this only once rather than every time `SupportedTokensLink` is called.)
File third_party/blink/renderer/modules/webmonetization/monetization_configuration.h:
Patch Set #11, Line 14:
class MODULES_EXPORT MonetizationConfiguration final {Does this (or anything other than `MonetizationCurrencyAmount` and `MonetizationEvent`) need `MODULES_EXPORT`?
MonetizationConfiguration(MonetizationConfiguration&& o)
: userWalletPaymentPointer_(o.userWalletPaymentPointer_),
maxAmount_(o.maxAmount_),
expiresAt_(o.expiresAt_) {}Can this just use `= default` rather than defining it?
blink::KURL UserWalletPaymentPointer() const {
return userWalletPaymentPointer_;
}
OpenPaymentsAmount MaxAmount() const { return maxAmount_; }
String ExpiresAt() { return expiresAt_; }Might it be better for these to return const references rather than returning by value?
(Also, should `ExpiresAt` also be a `const` method?)
File third_party/blink/renderer/modules/webmonetization/monetization_currency_amount.h:
String currency() const { return currency_; }
String value() const { return value_; }These could likely also return const references rather than copies.
File third_party/blink/renderer/modules/webmonetization/monetization_event.cc:
Patch Set #11, Line 18:
MonetizationEvent* MonetizationEvent::Create(
There's a second constructor provided which takes the init object only. […]
I think third_party/blink/renderer/core/events/progress_event.h is a reasonably typical example. (Why do you need the `Create` method that *doesn't* take the name?)
File third_party/blink/renderer/modules/webmonetization/monetization_provider.cc:
Patch Set #11, Line 11:
const KURL& paymentPointer) {I think one of the interesting questions with this design is likely about how this handles permissions for different URLs, including any necessary requests for permission and persisting those permissions. It looks like that part isn't implemented yet.
One thing I'd note, however, is that things that require requesting permission often have to be asynchronous, to account for the time to ask the user a question and await a response. They might even need to be asynchronous just to ask the browser process for persisted data relevant to the site. This makes me a little worried about the code here all looking like it's going to be synchronous.
File third_party/blink/renderer/modules/webmonetization/monetization_session.cc:
Patch Set #11, Line 25:
const HTMLLinkElement& link_element) {It feels a little bit fragile for the API to be passing an `HTMLLinkElement` through all these layers. However, on the other hand, if there's a future mechanism that involves starting monetization through something other than a `<link>`, you could change it then. So there's probably no need to change it now, although it also might be worth thinking about if you end up adding even *more* code that passes `HTMLLinkElement`s around.
(Another thing here that could possibly be an advantage of such a change is that it would avoid exposing `LinkResource` which currently appears to be mainly internal to how we handle links, but which your change is exposing externally.)
Patch Set #11, Line 51:
base::OnceCallback<void()> sending_wallet_address =It would generally be useful to have more specific links to the spec in this sort of code that point to the parts of the spec that code like this is implementing.
To view, visit change 4930834. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Surkov (Gerrit)
Alexander Surkov uploaded new patchset![]( "Open in Gerrit")
- Alexander Surkov
- Dmitry Gozman
- Nate Chapin
- Rouslan Solomakhin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Surkov (Gerrit)
Alexander Surkov added 11 comments![]( "Open in Gerrit")
if (LocalFrame* frame = owner_->GetDocument().GetFrame()) {
frame->Client()->StartMonetizationSession(*owner_);
}At least with the current code, I don't see any reason for this to go through `LocalFrame`. (It looks like it's modeled on `DidChangeManifest` / `DispatchDidChangeManifest`, but in that case the code in `LocalFrameClientImpl` attaches additional information to it.) Could this just call `CoreInitializer::GetInstance` directly?
(That said, maybe there's a reason you would want to go through `LocalFrame` in the future?)
yep, it seems so
Does this match the spec's description of when to fire load/error events at the link element? At the very least, it's hard for me to confirm that it does -- it might be good if the code looked more like the spec's description of how things should work.
Do you mean HTML spec or WM spec? If HTML spec, then does it has generic recommendations for HTML:link load/error events that would be applicable to rel=monetization? WM specs out the load/error events here https://webmonetization-preview.netlify.app/specification/#link-type-monetization.
It might be nice to find a way to do this only once rather than every time `SupportedTokensLink` is called.)
thank you for the catch
blink::KURL UserWalletPaymentPointer() const {
return userWalletPaymentPointer_;
}
OpenPaymentsAmount MaxAmount() const { return maxAmount_; }
String ExpiresAt() { return expiresAt_; }Might it be better for these to return const references rather than returning by value?
(Also, should `ExpiresAt` also be a `const` method?)
Acknowledged
MonetizationConfiguration(MonetizationConfiguration&& o)
: userWalletPaymentPointer_(o.userWalletPaymentPointer_),
maxAmount_(o.maxAmount_),
expiresAt_(o.expiresAt_) {}Can this just use `= default` rather than defining it?
hm, it complains that only special member functions and comparison operators may be defaulted
Does this (or anything other than `MonetizationCurrencyAmount` and `MonetizationEvent`) need `MODULES_EXPORT`?
Acknowledged
String currency() const { return currency_; }
String value() const { return value_; }These could likely also return const references rather than copies.
Acknowledged
This shouldn't ignore the string passed to it. (See other events for examples of how to deal with it; it probably needs a second constructor.)
There's a second constructor provided which takes the init object only. but ok, I will keep using the given string. I couldn't find a good example in the codebase, the closest thing was BeforeUnloadEvent (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/before_unload_event.h;l=34;drc=67d90538f11c6b232dbfd716075db52aeb34fd15) but it doesn't provide that constructor at all. Can you think of a more suitable example?
I think third_party/blink/renderer/core/events/progress_event.h is a reasonably typical example. (Why do you need the `Create` method that *doesn't* take the name?)
Do you mean event type?
ProgressEvent can have multiple event type such as loadstart, abort or progress (according to MDN https://developer.mozilla.org/en-US/docs/Web/API/ProgressEvent/ProgressEvent), while MonetizationEvent can only be of 'monetization' type. I was thinking how should I hardcode the 'monetization' type into MonetizationEvent class, and came with that hacky approach. Do you have recommendations how to workaround this?
I think one of the interesting questions with this design is likely about how this handles permissions for different URLs, including any necessary requests for permission and persisting those permissions. It looks like that part isn't implemented yet.
One thing I'd note, however, is that things that require requesting permission often have to be asynchronous, to account for the time to ask the user a question and await a response. They might even need to be asynchronous just to ask the browser process for persisted data relevant to the site. This makes me a little worried about the code here all looking like it's going to be synchronous.
Correct. This is part is tbd. It will involve UI to request permissions which is a chunk of work I suspect. In this patch I kept focus on implementing a dummy part but that one giving a good understanding of the architecture of the backend part. Once this part is done I was planning to start the user faced part.
Having said that you have a good point on asynchronous nature of the monetization configuration. I might need to rethink the thing now.
It feels a little bit fragile for the API to be passing an `HTMLLinkElement` through all these layers. However, on the other hand, if there's a future mechanism that involves starting monetization through something other than a `<link>`, you could change it then. So there's probably no need to change it now, although it also might be worth thinking about if you end up adding even *more* code that passes `HTMLLinkElement`s around.
(Another thing here that could possibly be an advantage of such a change is that it would avoid exposing `LinkResource` which currently appears to be mainly internal to how we handle links, but which your change is exposing externally.)
It might be a good idea to keep a monetization session separate from a HTML:link, but the spec currently bonds these two things together, HTML:link is a target for error and monetization events sent by a monetization session. I'm not sure if I have other ideas how to improve it.
It would generally be useful to have more specific links to the spec in this sort of code that point to the parts of the spec that code like this is implementing.
I'm not sure if the flow is outlined by the spec somewhere. I was guided by an example of https://github.com/interledger/web-monetization-extension/blob/main/src/background/grantFlow.ts. I'll rise a question about that in the group. Having said that the methods like GetSendingWalletAddress have a spec link in a header file.
Related details
- David Baron
- Dmitry Gozman
- Nate Chapin
- Rouslan Solomakhin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 4 comments![]( "Open in Gerrit")
owner_->ScheduleEvent();Alexander SurkovDoes this match the spec's description of when to fire load/error events at the link element? At the very least, it's hard for me to confirm that it does -- it might be good if the code looked more like the spec's description of how things should work.
Do you mean HTML spec or WM spec? If HTML spec, then does it has generic recommendations for HTML:link load/error events that would be applicable to rel=monetization? WM specs out the load/error events here https://webmonetization-preview.netlify.app/specification/#link-type-monetization.
I think I meant the Web Monetization spec.
That said, I think https://html.spec.whatwg.org/multipage/semantics.html#fetching-and-processing-a-resource-from-a-link-element is relevant, and defining steps that integrate properly into that section would be the correct way for the Web Monetization spec to define the behavior.
MonetizationConfiguration(MonetizationConfiguration&& o)
: userWalletPaymentPointer_(o.userWalletPaymentPointer_),
maxAmount_(o.maxAmount_),
expiresAt_(o.expiresAt_) {}Alexander SurkovCan this just use `= default` rather than defining it?
hm, it complains that only special member functions and comparison operators may be defaulted
A move constructor should count as a special member function, I think. There are plenty of examples in the code now, such as https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/style/filter_operations.h;l=49;drc=a8c7ce4436274b29971e4a8a95522ab96c3ed088
MonetizationEvent* MonetizationEvent::Create(Alexander SurkovThis shouldn't ignore the string passed to it. (See other events for examples of how to deal with it; it probably needs a second constructor.)
David BaronThere's a second constructor provided which takes the init object only. but ok, I will keep using the given string. I couldn't find a good example in the codebase, the closest thing was BeforeUnloadEvent (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/before_unload_event.h;l=34;drc=67d90538f11c6b232dbfd716075db52aeb34fd15) but it doesn't provide that constructor at all. Can you think of a more suitable example?
Alexander SurkovI think third_party/blink/renderer/core/events/progress_event.h is a reasonably typical example. (Why do you need the `Create` method that *doesn't* take the name?)
Do you mean event type?
ProgressEvent can have multiple event type such as loadstart, abort or progress (according to MDN https://developer.mozilla.org/en-US/docs/Web/API/ProgressEvent/ProgressEvent), while MonetizationEvent can only be of 'monetization' type. I was thinking how should I hardcode the 'monetization' type into MonetizationEvent class, and came with that hacky approach. Do you have recommendations how to workaround this?
I think the norm is not to hard code it; see third_party/blink/renderer/core/events/overscroll_event.h as an example.
(I think Web pages can still create such an event with a different name if they want to, although I'm not 100% sure about that.)
base::OnceCallback<void()> sending_wallet_address =Alexander SurkovIt would generally be useful to have more specific links to the spec in this sort of code that point to the parts of the spec that code like this is implementing.
I'm not sure if the flow is outlined by the spec somewhere. I was guided by an example of https://github.com/interledger/web-monetization-extension/blob/main/src/background/grantFlow.ts. I'll rise a question about that in the group. Having said that the methods like GetSendingWalletAddress have a spec link in a header file.
That might be a sign that the spec needs to be more precise about how things work. On the other hand, it's often OK to be doing prototypes when the spec isn't that precise, but by the time we think about shipping a feature it probably should have a sufficiently precise spec.
Related details
- Alexander Surkov
- Dmitry Gozman
- Nate Chapin
- Rouslan Solomakhin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Surkov (Gerrit)
Alexander Surkov added 1 comment![]( "Open in Gerrit")
Is there a chromestatus entry and intent to prototype for this?
Alexander Surkov(That would provide some context that would be helpful to give high-level feedback.)
We have a PRD https://docs.google.com/document/d/1TXXnoRNhlkVjeMX434ajUVQW3AB3k-iuznJTJbNdlis/edit?usp=sharing which is supposed to give an overview of the project. Is that something you look for?
That's useful -- but if you're prototyping something you should also be creating a chromestatus entry and sending an intent to prototype -- that's a part of our process and it helps the community understand what work is happening and gives folks a chance to comment early if they want to.
here's the link on chrome status https://chromestatus.com/feature/4917148755689472, the intent to prototype is https://groups.google.com/a/chromium.org/g/blink-dev/c/4Rqw4SbjO88/m/j7x8sTyzAAAJ
Related details
- David Baron
- Dmitry Gozman
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Surkov (Gerrit)
Alexander Surkov added 2 comments![]( "Open in Gerrit")
MonetizationConfiguration(MonetizationConfiguration&& o)
: userWalletPaymentPointer_(o.userWalletPaymentPointer_),
maxAmount_(o.maxAmount_),
expiresAt_(o.expiresAt_) {}Alexander SurkovCan this just use `= default` rather than defining it?
David Baronhm, it complains that only special member functions and comparison operators may be defaulted
A move constructor should count as a special member function, I think. There are plenty of examples in the code now, such as https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/style/filter_operations.h;l=49;drc=a8c7ce4436274b29971e4a8a95522ab96c3ed088
weird, no error, seems working now
MonetizationEvent* MonetizationEvent::Create(Alexander SurkovThis shouldn't ignore the string passed to it. (See other events for examples of how to deal with it; it probably needs a second constructor.)
David BaronThere's a second constructor provided which takes the init object only. but ok, I will keep using the given string. I couldn't find a good example in the codebase, the closest thing was BeforeUnloadEvent (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/before_unload_event.h;l=34;drc=67d90538f11c6b232dbfd716075db52aeb34fd15) but it doesn't provide that constructor at all. Can you think of a more suitable example?
Alexander SurkovI think third_party/blink/renderer/core/events/progress_event.h is a reasonably typical example. (Why do you need the `Create` method that *doesn't* take the name?)
David BaronDo you mean event type?
ProgressEvent can have multiple event type such as loadstart, abort or progress (according to MDN https://developer.mozilla.org/en-US/docs/Web/API/ProgressEvent/ProgressEvent), while MonetizationEvent can only be of 'monetization' type. I was thinking how should I hardcode the 'monetization' type into MonetizationEvent class, and came with that hacky approach. Do you have recommendations how to workaround this?
I think the norm is not to hard code it; see third_party/blink/renderer/core/events/overscroll_event.h as an example.
(I think Web pages can still create such an event with a different name if they want to, although I'm not 100% sure about that.)
ok, will change that
Alexander Surkov (Gerrit)
Alexander Surkov added 1 comment![]( "Open in Gerrit")
Nate -> Takashi per Nate/Takashi recommendations
Related details
- David Baron
- Dmitry Gozman
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 6 comments![]( "Open in Gerrit")
I feel it's difficult to review many classes that are almost empty for now.
Can we review the high-level design in a design doc, and go details in the code review, prefering per-class CL with actual implementation.
Or if this is just a prototype, and want a feedback, which is the point you have a concern, or need a help?
virtual void StartMonetizationSession(const HTMLLinkElement&) = 0;why we need this here?
if (!owner_) {could this be nullptr? or can we have CHECK(owner) in the constructor?
enum LinkResourceType { kStyle, kManifest, kMonetization, kOther };maybe good to have a spec link here until it gets standardized
static HashSet<AtomicString> SupportedTokensLink() {Can you rename as GenerateSupportedTokensLink?
const KURL payerPointer{"https://ilp.rafiki.money/petro"};
const OpenPaymentsAmount amount{"1", "USD", 2};
const String expiresAt{"2024-12-25"};Where do these hardcoded values come from?
Can you document it?
Related details
- Alexander Surkov
- David Baron
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Surkov (Gerrit)
Alexander Surkov added 5 comments![]( "Open in Gerrit")
virtual void StartMonetizationSession(const HTMLLinkElement&) = 0;why we need this here?
it is used by LinkMonetization
CoreInitializer::GetInstance().StartMonetizationSession(*owner_);
if (!owner_) {could this be nullptr? or can we have CHECK(owner) in the constructor?
I copied this pattern. LinkMonetization has OwnerRemoved() which nullifies the owner. I don't see a reason why link monetization would exist with no owner though, perhaps something cc related.
enum LinkResourceType { kStyle, kManifest, kMonetization, kOther };maybe good to have a spec link here until it gets standardized
Acknowledged
static HashSet<AtomicString> SupportedTokensLink() {Can you rename as GenerateSupportedTokensLink?
Acknowledged
const KURL payerPointer{"https://ilp.rafiki.money/petro"};
const OpenPaymentsAmount amount{"1", "USD", 2};
const String expiresAt{"2024-12-25"};Where do these hardcoded values come from?
Can you document it?
It's my personal wallet used for testing purpose only to implement the minimal working version. It will be replaced on user's wallet the next phase. I'll add a comment.
Related details
- Dmitry Gozman
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
I feel it's difficult to review many classes that are almost empty for now.
Can we review the high-level design in a design doc, and go details in the code review, prefering per-class CL with actual implementation.
Or if this is just a prototype, and want a feedback, which is the point you have a concern, or need a help?
Let me repeat this.
Did you already get your design doc reviewed by a senior project member?
Or, can you prepare it? It should not be well prepared, but just I want to know the whole design pictures, e.g. a block diagram that shows class structures, and what kind of browser side services are needed, and how each process communicates.
Related details
- Alexander Surkov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer added 1 comment![]( "Open in Gerrit")
Takashi ToyoshimaI feel it's difficult to review many classes that are almost empty for now.
Can we review the high-level design in a design doc, and go details in the code review, prefering per-class CL with actual implementation.
Or if this is just a prototype, and want a feedback, which is the point you have a concern, or need a help?
Let me repeat this.
Did you already get your design doc reviewed by a senior project member?
Or, can you prepare it? It should not be well prepared, but just I want to know the whole design pictures, e.g. a block diagram that shows class structures, and what kind of browser side services are needed, and how each process communicates.
Hey folks; I think there's been some confusion on the purpose of this CL. From the Chrome Payments side, we have been providing guidance to Alex as they have been looking at Web Monetization. As part of that guidance, we suggested building a prototype CL in order to determine whether the envisaged approach was possible. It was our intent that the CL be used to learn and then discarded, being followed by a design doc. And then after the design doc was approved, 'proper' implementation CLs would follow.
I see at the start of this CL, alexs did ask David and Dimitry for:
```
David, Dmitriy. Could you please provide some initial feedback on the implementation of the WebMonetization module? Although it's not yet complete, I would greatly appreciate your general thoughts on the approach I've taken and whether it aligns with the right direction.
```
And then later it looks like Nate was brought in for feedback on OpenPaymentsFetcher:
```
Looping into Nate for review/feedback on OpenPaymentsFetcher implementation.
```
(I don't know myself if there was a specific question for that module).
Later, looks like Nate redirected Alex to Takashi for the same:
```
Nate -> Takashi per Nate/Takashi recommendations
```
But I think the context of 'this is a prototype CL just for high-level feedback' was maybe lost there?
So Alex, from my side I would ask - do you think there are still outstanding high-level issues to be figured out via iterating on this prototype, or do you feel you have enough knowledge to build a design doc now?
Related details
- Alexander Surkov
- Dmitry Gozman
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman added 1 comment![]( "Open in Gerrit")
Takashi ToyoshimaI feel it's difficult to review many classes that are almost empty for now.
Can we review the high-level design in a design doc, and go details in the code review, prefering per-class CL with actual implementation.
Or if this is just a prototype, and want a feedback, which is the point you have a concern, or need a help?
Stephen McGruerLet me repeat this.
Did you already get your design doc reviewed by a senior project member?
Or, can you prepare it? It should not be well prepared, but just I want to know the whole design pictures, e.g. a block diagram that shows class structures, and what kind of browser side services are needed, and how each process communicates.
Hey folks; I think there's been some confusion on the purpose of this CL. From the Chrome Payments side, we have been providing guidance to Alex as they have been looking at Web Monetization. As part of that guidance, we suggested building a prototype CL in order to determine whether the envisaged approach was possible. It was our intent that the CL be used to learn and then discarded, being followed by a design doc. And then after the design doc was approved, 'proper' implementation CLs would follow.
I see at the start of this CL, alexs did ask David and Dimitry for:
```
David, Dmitriy. Could you please provide some initial feedback on the implementation of the WebMonetization module? Although it's not yet complete, I would greatly appreciate your general thoughts on the approach I've taken and whether it aligns with the right direction.
```And then later it looks like Nate was brought in for feedback on OpenPaymentsFetcher:
```
Looping into Nate for review/feedback on OpenPaymentsFetcher implementation.
```(I don't know myself if there was a specific question for that module).
Later, looks like Nate redirected Alex to Takashi for the same:
```
Nate -> Takashi per Nate/Takashi recommendations
```But I think the context of 'this is a prototype CL just for high-level feedback' was maybe lost there?
So Alex, from my side I would ask - do you think there are still outstanding high-level issues to be figured out via iterating on this prototype, or do you feel you have enough knowledge to build a design doc now?
My bad, I did not understand this CL is here to have some high-level overview. The plumbing between core and modules looks fine, as well as using ResourceFetcher to load the links. That's pretty much all the high-level details I can speak of. Shall I look at something specific, or perhaps you have some questions that need to be answered?
I am not qualified to comment on the spec or whether this fits the <link> model, and it seems more appropriate to send an Intent to Prototype email to blink-dev and seek feedback from API owners and other folks writing specs.
Let me know what you think. Sorry for not being very useful here!
Related details
- Alexander Surkov
- Stephen McGruer
- Takashi Toyoshima
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Thank you for the context.
So, I guess I was invited for advice around CORS problems Alex saw in this prototype. May I ask specific code points to know what the problem is now, and what kind of advice is needed?
Related details
- Alexander Surkov
- Dmitry Gozman
- Stephen McGruer
Alexander Surkov (Gerrit)
Alexander Surkov added 1 comment![]( "Open in Gerrit")
Takashi: right, that was my original intention to get some help with CORS. However we were able to fix the CORS problem (both the CL and the server side fixes applied), so now the CL would benefit from generic networking guidance/review. Also, I need to implement GNAP support (https://oauth.net/gnap/) for OpenPayments API which involves public/private keys generation on the browser side, so if you have any input on this (such as whether any libraries/classes/code snippets in chromium are available that I could use or get inspired by), it'd be very helpful.
Stephen: so far, there are two important pieces not yet covered by the patch:
- chrome:webmonetization page for Web Monetization UI
- GNAP implementation for OpenPayments
I'm not quite sure, but probably I should figure out those in this CL before moving it into the design doc. Having said that, I think I can start adding classes into the doc for the part coded in this CL.
Also, I wasn't quite sure about the purpose of this CL. We could use it as the one to prototype the ideas and develop the architecture, then put it into the design doc, get it approved, and get the CL landed as the first stage. And then, as the second stage, I could send more CLs with the actual implementation for all moving parts (such as OpenPayments API implementation and chrome:webmonetization UI).
What do you think?
Related details
- Dmitry Gozman
- Stephen McGruer
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer added 1 comment![]( "Open in Gerrit")
Apologies for the delay in replying!
Stephen: so far, there are two important pieces not yet covered by the patch:
> chrome:webmonetization page for Web Monetization UI
GNAP implementation for OpenPayments
> I'm not quite sure, but probably I should figure out those in this CL before moving it into the design doc. Having said that, I think I can start adding classes into the doc for the part coded in this CL.
That sounds good to me; both looking at these two additional parts in this CL, as well as starting on the design doc.
For the design doc, https://docs.google.com/document/d/14YBYKgk-uSfjfwpKFlp_omgUq5hwMVazy_M965s_1KA/edit#heading=h.7nki9mck5t64 may be useful as the general Chromium template.
Also, I wasn't quite sure about the purpose of this CL. We could use it as the one to prototype the ideas and develop the architecture, then put it into the design doc, get it approved, and get the CL landed as the first stage. And then, as the second stage, I could send more CLs with the actual implementation for all moving parts (such as OpenPayments API implementation and chrome:webmonetization UI).
> What do you think?
In Chrome, we tend to bias towards tightly scoped and concrete CLs for actual landing, rather than very broad and 'skeletal' CLs that are later filled in. (This isn't absolute, just a general bias! 😊). What I tend to do for projects like this is to never land the prototype CL, but to use it as a basis from which I start pulling off small, concrete pieces to send separately as individual CLs. This makes the CLs easier to understand, and reduces the number of OWNERS needed - while still having the original end to end prototype for folks to understand the bigger picture.
Does that sound reasonable to you?
Related details
- Alexander Surkov
- Dmitry Gozman
- Takashi Toyoshima
Alexander Surkov (Gerrit)
Alexander Surkov added 1 comment![]( "Open in Gerrit")
Yep. That sounds good to me. Then I'll start sketching out the classes in the design doc and will continue to prototype the missing parts here.
Related details
- Dmitry Gozman
- Stephen McGruer
- Takashi Toyoshima
Rouslan Solomakhin (Gerrit)
Rouslan Solomakhin added 3 comments![]( "Open in Gerrit")
Hi Alexander,
Disclaimer: I am operating under the assumption that this patch is a prototype to validate the feasibility of implementing your feature. We won't be able to give a thorough review on a single patch of 4k lines of code. That's when design docs come in handy.
It think it's time to take the lessons that you learned from this prototype and try to get a design doc written up. Please use [Chrome design doc template](https://docs.google.com/document/d/14YBYKgk-uSfjfwpKFlp_omgUq5hwMVazy_M965s_1KA/edit?usp=sharing) for this purpose.
Do you have a proposed directory layout documented anywhere? See [bit.ly/cc-number-validation-target-dd](bit.ly/cc-number-validation-target-dd) for example. Although it's not the same type of project, the doc aims to make the project easier to understand with:
- A list of modified files.
- A list of added files.
- A list of new build targets.
- A diagram that summarizes the code changes.
As a rule of thumb, the more diagrams, the better. This work will especially benefit from diagrams that describe flows of data.
High-level directional review for this patch:
1. Move as much code as possible from //chrome into //components/webmonetization. Then have a few places in //chrome make use of the //components/webmonetization code.
1. Check the feature flag status in the browser code.
1. Keep the screenshots in a Google Drive, Slides, or Doc, so that it's easier to quickly review the UI. Link to these screenshots for the patch description.
Cheers,
Rouslan
<part file="webmonetization.grdp" />It's better to put strings in `//components`.
9eb8b392eec34553a507c45e0d48328a28a0aa14It may be helpful to keep the screenshots in a Google Drive, Slides, or Doc, so that it's easier to quickly review the UI.
Related details
- Alexander Surkov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |