Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2012473002:
Remove NTP dependency on //content/... (Closed)
DescriptionRemove NTP dependency on //content/...
Provide threads to MostVisitedSites from MostVisitedSitesBridge; provide
those threads in turn to PopularSites. These classes are moving to
components, and the //content/... dep needs to be broken to compile on
iOS.
Moves the thumbnail read that was previously on the DB thread onto the
blocking thread pool so as to avoid needing to pass the DB thread in in
addition to the blocking pool.
BUG=603026
Committed: https://crrev.com/3e0b88bd66abcb8ab5a649ac9c41479737819ad8
Cr-Commit-Position: refs/heads/master@{#396445}
Patch Set 1 #Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : thread checker #
Total comments: 6
Patch Set 6 : #Patch Set 7 : sync #
Dependent Patchsets: Messages
Total messages: 45 (16 generated)
sfiera@chromium.org changed reviewers: + bauerb@chromium.org
blundell@chromium.org changed reviewers: + blundell@chromium.org
drive-by Nice work on the quick turnaround! https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (left): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:265: BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior( and this code stays in this class, just called on worker_pool. https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:189: const scoped_refptr<base::TaskRunner>& blocking_runner, this needs to be a SequencedWorkerPool*, no?
https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:127: MostVisitedSites(const scoped_refptr<base::SingleThreadTaskRunner>& ui_thread, If you are going to keep a reference, take the scoped_refptr by value and std::move it when assigning it to the member. https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (left): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:265: BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior( Could you keep this shutdown behavior?
treib@chromium.org changed reviewers: + treib@chromium.org
Nice work! (And sorry for not catching this earlier) https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:251: // TODO(treib): Move this to the blocking pool? Doesn't seem related to DB. Hm, maybe now is the time to resolve this TODO, so we don't have to pass around the DB thread? :) https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:294: ui_thread_->PostTask(FROM_HERE, base::Bind(callback_, false)); Hm. Do we actually need/want the UI thread here? Can we just post a task to the current thread? (That's what the other places in this file do.) I realize it's a pre-existing problem, but if we change that now we don't have to pass around the UI thread.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:251: // TODO(treib): Move this to the blocking pool? Doesn't seem related to DB. On 2016/05/24 15:16:11, Marc Treib wrote: > Hm, maybe now is the time to resolve this TODO, so we don't have to pass around > the DB thread? :) Done. https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:127: MostVisitedSites(const scoped_refptr<base::SingleThreadTaskRunner>& ui_thread, On 2016/05/24 15:14:47, Bernhard Bauer wrote: > If you are going to keep a reference, take the scoped_refptr by value and > std::move it when assigning it to the member. Done. https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (left): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:265: BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior( On 2016/05/24 15:14:47, Bernhard Bauer wrote: > Could you keep this shutdown behavior? Done. https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:189: const scoped_refptr<base::TaskRunner>& blocking_runner, On 2016/05/24 15:12:48, blundell wrote: > this needs to be a SequencedWorkerPool*, no? Done. https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:294: ui_thread_->PostTask(FROM_HERE, base::Bind(callback_, false)); On 2016/05/24 15:16:11, Marc Treib wrote: > Hm. Do we actually need/want the UI thread here? Can we just post a task to the > current thread? (That's what the other places in this file do.) > > I realize it's a pre-existing problem, but if we change that now we don't have > to pass around the UI thread. We want a single thread and not a thread pool (so that this task can't run immediately), but it doesn't have to be the UI thread, even though it always is in practice. I'm not familiar enough with Chrome threading to know the best way to guarantee that, but I'll take any pointers.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012473002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012473002/40001
https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:294: ui_thread_->PostTask(FROM_HERE, base::Bind(callback_, false)); On 2016/05/24 16:59:20, sfiera wrote: > On 2016/05/24 15:16:11, Marc Treib wrote: > > Hm. Do we actually need/want the UI thread here? Can we just post a task to > the > > current thread? (That's what the other places in this file do.) > > > > I realize it's a pre-existing problem, but if we change that now we don't have > > to pass around the UI thread. > > We want a single thread and not a thread pool (so that this task can't run > immediately), but it doesn't have to be the UI thread, even though it always is > in practice. I'm not familiar enough with Chrome threading to know the best way > to guarantee that, but I'll take any pointers. I think ThreadTaskRunnerHandle::Get()->PostTask(...) is what you're looking for.
https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:294: ui_thread_->PostTask(FROM_HERE, base::Bind(callback_, false)); On 2016/05/24 16:59:20, sfiera wrote: > On 2016/05/24 15:16:11, Marc Treib wrote: > > Hm. Do we actually need/want the UI thread here? Can we just post a task to > the > > current thread? (That's what the other places in this file do.) > > > > I realize it's a pre-existing problem, but if we change that now we don't have > > to pass around the UI thread. > > We want a single thread and not a thread pool (so that this task can't run > immediately), but it doesn't have to be the UI thread, even though it always is > in practice. I'm not familiar enough with Chrome threading to know the best way > to guarantee that, but I'll take any pointers. I think ThreadTaskRunnerHandle::Get()->PostTask(...) is what you're looking for.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2012473002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:398: ->GetTaskRunnerWithShutdownBehavior( This is a behavioral change, as you're no longer supplying the same task runner in all of these calls (this call just returns *a* TaskRunner from the pool). I would just preserve the |runner_| variable.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012473002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012473002/60001
https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:294: ui_thread_->PostTask(FROM_HERE, base::Bind(callback_, false)); On 2016/05/24 17:12:03, Marc Treib wrote: > On 2016/05/24 16:59:20, sfiera wrote: > > On 2016/05/24 15:16:11, Marc Treib wrote: > > > Hm. Do we actually need/want the UI thread here? Can we just post a task to > > the > > > current thread? (That's what the other places in this file do.) > > > > > > I realize it's a pre-existing problem, but if we change that now we don't > have > > > to pass around the UI thread. > > > > We want a single thread and not a thread pool (so that this task can't run > > immediately), but it doesn't have to be the UI thread, even though it always > is > > in practice. I'm not familiar enough with Chrome threading to know the best > way > > to guarantee that, but I'll take any pointers. > > I think ThreadTaskRunnerHandle::Get()->PostTask(...) is what you're looking for. Done. I removed the UI thread references from this file, but I still have them in most_visited_sites.cc to preserve DCHECKs. It looks like maybe I could save base::ThreadTaskRunnerHandle::Get() as original_thread_ and DCHECK(original_thread_->BelongsToCurrentThread()). Would that be correct? https://codereview.chromium.org/2012473002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/popular_sites.cc (right): https://codereview.chromium.org/2012473002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/popular_sites.cc:398: ->GetTaskRunnerWithShutdownBehavior( On 2016/05/24 18:01:45, blundell wrote: > This is a behavioral change, as you're no longer supplying the same task runner > in all of these calls (this call just returns *a* TaskRunner from the pool). I > would just preserve the |runner_| variable. Done. (In practice I think it should end up the same, since we never run more than one task from this file at once, but there's no reason to change it to use different runners either)
On 2016/05/24 18:18:59, sfiera wrote: > https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... > File chrome/browser/android/ntp/popular_sites.cc (right): > > https://codereview.chromium.org/2012473002/diff/20001/chrome/browser/android/... > chrome/browser/android/ntp/popular_sites.cc:294: ui_thread_->PostTask(FROM_HERE, > base::Bind(callback_, false)); > On 2016/05/24 17:12:03, Marc Treib wrote: > > On 2016/05/24 16:59:20, sfiera wrote: > > > On 2016/05/24 15:16:11, Marc Treib wrote: > > > > Hm. Do we actually need/want the UI thread here? Can we just post a task > to > > > the > > > > current thread? (That's what the other places in this file do.) > > > > > > > > I realize it's a pre-existing problem, but if we change that now we don't > > have > > > > to pass around the UI thread. > > > > > > We want a single thread and not a thread pool (so that this task can't run > > > immediately), but it doesn't have to be the UI thread, even though it always > > is > > > in practice. I'm not familiar enough with Chrome threading to know the best > > way > > > to guarantee that, but I'll take any pointers. > > > > I think ThreadTaskRunnerHandle::Get()->PostTask(...) is what you're looking > for. > > Done. > > I removed the UI thread references from this file, but I still have them in > most_visited_sites.cc to preserve DCHECKs. It looks like maybe I could save > base::ThreadTaskRunnerHandle::Get() as original_thread_ and > DCHECK(original_thread_->BelongsToCurrentThread()). Would that be correct? You can also use a base::ThreadChecker ivar, which you can call whenever you want to check that the current thread is the one on which the instance was created. > > https://codereview.chromium.org/2012473002/diff/40001/chrome/browser/android/... > File chrome/browser/android/ntp/popular_sites.cc (right): > > https://codereview.chromium.org/2012473002/diff/40001/chrome/browser/android/... > chrome/browser/android/ntp/popular_sites.cc:398: > ->GetTaskRunnerWithShutdownBehavior( > On 2016/05/24 18:01:45, blundell wrote: > > This is a behavioral change, as you're no longer supplying the same task > runner > > in all of these calls (this call just returns *a* TaskRunner from the pool). I > > would just preserve the |runner_| variable. > > Done. > > (In practice I think it should end up the same, since we never run more than one > task from this file at once, but there's no reason to change it to use different > runners either)
Cool, ThreadChecker added.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012473002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012473002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Awesome! LGTM with one question below. https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:127: MostVisitedSites(scoped_refptr<base::SequencedWorkerPool> blocking_pool, It looks like you actually only need one TaskRunner, rather than a whole pool. Would it be better to just pass in a TaskRunner? (Actual question - I'm not sure what's better. One consequence would be that the shutdown behavior would be defined outside of this class.)
https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:127: MostVisitedSites(scoped_refptr<base::SequencedWorkerPool> blocking_pool, On 2016/05/25 08:39:26, Marc Treib wrote: > It looks like you actually only need one TaskRunner, rather than a whole pool. > Would it be better to just pass in a TaskRunner? (Actual question - I'm not sure > what's better. One consequence would be that the shutdown behavior would be > defined outside of this class.) That's what I had initially, but bauerb asked to keep shutdown behavior and blundell suggested the SWP here.
LGTM Please add motivation to the CL description (i.e., for intending componentization and sharing with iOS, which can't use //content) https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:248: blocking_pool_ Usually I would suggest making behavioral changes in separate CLs from dependency injection changes for separation of concerns. Something to note for the future. In this case, please add the behavioral change to the CL description. https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:127: MostVisitedSites(scoped_refptr<base::SequencedWorkerPool> blocking_pool, On 2016/05/25 08:44:44, sfiera wrote: > On 2016/05/25 08:39:26, Marc Treib wrote: > > It looks like you actually only need one TaskRunner, rather than a whole pool. > > Would it be better to just pass in a TaskRunner? (Actual question - I'm not > sure > > what's better. One consequence would be that the shutdown behavior would be > > defined outside of this class.) > > That's what I had initially, but bauerb asked to keep shutdown behavior and > blundell suggested the SWP here. Yeah, I think that given that this class explicitly defines shutdown behavior, it makes sense to keep that logic internal to the class.
Description was changed from ========== Remove NTP dependency on //content/... Provide threads to MostVisitedSites from MostVisitedSitesBridge; provide those threads in turn to PopularSites. BUG=603026 ========== to ========== Remove NTP dependency on //content/... Provide threads to MostVisitedSites from MostVisitedSitesBridge; provide those threads in turn to PopularSites. These classes are moving to components, and the //content/... dep needs to be broken to compile on iOS. Moves the thumbnail read that was previously on the DB thread onto the blocking thread pool so as to avoid needing to pass the DB thread in in addition to the blocking pool. BUG=603026 ==========
Description updated. Thanks for the drive-by! (still need an LGTM from bauerb, the *actual* reviewer :))
LGTM, but one last issue: https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:249: ->GetTaskRunnerWithShutdownBehavior( Sorry to barge in again here -- this creates a new TaskRunner every time, which is kind of unnecessary. You can just get a TaskRunner in the constructor and store that in a member instead of the SWP. (Also, just for the sake of completeness: the TaskRunner here does not guarantee that tasks will run sequentially; a SequencedTaskRunner would do that. That's probably fine in this case, but just something to be aware of.)
I'll tick the dry run now and if you don't have anything to add before that finishes, submit when it does. https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/2012473002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:249: ->GetTaskRunnerWithShutdownBehavior( On 2016/05/25 17:02:49, Bernhard Bauer wrote: > Sorry to barge in again here -- this creates a new TaskRunner every time, which > is kind of unnecessary. You can just get a TaskRunner in the constructor and > store that in a member instead of the SWP. OK, done. Though that's in addition to the SWP, which is needed for PopularSites, instead of instead of. > (Also, just for the sake of completeness: the TaskRunner here does not guarantee > that tasks will run sequentially; a SequencedTaskRunner would do that. That's > probably fine in this case, but just something to be aware of.) Seems like it's probably preferable to support parallel fetch of thumbnails this way, but it's called from Java code that I don't know.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012473002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012473002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2012473002/#ps120001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012473002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove NTP dependency on //content/... Provide threads to MostVisitedSites from MostVisitedSitesBridge; provide those threads in turn to PopularSites. These classes are moving to components, and the //content/... dep needs to be broken to compile on iOS. Moves the thumbnail read that was previously on the DB thread onto the blocking thread pool so as to avoid needing to pass the DB thread in in addition to the blocking pool. BUG=603026 ========== to ========== Remove NTP dependency on //content/... Provide threads to MostVisitedSites from MostVisitedSitesBridge; provide those threads in turn to PopularSites. These classes are moving to components, and the //content/... dep needs to be broken to compile on iOS. Moves the thumbnail read that was previously on the DB thread onto the blocking thread pool so as to avoid needing to pass the DB thread in in addition to the blocking pool. BUG=603026 Committed: https://crrev.com/3e0b88bd66abcb8ab5a649ac9c41479737819ad8 Cr-Commit-Position: refs/heads/master@{#396445} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3e0b88bd66abcb8ab5a649ac9c41479737819ad8 Cr-Commit-Position: refs/heads/master@{#396445} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
