Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
769273004:
Push API: Merge the FromDocument and FromWorker registration methods (Closed)
DescriptionPush API: Merge the FromDocument and FromWorker registration methods
Patch https://crrev.com/763833004 forked the register methods into very
similar FromDocument and FromWorker methods. This patch removes that
code duplication, which is currently a small win, but will become a
bigger win over time as my next few patches introduce additional thread
hops into the registration flow (to read and write registration IDs
to/from storage).
Depends on https://codereview.chromium.org/770423003
BUG=none
Committed: https://crrev.com/3068082ae5f0063bd998517ad820d0bf53fb73fe
Cr-Commit-Position: refs/heads/master@{#307018}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address review comments #
Total comments: 8
Patch Set 3 : Address round 2 review comments #
Total comments: 1
Patch Set 4 : Add RegisterData constructor to init scalars #
Total comments: 4
Patch Set 5 : from_document -> FromDocument #
Messages
Total messages: 17 (3 generated)
johnme@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:242: RegisterData* data, PushRegistrationStatus status) { Looks like this does not need to be a raw pointer. https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:254: RegisterData* data, const std::string& push_registration_id) { Looks like this does not need to be a raw pointer. https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:34: RegisterData(); I think you can omit this constructor.
peter@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:89: scoped_ptr<RegisterData> data(new RegisterData); Why not just allocate this on stack and store it by value in base::Bind()? There's almost no data in it, I see little value on storing this on heap. That allows you to avoid pointers elsewhere and just use |const RegisterData&|. https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:198: service()->RegisterFromWorker( FWIW, it seems more valuable to me to merge PushMessagingService::Register() than the methods in the PushMessagingMessageFilter. In either case, I would punt this since it's a lower priority clean-up, unless you need it for something else of course. https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:31: bool user_gesture; Nit: Mounir was arguing the other day that we should just stop passing |user_gesture| around. It'd be cool if this would end up matching with the struct I pass around IPC in the following CL: https://codereview.chromium.org/770023002/
Addressed review comments https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:89: scoped_ptr<RegisterData> data(new RegisterData); On 2014/12/03 12:26:11, Peter Beverloo wrote: > Why not just allocate this on stack and store it by value in base::Bind()? > There's almost no data in it, I see little value on storing this on heap. That > allows you to avoid pointers elsewhere and just use |const RegisterData&|. Done (this costs a couple of extra string copies of the origin, but it does make the code noticeably simpler). https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:198: service()->RegisterFromWorker( On 2014/12/03 12:26:11, Peter Beverloo wrote: > FWIW, it seems more valuable to me to merge PushMessagingService::Register() > than the methods in the PushMessagingMessageFilter. In either case, I would punt > this since it's a lower priority clean-up, unless you need it for something else > of course. I'm about to land 3-4 more hops for the register flow (for storing/getting data from SW storage on IO thread). I don't want to have to do that twice :) https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:242: RegisterData* data, PushRegistrationStatus status) { On 2014/12/03 12:18:59, Michael van Ouwerkerk wrote: > Looks like this does not need to be a raw pointer. Done (it's now a const RegisterData& since I've stopped using scoped_ptrs). https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:254: RegisterData* data, const std::string& push_registration_id) { On 2014/12/03 12:18:59, Michael van Ouwerkerk wrote: > Looks like this does not need to be a raw pointer. Done (it's now a const RegisterData& since I've stopped using scoped_ptrs). https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:31: bool user_gesture; On 2014/12/03 12:26:11, Peter Beverloo wrote: > Nit: Mounir was arguing the other day that we should just stop passing > |user_gesture| around. It'd be cool if this would end up matching with the > struct I pass around IPC in the following CL: > > https://codereview.chromium.org/770023002/ Sure, if that lands first I'll update this as part of fixing the merge conflict. https://codereview.chromium.org/769273004/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:34: RegisterData(); On 2014/12/03 12:18:59, Michael van Ouwerkerk wrote: > I think you can omit this constructor. Nope, Chromium style checker complains that a struct with "non-trivial" members (the scoped_ptr is what triggers it, because it's a templated type) must have a constructor and destructor :-(
https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:45: PushMessagingMessageFilter::RegisterData::~RegisterData() { micro nit: {} can be on the same line https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:107: scoped_ptr<DocumentRegisterData>(new DocumentRegisterData); nit: make_scoped_ptr() https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:114: this, data, sender_id)); Hmm. We're now ending up in the following situation: (1) We allocate a new DocumentRegisterData object on line 106. (2) We invoke the copy constructor of RegisterData on line 114 (pass by value to base::Bind). (3) The copy constructor allocates a new instance of DocumentRegisterData and invokes the compiler-generated copy ctor of that. (4) The allocation made in (1) goes out of scope, and gets freed. Can we find a way to put the DocumentRegisterData in RegisterData to avoid the custom DRD copy constructor, as well as the on-heap/on-stack mixture? Perhaps one option would be to have a helper function that validates that render_frame_id in fact is a valid render frame, rather than checking whether |document_data| is assigned some memory? https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:206: weak_factory_ui_to_ui_.GetWeakPtr(), data)); nit: maybe |weak_factory_ui_|?
Addressed 2nd round review comments. https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:45: PushMessagingMessageFilter::RegisterData::~RegisterData() { On 2014/12/04 14:04:32, Peter Beverloo wrote: > micro nit: {} can be on the same line Deleted. https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:107: scoped_ptr<DocumentRegisterData>(new DocumentRegisterData); On 2014/12/04 14:04:32, Peter Beverloo wrote: > nit: make_scoped_ptr() Deleted. https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:114: this, data, sender_id)); On 2014/12/04 14:04:32, Peter Beverloo wrote: > Hmm. We're now ending up in the following situation: > > (1) We allocate a new DocumentRegisterData object on line 106. > (2) We invoke the copy constructor of RegisterData on line 114 (pass by value to > base::Bind). > (3) The copy constructor allocates a new instance of DocumentRegisterData and > invokes the compiler-generated copy ctor of that. > (4) The allocation made in (1) goes out of scope, and gets freed. > > Can we find a way to put the DocumentRegisterData in RegisterData to avoid the > custom DRD copy constructor, as well as the on-heap/on-stack mixture? > > Perhaps one option would be to have a helper function that validates that > render_frame_id in fact is a valid render frame, rather than checking whether > |document_data| is assigned some memory? Done. https://codereview.chromium.org/769273004/diff/20001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:206: weak_factory_ui_to_ui_.GetWeakPtr(), data)); On 2014/12/04 14:04:32, Peter Beverloo wrote: > nit: maybe |weak_factory_ui_|? That would make it ambiguous whether ui refers to the sender, recipient, or both (and technically that's part of https://codereview.chromium.org/770423003 rather than this CL).
https://codereview.chromium.org/769273004/diff/40001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/769273004/diff/40001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.h:31: struct RegisterData { This needs a constructor setting defaults for the non-scalar values (also allows you to avoid initializing render_frame_id and user_visible_only for the worker case). Having *some* code in the .cc file also allows you to inline from_document(), and avoid including child_process_host here. (Right now, user_visible_only is uninitialized for the Workers case, this will make msan unhappy.)
lgtm % nits
https://codereview.chromium.org/769273004/diff/60001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/769273004/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.h:32: RegisterData(const RegisterData& other) = default; The C++11 style guide says: "Doesn't work for move constructors and move assignment operators in MSVC2013" Does that apply here? It is a move constructor. https://codereview.chromium.org/769273004/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.h:33: bool from_document() const; IsDocument() since it's not a getter. Sorry I missed this in the previous round!
lgtm
https://codereview.chromium.org/769273004/diff/60001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/769273004/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.h:32: RegisterData(const RegisterData& other) = default; On 2014/12/05 12:39:09, Peter Beverloo wrote: > The C++11 style guide says: > > "Doesn't work for move constructors and move assignment operators in MSVC2013" > > Does that apply here? It is a move constructor. I understand the more detailed explanation at https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qgU4mh_MpGA/t0lmD... as meaning that it's fine to use "= default" for "A::A(const A&)", just not after "A::A(A&&) = default" (which would be banned anyway in Chrome). https://codereview.chromium.org/769273004/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.h:33: bool from_document() const; On 2014/12/05 12:39:09, Peter Beverloo wrote: > IsDocument() since it's not a getter. Sorry I missed this in the previous round! Done.
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/769273004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3068082ae5f0063bd998517ad820d0bf53fb73fe Cr-Commit-Position: refs/heads/master@{#307018} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
