Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1126963003:
[components/storage_monitor] Use ThreadTaskRunnerHandle instead of LoopProxy (Closed)
|
Created:
5 years, 7 months ago by Pranay Modified:
5 years, 7 months ago Reviewers:
Lei Zhang CC:
chromium-reviews, anujsharma, vandebo (ex-Chrome), gbillock Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace MessageLoopProxy usage with ThreadTaskRunnerHandle in components/storage_monitor module.
MessageLoopProxy is deprecated.
This basically does a search and replace:
MessageLoopProxy::current() -> ThreadTaskRunnerHandle::Get().
BUG=391045
Committed: https://crrev.com/7344e0bb8d8760931c2613607946ff1645968321
Cr-Commit-Position: refs/heads/master@{#330720}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fixed Review Comments #
Total comments: 2
Patch Set 3 : Fixed Review Comments 2 #
Total comments: 2
Messages
Total messages: 25 (8 generated)
pranay.kumar@samsung.com changed reviewers: + erikwright@chromium.org
Hi, PTAL Thanks
On 2015/05/06 11:14:45, Pranay wrote: > Hi, > PTAL > > Thanks Please select a more specific OWNER when possible. The following list, in this case: https://code.google.com/p/chromium/codesearch#chromium/src/components/storage...
https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... File components/storage_monitor/storage_monitor_chromeos.cc (right): https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... components/storage_monitor/storage_monitor_chromeos.cc:110: device::MediaTransferProtocolManager::Initialize(task_runner)); shouldn't this just be ...Initialize(scoped_refptr<base::SingleThreadTaskRunner>()) ? https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... File components/storage_monitor/storage_monitor_linux.cc (right): https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... components/storage_monitor/storage_monitor_linux.cc:270: device::MediaTransferProtocolManager::Initialize(task_runner)); here, as well, I would just inline this.
erikwright@chromium.org changed reviewers: - erikwright@chromium.org
https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... File components/storage_monitor/storage_monitor_chromeos.cc (right): https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... components/storage_monitor/storage_monitor_chromeos.cc:110: device::MediaTransferProtocolManager::Initialize(task_runner)); On 2015/05/11 18:29:18, erikwright wrote: > shouldn't this just be > > ...Initialize(scoped_refptr<base::SingleThreadTaskRunner>()) > > ? Done. https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... File components/storage_monitor/storage_monitor_linux.cc (right): https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... components/storage_monitor/storage_monitor_linux.cc:270: device::MediaTransferProtocolManager::Initialize(task_runner)); On 2015/05/11 18:29:18, erikwright wrote: > here, as well, I would just inline this. Done.
pranay.kumar@samsung.com changed reviewers: + vandebo@chromium.org
PTAL Thanks
pranay.kumar@samsung.com changed reviewers: + gbillock@google.com
Since Vandebo is not part of Chrome now, adding Gbillock as reviewer. PTAL
vandebo@chromium.org changed reviewers: + thestig@chromium.org - gbillock@google.com, vandebo@chromium.org
Sorry I missed this one, I usually redirect immediately. Lei can help you out or redirect better.
Thanks Vandebo for your reply..
https://codereview.chromium.org/1126963003/diff/20001/components/storage_moni... File components/storage_monitor/storage_monitor_linux.cc (right): https://codereview.chromium.org/1126963003/diff/20001/components/storage_moni... components/storage_monitor/storage_monitor_linux.cc:270: scoped_refptr<base::SingleThreadTaskRunner>())); Uh, this isn't right. Look closely.
Done https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... File components/storage_monitor/storage_monitor_linux.cc (right): https://codereview.chromium.org/1126963003/diff/1/components/storage_monitor/... components/storage_monitor/storage_monitor_linux.cc:270: device::MediaTransferProtocolManager::Initialize(task_runner)); Sorry..earlier changes were incorrect. Fixed as pointed out by Lei https://codereview.chromium.org/1126963003/diff/20001/components/storage_moni... File components/storage_monitor/storage_monitor_linux.cc (right): https://codereview.chromium.org/1126963003/diff/20001/components/storage_moni... components/storage_monitor/storage_monitor_linux.cc:270: scoped_refptr<base::SingleThreadTaskRunner>())); Oops! I'm sorry. Thanks for pointing out, Lei.
lgtm https://codereview.chromium.org/1126963003/diff/40001/components/storage_moni... File components/storage_monitor/storage_monitor_linux.cc (right): https://codereview.chromium.org/1126963003/diff/40001/components/storage_moni... components/storage_monitor/storage_monitor_linux.cc:268: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE))); So this returns a MessageLoopProxy. What's the long term plan to fix that?
Thanks for the LGTM, Lei. Since GetMessageLoopProxyForThread(ID) deals over "proxies" (scoped_refptr<base::MessageLoopProxy> proxies[BrowserThread::ID_COUNT]), so that implementation could be changed, via introduction of a BrowserThreadSequencedTaskRunner class ( in analogy of BrowserThreadMessageLoopProxy replacing it or along with) or direct conversion inside GetMessageLoopProxyForThread(ID) can be done (just abstract opinion of mine). Let me know your thoughts over the same :) -Pranay
The CQ bit was checked by pranay.kumar@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1126963003/40001
The CQ bit was unchecked by pranay.kumar@samsung.com
https://codereview.chromium.org/1126963003/diff/40001/components/storage_moni... File components/storage_monitor/storage_monitor_linux.cc (right): https://codereview.chromium.org/1126963003/diff/40001/components/storage_moni... components/storage_monitor/storage_monitor_linux.cc:268: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE))); On 2015/05/15 05:03:34, Lei Zhang wrote: > So this returns a MessageLoopProxy. What's the long term plan to fix that? Acknowledged.
The CQ bit was checked by pranay.kumar@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1126963003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7344e0bb8d8760931c2613607946ff1645968321 Cr-Commit-Position: refs/heads/master@{#330720} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
