[Payment Request] Add interface stubs
Created attachment 318530 [details] Patch
Comment on attachment 318530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318530&action=review > Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28 > + EnabledBySetting=PaymentRequest, I never used EnabledBySetting. What is its purpose? Would a runtime flag be more or less suited? > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:44 > + : ActiveDOMObject { &document } I haven't seen that syntax this way before. Are we not usually doing ActiveDOMObject()? > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55 > + promise->reject(); Maybe use NotSupportedError here and elsewhere. > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:63 > +void PaymentRequest::canMakePayment(Ref<DeferredPromise>&& promise) You might want to look at DOMPromiseDeferred. You could replace Ref<DeferredPromise>&& by something like DOMPromiseDeferred<IDLBoolean>. If it is too long an alias can be used also. This is a way of typing what is resolved which usually helps the compiler figure out what to do with the resolve parameter. Usually useful with ref counted types. There is also the possibility to use DOMPromiseDeferred::settle which would take something like ExceptionOr<bool>&& in that specific case. > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:37 > + I am not sure we need all these includes. wtf/Forward, WTFstring and maybe others are probably defined in EventTarget and or ActiveDOMOjbect. > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:58 > + const RefPtr<PaymentAddress>& shippingAddress() const { return m_shippingAddress; } Why not PaymentAddress* without a const? > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:60 > + const std::optional<PaymentShippingType> shippingType() const { return m_shippingType; } Why const? I guess there is no default shipping type, hence optional > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:71 > + void stop() final { } I guess the plan is to implement these in the future. > Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:35 > +void PaymentResponse::complete(std::optional<PaymentComplete>, Ref<DeferredPromise>&& promise) optional&& > Source/WebCore/Modules/paymentrequest/PaymentResponse.h:38 > +#include <wtf/text/WTFString.h> Ditto here for the includes.
(In reply to youenn fablet from comment #2) > Comment on attachment 318530 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318530&action=review > > > Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28 > > + EnabledBySetting=PaymentRequest, > > I never used EnabledBySetting. What is its purpose? > Would a runtime flag be more or less suited? It works like EnabledAtRuntime, but bases its decision on Settings rather than RuntimeEnabledFeatures. A runtime-enabled feature could work here too, and there are cases where only a runtime-enable feature would work, but this isn't one of those cases, so using a setting seemed simplest. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:44 > > + : ActiveDOMObject { &document } > > I haven't seen that syntax this way before. > Are we not usually doing ActiveDOMObject()? I've noticed that a lot of new code is using universal initialization for initializer lists. I don't think we have a style guideline about this, but I prefer it. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55 > > + promise->reject(); > > Maybe use NotSupportedError here and elsewhere. Will do. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:63 > > +void PaymentRequest::canMakePayment(Ref<DeferredPromise>&& promise) > > You might want to look at DOMPromiseDeferred. > You could replace Ref<DeferredPromise>&& by something like > DOMPromiseDeferred<IDLBoolean>. > If it is too long an alias can be used also. > This is a way of typing what is resolved which usually helps the compiler > figure out what to do with the resolve parameter. > Usually useful with ref counted types. > There is also the possibility to use DOMPromiseDeferred::settle which would > take something like ExceptionOr<bool>&& in that specific case. Ok, I'll try this! > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:37 > > + > > I am not sure we need all these includes. > wtf/Forward, WTFstring and maybe others are probably defined in EventTarget > and or ActiveDOMOjbect. Will remove what I can. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:58 > > + const RefPtr<PaymentAddress>& shippingAddress() const { return m_shippingAddress; } > > Why not PaymentAddress* without a const? That should probably work. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:60 > > + const std::optional<PaymentShippingType> shippingType() const { return m_shippingType; } > > Why const? Oops, typo. > I guess there is no default shipping type, hence optional Right. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.h:71 > > + void stop() final { } > > I guess the plan is to implement these in the future. Right. > > > Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:35 > > +void PaymentResponse::complete(std::optional<PaymentComplete>, Ref<DeferredPromise>&& promise) > > optional&& Ok. > > > Source/WebCore/Modules/paymentrequest/PaymentResponse.h:38 > > +#include <wtf/text/WTFString.h> > > Ditto here for the includes. Ok. Thanks for the review!
Comment on attachment 318530 [details] Patch Attachment 318530 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4338783 New failing tests: imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/allowpaymentrequest-attribute-cross-origin-bc-containers.https.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/removing-allowpaymentrequest.https.sub.html imported/w3c/web-platform-tests/payment-request/payment-request-abort-method.https.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest-timing.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/no-attribute-cross-origin-bc-containers.https.html
Created attachment 318546 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 318530 [details] Patch Attachment 318530 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4338727 New failing tests: imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/allowpaymentrequest-attribute-cross-origin-bc-containers.https.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/removing-allowpaymentrequest.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest-timing.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/no-attribute-cross-origin-bc-containers.https.html imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https.html
Created attachment 318547 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Andy Estes from comment #3) > (In reply to youenn fablet from comment #2) > > Comment on attachment 318530 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=318530&action=review > > > > > Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28 > > > + EnabledBySetting=PaymentRequest, > > > > I never used EnabledBySetting. What is its purpose? > > Would a runtime flag be more or less suited? > > It works like EnabledAtRuntime, but bases its decision on Settings rather > than RuntimeEnabledFeatures. A runtime-enabled feature could work here too, > and there are cases where only a runtime-enable feature would work, but this > isn't one of those cases, so using a setting seemed simplest. > Where possible, EnabledBySetting should be used, and little by little we should try and remove use of RuntimeEnabledFeatures. There isn't a compelling reason to have two settings concepts in WebCore (Settings and RuntimeEnabledFeatures).
(In reply to Sam Weinig from comment #8) > (In reply to Andy Estes from comment #3) > > (In reply to youenn fablet from comment #2) > > > Comment on attachment 318530 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=318530&action=review > > > > > > > Source/WebCore/Modules/paymentrequest/PaymentAddress.idl:28 > > > > + EnabledBySetting=PaymentRequest, > > > > > > I never used EnabledBySetting. What is its purpose? > > > Would a runtime flag be more or less suited? > > > > It works like EnabledAtRuntime, but bases its decision on Settings rather > > than RuntimeEnabledFeatures. A runtime-enabled feature could work here too, > > and there are cases where only a runtime-enable feature would work, but this > > isn't one of those cases, so using a setting seemed simplest. > > > > Where possible, EnabledBySetting should be used, and little by little we > should try and remove use of RuntimeEnabledFeatures. There isn't a > compelling reason to have two settings concepts in WebCore (Settings and > RuntimeEnabledFeatures). I agree there's not a compelling reason to have both and we'd ideally only have one. That said, as long as the Settings objects is attached to a Page, we won't be able to move to it completely. (Not relevant to this patch, just logging a distinct difference between the two for future code historians needing to know.
Comment on attachment 318530 [details] Patch Attachment 318530 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4339287 New failing tests: imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/allowpaymentrequest-attribute-cross-origin-bc-containers.https.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/removing-allowpaymentrequest.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/setting-allowpaymentrequest-timing.https.sub.html imported/w3c/web-platform-tests/payment-request/allowpaymentrequest/no-attribute-cross-origin-bc-containers.https.html imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https.html
Created attachment 318559 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 318563 [details] Patch
Created attachment 318568 [details] Patch
Comment on attachment 318568 [details] Patch Attachment 318568 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4340188 New failing tests: imported/w3c/web-platform-tests/payment-request/payment-request-abort-method.https.html
Created attachment 318573 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 318568 [details] Patch Attachment 318568 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4340433 New failing tests: imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https.html
Created attachment 318577 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 318588 [details] Patch
Created attachment 318589 [details] Patch
Comment on attachment 318589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318589&action=review > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55 > + RunLoop::main().dispatch([promise = WTFMove(promise)]() mutable { You do not need this, promise rejection callback will be done asynchronously even if you call reject synchronously.
Created attachment 318590 [details] Patch
(In reply to youenn fablet from comment #20) > Comment on attachment 318589 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318589&action=review > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:55 > > + RunLoop::main().dispatch([promise = WTFMove(promise)]() mutable { > > You do not need this, promise rejection callback will be done asynchronously > even if you call reject synchronously. Maybe I'm missing something, but I don't think this is right. When I trace the call to reject(), I see JSC::Interpreter::executeCall() called immediately. I don't see where the asynchrony happens. Anyway, without this, two of the payment-request tests become flaky, so I'm going to keep it for now. This code is temporary, anyway.
(In reply to Andy Estes from comment #22) > Maybe I'm missing something Oh does the Promise code in builtins that handle the asynchrony?
(In reply to Andy Estes from comment #23) > (In reply to Andy Estes from comment #22) > > Maybe I'm missing something > > Oh does the Promise code in builtins that handle the asynchrony? Ok, I see how this works now. I missed the builtin code that enqueues a microtask. I agree I shouldn't need the RunLoop dispatching. I'll remove the RunLoop code and skip the flaky tests instead. These tests will likely stop being flaky once show() and abort() have real implementations.
Created attachment 318592 [details] Patch
Comment on attachment 318592 [details] Patch Clearing flags on attachment: 318592 Committed r220955: <http://trac.webkit.org/changeset/220955>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33981728>