Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1109933004:
Adding a comment for MessageLoopProxy deprecation in message_loop_proxy.h (Closed)
|
Created:
5 years, 7 months ago by anujsharma Modified:
5 years, 7 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, sadrul, Ryan Sleevi, stevenjb, Pranay Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding a comment to message_loop_proxy.h informing developers of its deprecation.
base::MessageLoopProxy::current() -> base::ThreadTaskRunnerHandle::Get()
scoped_refptr<base::MessageLoopProxy> -> scoped_refptr<base::SingleThreadTaskRunner>
base::MessageLoopProxy -> base::SingleThreadTaskRunner
BUG=391045
Committed: https://crrev.com/c4a4946256cd0abd02b187bd9efbd8d9b3c99b2a
Cr-Commit-Position: refs/heads/master@{#328294}
Patch Set 1 #Patch Set 2 : Fixed suggestion #
Total comments: 6
Patch Set 3 : Fixed comments #
Total comments: 6
Patch Set 4 : Adding nits #
Total comments: 2
Patch Set 5 : Fixed nit #Messages
Total messages: 20 (3 generated)
anujk.sharma@samsung.com changed reviewers: + thestig@chromium.org
PTAL This is simply adding a comment in message_loop_proxy.h for deprecation. Thanks!!
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Not LG (but not saying TM to avoid needing to re-review) The TODO is unnecessary, nor is it how Chromium/Google STYLE guide do TODOs. Just describe what is going on, what to do, where to go for more info... "MessageLoopProxy is depreciated. Code should prefer to depend on TaskRunner (or the various specializations) for passing task runners around, and should use ThreadTaskRunnerHandle::Get() to get the thread's associated task runner. See crbug.com/foo for more details." Or *something* like that. Document the thing is depreciated, provide an example of how to accomplish it with the new thing, and move on from there.
On 2015/04/30 06:31:07, Ryan Sleevi wrote: > Not LG (but not saying TM to avoid needing to re-review) > > The TODO is unnecessary, nor is it how Chromium/Google STYLE guide do TODOs. > > Just describe what is going on, what to do, where to go for more info... > > "MessageLoopProxy is depreciated. Code should prefer to depend on TaskRunner (or > the various specializations) for passing task runners around, and should use > ThreadTaskRunnerHandle::Get() to get the thread's associated task runner. > > See crbug.com/foo for more details." > > Or *something* like that. > > Document the thing is depreciated, provide an example of how to accomplish it > with the new thing, and move on from there. Ok sure!! I will make the changes and re-upload the same. Thanks for your comments!!
Thanks Ryan for guiding me to use proper syntax. I have made some changes as per your suggestion. PTAL
The CL description can use some work too. Why say "This is basically to foo" when you can just say "foo" ? https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... File base/message_loop/message_loop_proxy.h (right): https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:13: //MessageLoopProxy is depreciated. Code should prefer to depend on TaskRunner(or - Comments have a space after "//" - Space after TaskRunner https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:17: //See crbug.com/391045 for more details. Ad http:// https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:18: // This basically does a search and replace: You can omit this line.
Thanks Lei for your comments!! I have fixed the comments. Could you please take a look now. Thanks!! https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... File base/message_loop/message_loop_proxy.h (right): https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:13: //MessageLoopProxy is depreciated. Code should prefer to depend on TaskRunner(or On 2015/04/30 20:52:02, Lei Zhang wrote: > - Comments have a space after "//" > - Space after TaskRunner Done. https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:17: //See crbug.com/391045 for more details. On 2015/04/30 20:52:02, Lei Zhang wrote: > Ad http:// Done. https://codereview.chromium.org/1109933004/diff/20001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:18: // This basically does a search and replace: On 2015/04/30 20:52:02, Lei Zhang wrote: > You can omit this line. Done.
You haven't updated the CL description. https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... File base/message_loop/message_loop_proxy.h (right): https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:18: // Example for these changes:- There's an extra '-' here.
https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... File base/message_loop/message_loop_proxy.h (right): https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:13: // MessageLoopProxy is depreciated. Code should prefer to depend on TaskRunner s/depreciated/deprecated/ https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:22: // scoped_refptr<SingleThreadTaskRunner> This is, of course, wrong - missing a base:: qualifier (since you omit base on lines 20 & 23, presumably you should omit them on both line 21/22 rather than mixing and matching)
Thanks lei and Ryan for your comments!! I have fixed the changes. Could you please take a look now. Thanks!! https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... File base/message_loop/message_loop_proxy.h (right): https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:13: // MessageLoopProxy is depreciated. Code should prefer to depend on TaskRunner On 2015/05/05 01:09:38, Ryan Sleevi wrote: > s/depreciated/deprecated/ Done. https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:18: // Example for these changes:- On 2015/05/05 00:40:31, Lei Zhang wrote: > There's an extra '-' here. Removed!! https://codereview.chromium.org/1109933004/diff/40001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:22: // scoped_refptr<SingleThreadTaskRunner> On 2015/05/05 01:09:38, Ryan Sleevi wrote: > This is, of course, wrong - missing a base:: qualifier (since you omit base on > lines 20 & 23, presumably you should omit them on both line 21/22 rather than > mixing and matching) Done.
@Lei I have modified the CL description as well. PTAL Thanks!!
https://codereview.chromium.org/1109933004/diff/60001/base/message_loop/messa... File base/message_loop/message_loop_proxy.h (right): https://codereview.chromium.org/1109933004/diff/60001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:23: // MessageLoopProxy -> SingleThreadTaskRunner If you are going to use base:: in the above lines in these examples, shouldn't you do the same here?
Ah my bad :( Thanks for notifying this. Done now!! PTAL https://codereview.chromium.org/1109933004/diff/60001/base/message_loop/messa... File base/message_loop/message_loop_proxy.h (right): https://codereview.chromium.org/1109933004/diff/60001/base/message_loop/messa... base/message_loop/message_loop_proxy.h:23: // MessageLoopProxy -> SingleThreadTaskRunner On 2015/05/05 04:31:26, Lei Zhang wrote: > If you are going to use base:: in the above lines in these examples, shouldn't > you do the same here? Done.
lgtm
On 2015/05/05 04:39:17, Lei Zhang wrote: > lgtm Thanks lei for lgtm.
The CQ bit was checked by anujk.sharma@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1109933004/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/c4a4946256cd0abd02b187bd9efbd8d9b3c99b2a Cr-Commit-Position: refs/heads/master@{#328294} |
