Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1880353004:
PaymentRequest should be a ContextLifecycleObserver (Closed)
DescriptionPaymentRequest should be a ContextLifecycleObserver
This CL makes PaymentRequest a ContextLifecycleObserver.
Since mojo::Binding::Close() has a precondition that it
is bound, check that in PaymentRequest::cleanUp before
calling Close(), if not Payment unit tests that create
PaymentRequests that do not bind will crash.
BUG=602660
Committed: https://crrev.com/5b76c04ae3040e16d924495416b722b2fcf13616
Cr-Commit-Position: refs/heads/master@{#387214}
Patch Set 1 #Patch Set 2 : V2 #
Total comments: 6
Patch Set 3 : Address review comments #
Messages
Total messages: 19 (8 generated)
PaymentRequest should be a ContextLifecycleObserver BUG=587995
Description was changed from ========== lifecycle WIP BUG= ========== to ========== PaymentRequest should be a ContextLifecycleObserver BUG= ==========
Description was changed from ========== PaymentRequest should be a ContextLifecycleObserver BUG= ========== to ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. It also changes mojo::Binding::Close() to only close the message pipe if it has been bound in the first place, if not the Payment webkit unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ==========
rob.buis@samsung.com changed reviewers: + rouslan@chromium.org
PTAL.
Description was changed from ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. It also changes mojo::Binding::Close() to only close the message pipe if it has been bound in the first place, if not the Payment webkit unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ========== to ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. It also changes mojo::Binding::Close() to only close the message pipe if it has been bound in the first place, if not Payment unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ==========
https://codereview.chromium.org/1880353004/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/binding.h (right): https://codereview.chromium.org/1880353004/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/binding.h:159: void Close() { if (internal_state_.is_bound()) internal_state_.Close(); } Instead of checking is_bound() here, let's check it in PaymentRequest::cleanUp(). https://codereview.chromium.org/1880353004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/1880353004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:290: ScriptPromise PaymentRequest::complete(ScriptState* scriptState, bool success) Let's keep the order of methods in PaymentRequest.cpp and PaymentRequest.h the same. If you did not move "complete()" method in PaymentRequest.h, please do not move the "complete()" method in PaymentRequest.cpp. https://codereview.chromium.org/1880353004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:369: m_clientBinding.Close(); I recommend checking Binding::is_bound() in here instead of inside of Close(). This line should change to: if (m_clientBinding.is_bound()) m_clientBinding.Close();
https://codereview.chromium.org/1880353004/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/binding.h (right): https://codereview.chromium.org/1880353004/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/binding.h:159: void Close() { if (internal_state_.is_bound()) internal_state_.Close(); } On 2016/04/13 22:56:46, Rouslan wrote: > Instead of checking is_bound() here, let's check it in > PaymentRequest::cleanUp(). Done. https://codereview.chromium.org/1880353004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/1880353004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:290: ScriptPromise PaymentRequest::complete(ScriptState* scriptState, bool success) On 2016/04/13 22:56:46, Rouslan wrote: > Let's keep the order of methods in PaymentRequest.cpp and PaymentRequest.h the > same. If you did not move "complete()" method in PaymentRequest.h, please do not > move the "complete()" method in PaymentRequest.cpp. Done. https://codereview.chromium.org/1880353004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:369: m_clientBinding.Close(); On 2016/04/13 22:56:46, Rouslan wrote: > I recommend checking Binding::is_bound() in here instead of inside of Close(). > This line should change to: > > if (m_clientBinding.is_bound()) > m_clientBinding.Close(); Done.
lgtm
> It > also changes mojo::Binding::Close() to only close the > message pipe if it has been bound in the first place. Please update this sentence in the description.
Description was changed from ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. It also changes mojo::Binding::Close() to only close the message pipe if it has been bound in the first place, if not Payment unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ========== to ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. Since mojo::Binding::Close() has a precondition that it is bound, check that in PaymentRequest::cleanUp before calling Close(), if not Payment unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ==========
On 2016/04/14 00:34:53, Rouslan wrote: > > It > > also changes mojo::Binding::Close() to only close the > > message pipe if it has been bound in the first place. > > Please update this sentence in the description. Good point! Done.
LGTM, nice fix.
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880353004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880353004/40001
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. Since mojo::Binding::Close() has a precondition that it is bound, check that in PaymentRequest::cleanUp before calling Close(), if not Payment unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ========== to ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. Since mojo::Binding::Close() has a precondition that it is bound, check that in PaymentRequest::cleanUp before calling Close(), if not Payment unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. Since mojo::Binding::Close() has a precondition that it is bound, check that in PaymentRequest::cleanUp before calling Close(), if not Payment unit tests that create PaymentRequests that do not bind will crash. BUG=602660 ========== to ========== PaymentRequest should be a ContextLifecycleObserver This CL makes PaymentRequest a ContextLifecycleObserver. Since mojo::Binding::Close() has a precondition that it is bound, check that in PaymentRequest::cleanUp before calling Close(), if not Payment unit tests that create PaymentRequests that do not bind will crash. BUG=602660 Committed: https://crrev.com/5b76c04ae3040e16d924495416b722b2fcf13616 Cr-Commit-Position: refs/heads/master@{#387214} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5b76c04ae3040e16d924495416b722b2fcf13616 Cr-Commit-Position: refs/heads/master@{#387214} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
