Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12952005:
Delay bookmarks load while the profile is loading. (Closed)
|
Created:
7 years, 9 months ago by msarda Modified:
7 years, 8 months ago Reviewers:
sail, willchan no longer on Chromium, erikwright (departed), willchan, Jeffrey Yasskin, Miranda Callahan, mirandac, sky CC:
chromium-reviews, tfarina, rginda+watch_chromium.org, erikwright+watch_chromium.org, browser-components-watch_chromium.org, sail+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDelay bookmarks load while the profile is loading.
This CL adds a new DeferredSequencedtaskRunner that queues up tasks
until a first call to Start is issued. It creates such a task runner for the
execution of bookmarks I/O operations. At profile creation, the bookmarks
task runner is stopped and its execution is started after the profile has
finished loading.
BUG=NONE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194956
Patch Set 1 #Patch Set 2 : Use DeferredSequencedTaskRunner instead of PausableSequencedTaskRunner. #Patch Set 3 : Fix testing_profile.h #
Total comments: 3
Patch Set 4 : Move BookmarkTaskRunner to BookmarkModel. #Patch Set 5 : Fix nits. #
Total comments: 5
Patch Set 6 : Address mirandac's comments: add BookmarkModel::StartLoading #
Total comments: 16
Patch Set 7 : Add startup_task_runner_service. #
Total comments: 4
Patch Set 8 : Add deferred_sequenced_task_runner_unittest #
Total comments: 40
Patch Set 9 : Address willchan's and jyasskin code reviews. #
Total comments: 12
Patch Set 10 : Fix deferred_sequenced_task_runner_unittest.cc nits. #Patch Set 11 : Fix DeferredSequencedTaskRunnerTest.DeferredStartWithMultipleThreads #Patch Set 12 : Erase the deferred task queue during iteration. #Patch Set 13 : Add ObjectDestructionOrder unit test. #
Total comments: 58
Patch Set 14 : Address erikwright's code review. #Patch Set 15 : Fix nits. #
Total comments: 2
Patch Set 16 : Fix erase #Patch Set 17 : Fix sync_integration_tests #Patch Set 18 : Add start_deferred_task_runners argument to RegisterTestingProfile #Patch Set 19 : Fix comment. #Messages
Total messages: 68 (0 generated)
Please take a look.
https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... File chrome/browser/profiles/profile_impl.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... chrome/browser/profiles/profile_impl.cc:144: COMPILE_ASSERT(sizeof(ProfileImpl) <= 744u, profile_impl_size_unexpected); Will your patch break this?
https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... File chrome/browser/profiles/profile_impl.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... chrome/browser/profiles/profile_impl.cc:144: COMPILE_ASSERT(sizeof(ProfileImpl) <= 744u, profile_impl_size_unexpected); On 2013/03/26 17:25:02, sail wrote: > Will your patch break this? Ninja compiled the code on my machine so I suppose that compile asserts are passing. Am I wrong? What else should I do to test if this breaks?
https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... File chrome/browser/profiles/profile_impl.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... chrome/browser/profiles/profile_impl.cc:144: COMPILE_ASSERT(sizeof(ProfileImpl) <= 744u, profile_impl_size_unexpected); On 2013/03/26 17:33:26, msarda wrote: > On 2013/03/26 17:25:02, sail wrote: > > Will your patch break this? > > Ninja compiled the code on my machine so I suppose that compile asserts are > passing. Am I wrong? > What else should I do to test if this breaks? Maybe we don't support COMPILE_ASSERT on your platform. To avoid hitting this assert you should remove the member variable you added to ProfileImpl.
I believe that this assert will only fire on Linux -- it should pop up in the trybot runs. And yes, it will almost certainly fire. Is there a way to make this a ProfileKeyedService? On 2013/03/26 17:46:24, sail wrote: > https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... > File chrome/browser/profiles/profile_impl.cc (right): > > https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/prof... > chrome/browser/profiles/profile_impl.cc:144: COMPILE_ASSERT(sizeof(ProfileImpl) > <= 744u, profile_impl_size_unexpected); > On 2013/03/26 17:33:26, msarda wrote: > > On 2013/03/26 17:25:02, sail wrote: > > > Will your patch break this? > > > > Ninja compiled the code on my machine so I suppose that compile asserts are > > passing. Am I wrong? > > What else should I do to test if this breaks? > > Maybe we don't support COMPILE_ASSERT on your platform. > To avoid hitting this assert you should remove the member variable you added to > ProfileImpl.
On Tue, Mar 26, 2013 at 10:51 AM, <mirandac@chromium.org> wrote: > I believe that this assert will only fire on Linux -- it should pop up in > the > trybot runs. And yes, it will almost certainly fire. Is there a way to > make this > a ProfileKeyedService? It works on Mac too. Go Mac! > > > On 2013/03/26 17:46:24, sail wrote: > > https://chromiumcodereview.**appspot.com/12952005/diff/** > 4005/chrome/browser/profiles/**profile_impl.cc<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc> > >> File chrome/browser/profiles/**profile_impl.cc (right): >> > > > https://chromiumcodereview.**appspot.com/12952005/diff/** > 4005/chrome/browser/profiles/**profile_impl.cc#newcode144<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc#newcode144> > >> chrome/browser/profiles/**profile_impl.cc:144: >> > COMPILE_ASSERT(sizeof(**ProfileImpl) > >> <= 744u, profile_impl_size_unexpected); >> On 2013/03/26 17:33:26, msarda wrote: >> > On 2013/03/26 17:25:02, sail wrote: >> > > Will your patch break this? >> > >> > Ninja compiled the code on my machine so I suppose that compile asserts >> are >> > passing. Am I wrong? >> > What else should I do to test if this breaks? >> > > Maybe we don't support COMPILE_ASSERT on your platform. >> To avoid hitting this assert you should remove the member variable you >> added >> > to > >> ProfileImpl. >> > > > > https://chromiumcodereview.**appspot.com/12952005/<https://chromiumcodereview... >
The ninja build worked for me on mac. On 2013/03/26 17:55:13, sail wrote: > On Tue, Mar 26, 2013 at 10:51 AM, <mailto:mirandac@chromium.org> wrote: > > > I believe that this assert will only fire on Linux -- it should pop up in > > the > > trybot runs. And yes, it will almost certainly fire. Is there a way to > > make this > > a ProfileKeyedService? > > > It works on Mac too. Go Mac! > > > > > > > > On 2013/03/26 17:46:24, sail wrote: > > > > https://chromiumcodereview.**appspot.com/12952005/diff/** > > > 4005/chrome/browser/profiles/**profile_impl.cc<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc> > > > >> File chrome/browser/profiles/**profile_impl.cc (right): > >> > > > > > > https://chromiumcodereview.**appspot.com/12952005/diff/** > > > 4005/chrome/browser/profiles/**profile_impl.cc#newcode144<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc#newcode144> > > > >> chrome/browser/profiles/**profile_impl.cc:144: > >> > > COMPILE_ASSERT(sizeof(**ProfileImpl) > > > >> <= 744u, profile_impl_size_unexpected); > >> On 2013/03/26 17:33:26, msarda wrote: > >> > On 2013/03/26 17:25:02, sail wrote: > >> > > Will your patch break this? > >> > > >> > Ninja compiled the code on my machine so I suppose that compile asserts > >> are > >> > passing. Am I wrong? > >> > What else should I do to test if this breaks? > >> > > > > Maybe we don't support COMPILE_ASSERT on your platform. > >> To avoid hitting this assert you should remove the member variable you > >> added > >> > > to > > > >> ProfileImpl. > >> > > > > > > > > > https://chromiumcodereview.**appspot.com/12952005/%3Chttps://chromiumcoderevi...> > >
On 2013/03/26 17:56:22, msarda wrote: > The ninja build worked for me on mac. Ahh, sorry. The assert is #ifdefed out on Mac. Boo Mac :-( Here's the #ifdef: #if defined(OS_LINUX) && defined(TOOLKIT_GTK) && defined(ARCH_CPU_X86_64) && \ !defined(_GLIBCXX_DEBUG) > > On 2013/03/26 17:55:13, sail wrote: > > On Tue, Mar 26, 2013 at 10:51 AM, <mailto:mirandac@chromium.org> wrote: > > > > > I believe that this assert will only fire on Linux -- it should pop up in > > > the > > > trybot runs. And yes, it will almost certainly fire. Is there a way to > > > make this > > > a ProfileKeyedService? > > > > > > It works on Mac too. Go Mac! > > > > > > > > > > > > > On 2013/03/26 17:46:24, sail wrote: > > > > > > https://chromiumcodereview.**appspot.com/12952005/diff/** > > > > > > 4005/chrome/browser/profiles/**profile_impl.cc<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc> > > > > > >> File chrome/browser/profiles/**profile_impl.cc (right): > > >> > > > > > > > > > https://chromiumcodereview.**appspot.com/12952005/diff/** > > > > > > 4005/chrome/browser/profiles/**profile_impl.cc#newcode144<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc#newcode144> > > > > > >> chrome/browser/profiles/**profile_impl.cc:144: > > >> > > > COMPILE_ASSERT(sizeof(**ProfileImpl) > > > > > >> <= 744u, profile_impl_size_unexpected); > > >> On 2013/03/26 17:33:26, msarda wrote: > > >> > On 2013/03/26 17:25:02, sail wrote: > > >> > > Will your patch break this? > > >> > > > >> > Ninja compiled the code on my machine so I suppose that compile asserts > > >> are > > >> > passing. Am I wrong? > > >> > What else should I do to test if this breaks? > > >> > > > > > > Maybe we don't support COMPILE_ASSERT on your platform. > > >> To avoid hitting this assert you should remove the member variable you > > >> added > > >> > > > to > > > > > >> ProfileImpl. > > >> > > > > > > > > > > > > > > > https://chromiumcodereview.**appspot.com/12952005/%253Chttps://chromiumcodere...> > > >
I think it only fires for Linux because the size is set specifically for the Linux build -- not sure that would be valid for other OSses, even Mac. I talked to Mihai on IRC about making this var a ProfileKeyedService, and he's all set for doing it that way. On 2013/03/26 17:59:23, sail wrote: > On 2013/03/26 17:56:22, msarda wrote: > > The ninja build worked for me on mac. > > Ahh, sorry. The assert is #ifdefed out on Mac. Boo Mac :-( > > Here's the #ifdef: > #if defined(OS_LINUX) && defined(TOOLKIT_GTK) && defined(ARCH_CPU_X86_64) && \ > !defined(_GLIBCXX_DEBUG) > > > > > On 2013/03/26 17:55:13, sail wrote: > > > On Tue, Mar 26, 2013 at 10:51 AM, <mailto:mirandac@chromium.org> wrote: > > > > > > > I believe that this assert will only fire on Linux -- it should pop up in > > > > the > > > > trybot runs. And yes, it will almost certainly fire. Is there a way to > > > > make this > > > > a ProfileKeyedService? > > > > > > > > > It works on Mac too. Go Mac! > > > > > > > > > > > > > > > > > > On 2013/03/26 17:46:24, sail wrote: > > > > > > > > https://chromiumcodereview.**appspot.com/12952005/diff/** > > > > > > > > > > 4005/chrome/browser/profiles/**profile_impl.cc<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc> > > > > > > > >> File chrome/browser/profiles/**profile_impl.cc (right): > > > >> > > > > > > > > > > > > https://chromiumcodereview.**appspot.com/12952005/diff/** > > > > > > > > > > 4005/chrome/browser/profiles/**profile_impl.cc#newcode144<https://chromiumcodereview.appspot.com/12952005/diff/4005/chrome/browser/profiles/profile_impl.cc#newcode144> > > > > > > > >> chrome/browser/profiles/**profile_impl.cc:144: > > > >> > > > > COMPILE_ASSERT(sizeof(**ProfileImpl) > > > > > > > >> <= 744u, profile_impl_size_unexpected); > > > >> On 2013/03/26 17:33:26, msarda wrote: > > > >> > On 2013/03/26 17:25:02, sail wrote: > > > >> > > Will your patch break this? > > > >> > > > > >> > Ninja compiled the code on my machine so I suppose that compile asserts > > > >> are > > > >> > passing. Am I wrong? > > > >> > What else should I do to test if this breaks? > > > >> > > > > > > > > Maybe we don't support COMPILE_ASSERT on your platform. > > > >> To avoid hitting this assert you should remove the member variable you > > > >> added > > > >> > > > > to > > > > > > > >> ProfileImpl. > > > >> > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.**appspot.com/12952005/%25253Chttps://chromiumcode...> > > > >
I took the BookmarkTaskRunner and moved it to the BookmarkModel. It is the bookmark model, so it seems natural to have it in the bookmark code. I think this is better than having its own PKS. PTAL.
From the point of view of not increasing Profile size, this is much cleaner than the last implementation. I'm not entirely convinced by the argument that this method should not be a PKS at the end of the day (along with its brother, the IOTaskRunner), but I won't argue against this solution if reviewers more familiar with the bookmark model give it the ok. https://chromiumcodereview.appspot.com/12952005/diff/20001/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/20001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:65: bool started_; Perhaps I'm missing something -- I don't see where this bool is set? https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/browser/pro... File chrome/browser/profiles/profile_manager.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/browser/pro... chrome/browser/profiles/profile_manager.cc:767: model->GetBookmarkTaskRunner()->Start(); It seems wrong to expose this inner lazy constructor and let the outside world know about our inner workings; perhaps just give the BookmarkModel a StartLoad method, or something with a better name, that calls GetBookmarkTaskRunner privately to lazily construct the taskrunner? Then you could do a one-liner here: BookmarkModelFactory::GetForProfile(profile)->StartLoading(); https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/test/base/u... File chrome/test/base/ui_test_utils.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/test/base/u... chrome/test/base/ui_test_utils.cc:421: model->GetBookmarkTaskRunner()->Start(); See remark in profile_manager about changing to model->StartLoading();
https://chromiumcodereview.appspot.com/12952005/diff/20001/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/20001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:65: bool started_; On 2013/03/27 13:31:46, Miranda Callahan wrote: > Perhaps I'm missing something -- I don't see where this bool is set? Done. Thanks for catching this! https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/browser/pro... File chrome/browser/profiles/profile_manager.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/browser/pro... chrome/browser/profiles/profile_manager.cc:767: model->GetBookmarkTaskRunner()->Start(); On 2013/03/27 13:31:46, Miranda Callahan wrote: > It seems wrong to expose this inner lazy constructor and let the outside world > know about our inner workings; perhaps just give the BookmarkModel a StartLoad > method, or something with a better name, that calls GetBookmarkTaskRunner > privately to lazily construct the taskrunner? Then you could do a one-liner > here: > > BookmarkModelFactory::GetForProfile(profile)->StartLoading(); Done.
Thanks -- LGTM! Definitely wait for Erik's review, though -- I concentrated specifically on the Profile-oriented bits, as opposed to the task runner and bookmark model itself. On 2013/03/27 14:16:48, msarda wrote: > https://chromiumcodereview.appspot.com/12952005/diff/20001/base/deferred_sequ... > File base/deferred_sequenced_task_runner.h (right): > > https://chromiumcodereview.appspot.com/12952005/diff/20001/base/deferred_sequ... > base/deferred_sequenced_task_runner.h:65: bool started_; > On 2013/03/27 13:31:46, Miranda Callahan wrote: > > Perhaps I'm missing something -- I don't see where this bool is set? > > Done. > Thanks for catching this! > > https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/browser/pro... > File chrome/browser/profiles/profile_manager.cc (right): > > https://chromiumcodereview.appspot.com/12952005/diff/20001/chrome/browser/pro... > chrome/browser/profiles/profile_manager.cc:767: > model->GetBookmarkTaskRunner()->Start(); > On 2013/03/27 13:31:46, Miranda Callahan wrote: > > It seems wrong to expose this inner lazy constructor and let the outside world > > know about our inner workings; perhaps just give the BookmarkModel a StartLoad > > method, or something with a better name, that calls GetBookmarkTaskRunner > > privately to lazily construct the taskrunner? Then you could do a one-liner > > here: > > > > BookmarkModelFactory::GetForProfile(profile)->StartLoading(); > > Done.
Please, put Scott (sky) on loop when you are done addressing other reviewer's review. Thanks! -- Thiago
Also, update your CL description. It's DeferredSequencedTaskRunner rather than PausableSequencedtaskRunner now. -- Thiago
https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: Copyright 2013
+ Scott
Remind me why are we doing this?
On 2013/03/27 14:59:26, sky wrote: > Remind me why are we doing this? On iOS, we discovered that start-up is slowed down by the fact that bookmarks / history and other expensive services start loading their data at start-up. All this work is done on background threads, yet this interrupts the main thread. The idea is to delay this after the profile is loaded (on iOS we plan to delay it even more, until after the applications starts). I created a thread about this on chromium dev: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Start-... There was some discussion on the thread, on a downstream CL and over email. This lead to this CL.
What does 'profile is loaded' mean? On Wed, Mar 27, 2013 at 8:18 AM, <msarda@chromium.org> wrote: > On 2013/03/27 14:59:26, sky wrote: > >> Remind me why are we doing this? >> > > On iOS, we discovered that start-up is slowed down by the fact that > bookmarks / > history and other expensive services start loading their data at start-up. > All > this work is done on background threads, yet this interrupts the main > thread. > The idea is to delay this after the profile is loaded (on iOS we plan to > delay > it even more, until after the applications starts). > > I created a thread about this on chromium dev: > https://groups.google.com/a/**chromium.org/forum/#!searchin/** > chromium-dev/Start-up$**20performance/chromium-dev/** > zcLsAHooHps/S_TlFGr9v4EJ<https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Start-up$20performance/chromium-dev/zcLsAHooHps/S_TlFGr9v4EJ> > > There was some discussion on the thread, on a downstream CL and over > email. This > lead to this CL. > > > https://chromiumcodereview.**appspot.com/12952005/<https://chromiumcodereview... >
You mentioned in the thread that History creates its own thread for this. Have you tried this approach here? -- Thiago
On 2013/03/27 16:16:36, tfarina wrote: > You mentioned in the thread that History creates its own thread for > this. Have you tried this approach here? > > -- > Thiago Nope. It's better to use a task runner instead of a thread for different reasons (share a pool of threads for example).
Adding willchan for base.
Adding Jeffrey Yasskin as he had a lot of comments on the chromium dev thread.
Is there a way to make the bookmarkmodel not track any of this? By that I mean not having to keep around the deferredstatetaskrunner. Could the deferredstatetaskrunner be made an implementation detail of Profile? Perhaps the BookmarkModel could call into the PRofile to get the SequencedTaskRunner to use and the Profile could create and track it?
If you look at patch 3, that's exactly what I did. The problem is that we are no longer allowed to add fields to the ProfileImpl class. A potential solution would be to have a PSKFactory that creates a PSK that has a single field which is the BookmarksTaskRunner. I would name that something like ProfileTaskRunnersServiceFactory, ProfileTaskRunnersService with a method GetBookmarkstaskRunner. If I could do that if everybody agrees this is the right thing to do. On 2013/03/27 17:26:32, sky wrote: > Is there a way to make the bookmarkmodel not track any of this? By that I mean > not having to keep around the deferredstatetaskrunner. Could the > deferredstatetaskrunner be made an implementation detail of Profile? Perhaps the > BookmarkModel could call into the PRofile to get the SequencedTaskRunner to use > and the Profile could create and track it?
I'd like it to be a ProfileKeyedService instead of directly attached to the Profile if we do it this way -- see my comment on patchset 3. (And I agree that this would be cleaner than having it attached to the bookmarkmodel directly -- see comment #11. :-) On 2013/03/27 17:26:32, sky wrote: > Is there a way to make the bookmarkmodel not track any of this? By that I mean > not having to keep around the deferredstatetaskrunner. Could the > deferredstatetaskrunner be made an implementation detail of Profile? Perhaps the > BookmarkModel could call into the PRofile to get the SequencedTaskRunner to use > and the Profile could create and track it?
(Whoops, Mihai's comment overlapped mine. :-) On 2013/03/27 17:30:57, Miranda Callahan wrote: > I'd like it to be a ProfileKeyedService instead of directly attached to the > Profile if we do it this way -- see my comment on patchset 3. (And I agree that > this would be cleaner than having it attached to the bookmarkmodel directly -- > see comment #11. :-) > > On 2013/03/27 17:26:32, sky wrote: > > Is there a way to make the bookmarkmodel not track any of this? By that I mean > > not having to keep around the deferredstatetaskrunner. Could the > > deferredstatetaskrunner be made an implementation detail of Profile? Perhaps > the > > BookmarkModel could call into the PRofile to get the SequencedTaskRunner to > use > > and the Profile could create and track it?
+akalin jyasskin's in transit to Munich today. Is there a hurry for this CL? I'd love for jyasskin & akalin to chime in with their opinions on the new task runner if you don't mind waiting. On Wed, Mar 27, 2013 at 10:33 AM, <mirandac@chromium.org> wrote: > (Whoops, Mihai's comment overlapped mine. :-) > > > On 2013/03/27 17:30:57, Miranda Callahan wrote: > >> I'd like it to be a ProfileKeyedService instead of directly attached to >> the >> Profile if we do it this way -- see my comment on patchset 3. (And I agree >> > that > >> this would be cleaner than having it attached to the bookmarkmodel >> directly -- >> see comment #11. :-) >> > > On 2013/03/27 17:26:32, sky wrote: >> > Is there a way to make the bookmarkmodel not track any of this? By that >> I >> > mean > >> > not having to keep around the deferredstatetaskrunner. Could the >> > deferredstatetaskrunner be made an implementation detail of Profile? >> Perhaps >> the >> > BookmarkModel could call into the PRofile to get the >> SequencedTaskRunner to >> use >> > and the Profile could create and track it? >> > > > > https://chromiumcodereview.**appspot.com/12952005/<https://chromiumcodereview... >
Let's wait for their review and then I can move forward and move the TaskRunner in a PSK as discussed above. On 2013/03/27 17:36:24, willchan wrote: > +akalin > > jyasskin's in transit to Munich today. Is there a hurry for this CL? I'd > love for jyasskin & akalin to chime in with their opinions on the new task > runner if you don't mind waiting. > > > On Wed, Mar 27, 2013 at 10:33 AM, <mailto:mirandac@chromium.org> wrote: > > > (Whoops, Mihai's comment overlapped mine. :-) > > > > > > On 2013/03/27 17:30:57, Miranda Callahan wrote: > > > >> I'd like it to be a ProfileKeyedService instead of directly attached to > >> the > >> Profile if we do it this way -- see my comment on patchset 3. (And I agree > >> > > that > > > >> this would be cleaner than having it attached to the bookmarkmodel > >> directly -- > >> see comment #11. :-) > >> > > > > On 2013/03/27 17:26:32, sky wrote: > >> > Is there a way to make the bookmarkmodel not track any of this? By that > >> I > >> > > mean > > > >> > not having to keep around the deferredstatetaskrunner. Could the > >> > deferredstatetaskrunner be made an implementation detail of Profile? > >> Perhaps > >> the > >> > BookmarkModel could call into the PRofile to get the > >> SequencedTaskRunner to > >> use > >> > and the Profile could create and track it? > >> > > > > > > > > > https://chromiumcodereview.**appspot.com/12952005/%3Chttps://chromiumcoderevi...> > >
I'm only *mostly* dead^Win transit. +1 to putting this in a PKS. I'd probably call that the StartupTaskRunner instead of thinking of it as specific to bookmarks. +1 to the design overall. On 2013/03/27 17:30:57, Miranda Callahan wrote: > I'd like it to be a ProfileKeyedService instead of directly attached to the > Profile if we do it this way -- see my comment on patchset 3. (And I agree that > this would be cleaner than having it attached to the bookmarkmodel directly -- > see comment #11. :-) > > On 2013/03/27 17:26:32, sky wrote: > > Is there a way to make the bookmarkmodel not track any of this? By that I mean > > not having to keep around the deferredstatetaskrunner. Could the > > deferredstatetaskrunner be made an implementation detail of Profile? Perhaps > the > > BookmarkModel could call into the PRofile to get the SequencedTaskRunner to > use > > and the Profile could create and track it? https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:81: target_task_runner_->PostNonNestableDelayedTask(task.posted_from, Calling user-controlled functions while |lock_| is held risks deadlock, although I expect that there will be a natural order of the relevant locks that'll prevent it in this case. It would be safer to have this function move (swap+clear) the deferred_tasks_queue_ into a local variable under the lock, and then release the lock and iterate that variable into the target_task_runner_. You'd loop to do this dump possibly several times until you see the deferred_tasks_queue_ empty. Only then would you set started_ to true to let other Posters take their fast path. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:23: class BASE_EXPORT DeferredSequencedTaskRunner : public SequencedTaskRunner { Please write a test. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:54: TimeDelta delay; Do you want the delay to be measured from Start() or PostDelayedTask()? I think either would be reasonable, but you should document the choice. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:59: virtual void QueueDeferredTask(const tracked_objects::Location& from_here, Does this need to be virtual? I don't like encouraging implementation inheritance. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:64: // True if this DeferredSequencedTaskRunner was already started. s/was already/has been/, I think. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:70: // Queue if task to be posted to |target_task_runner_| when this s/if/of/?? s/task/tasks/?
https://chromiumcodereview.appspot.com/12952005/diff/20005/chrome/browser/boo... File chrome/browser/bookmarks/bookmark_model.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.cc:974: new base::DeferredSequencedTaskRunner(profile_->GetIOTaskRunner()); Can we do something similar to |blocking_task_runner_| of chrome/browser/chromeos/drive/drive_system_service.cc?
https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:81: target_task_runner_->PostNonNestableDelayedTask(task.posted_from, With this design, can we guarantee that if Start is called twice from different threads at the same time, the tasks are posted in sequence? Or we don't care about this case? On 2013/03/27 18:07:37, Jeffrey Yasskin wrote: > Calling user-controlled functions while |lock_| is held risks deadlock, although > I expect that there will be a natural order of the relevant locks that'll > prevent it in this case. > > It would be safer to have this function move (swap+clear) the > deferred_tasks_queue_ into a local variable under the lock, and then release the > lock and iterate that variable into the target_task_runner_. You'd loop to do > this dump possibly several times until you see the deferred_tasks_queue_ empty. > Only then would you set started_ to true to let other Posters take their fast > path. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/03/27 14:35:11, tfarina wrote: > nit: Copyright 2013 Done. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:54: TimeDelta delay; On 2013/03/27 18:07:37, Jeffrey Yasskin wrote: > Do you want the delay to be measured from Start() or PostDelayedTask()? I think > either would be reasonable, but you should document the choice. Done. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:59: virtual void QueueDeferredTask(const tracked_objects::Location& from_here, On 2013/03/27 18:07:37, Jeffrey Yasskin wrote: > Does this need to be virtual? I don't like encouraging implementation > inheritance. Done. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:64: // True if this DeferredSequencedTaskRunner was already started. On 2013/03/27 18:07:37, Jeffrey Yasskin wrote: > s/was already/has been/, I think. Done. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.h:70: // Queue if task to be posted to |target_task_runner_| when this On 2013/03/27 18:07:37, Jeffrey Yasskin wrote: > s/if/of/?? s/task/tasks/? Done. https://chromiumcodereview.appspot.com/12952005/diff/20005/chrome/browser/boo... File chrome/browser/bookmarks/bookmark_model.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.cc:974: new base::DeferredSequencedTaskRunner(profile_->GetIOTaskRunner()); I think so. We should probably have a similar start-up task runner for the drive_system_service.cc. This will be done in a subsequent CL (this is already starting to be toooo long). On 2013/03/28 22:57:01, tfarina wrote: > Can we do something similar to |blocking_task_runner_| of > chrome/browser/chromeos/drive/drive_system_service.cc? https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... File chrome/browser/bookmarks/DEPS (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... chrome/browser/bookmarks/DEPS:26: "!chrome/browser/profiles/startup_task_runner_service.h", This is bad (I saw the comments above and below). However, I think this is the right solution for now and matches the existing factories. In my opinion: * The right place for startup_task_runner_service.h is in chrome/browser/profiles/ (where I put it) * All the factories should probably live in chrome/browser/profiles/ to avoid all this include mess. Let me know if you have a different solution here.
Thanks for all your work on this CL!! Getting very clean -- just one real comment from my PoV. https://chromiumcodereview.appspot.com/12952005/diff/29001/chrome/browser/boo... File chrome/browser/bookmarks/bookmark_model.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/29001/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.cc:236: void BookmarkModel::Load(scoped_refptr<base::SequencedTaskRunner> task_runner) { Do we really need to pass the task_runner here? Now that the task_runner is a PKS, we can just get it directly anywhere we know what the Profile is. Since you already have the Profile information here, you can skip passing in the task_runner, and just pass StartupTaskRunnerServiceFactory::GetForProfile(profile)->GetBookmarkTaskRunner() below. https://chromiumcodereview.appspot.com/12952005/diff/29001/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.cc:253: store_ = new BookmarkStorage(profile_, this, task_runner); see comment on "Load" above. https://chromiumcodereview.appspot.com/12952005/diff/29001/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service_factory.h (right): https://chromiumcodereview.appspot.com/12952005/diff/29001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service_factory.h:11: class StartupTaskRunnerService; alpha-nit
Miranda: Thank you for you quick review! I'll wait for sky's review before addressing your change, just to be sure I get it right the next time :) https://chromiumcodereview.appspot.com/12952005/diff/29001/chrome/browser/boo... File chrome/browser/bookmarks/bookmark_model.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/29001/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.cc:236: void BookmarkModel::Load(scoped_refptr<base::SequencedTaskRunner> task_runner) { I did that in a local change. However, I understood from the other files that we want the bookmarks to be a component and have less dependency on chrome/browser/.. code. I used this approach for that reason and for unit tests. @sky: Let me know what is the approach you guys prefer here and I'll do the change accordingly. On 2013/03/29 14:40:13, Miranda Callahan wrote: > Do we really need to pass the task_runner here? Now that the task_runner is a > PKS, we can just get it directly anywhere we know what the Profile is. Since you > already have the Profile information here, you can skip passing in the > task_runner, and just pass > > StartupTaskRunnerServiceFactory::GetForProfile(profile)->GetBookmarkTaskRunner() > > below.
https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... File chrome/browser/bookmarks/DEPS (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... chrome/browser/bookmarks/DEPS:7: "+chrome/browser/api", looks like you need to sync your checkout. I have already removed this entry. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... chrome/browser/bookmarks/DEPS:26: "!chrome/browser/profiles/startup_task_runner_service.h", The decision is from Scott, so I'll let him decide if he will be fine in adding yet another 'dependency'. But I wish you could try hard to avoid adding another one here!
I like the injection route, less dependencies. bookmarks (and chrom/test and chrome/chrome_browser.gypi changes) LGTM
On 2013/03/29 15:41:25, sky wrote: > I like the injection route, less dependencies. bookmarks (and chrom/test and > chrome/chrome_browser.gypi changes) LGTM Miranda, Yasskin: PTAL and let me know if this looks good to you as well.
LGTM! On 2013/03/29 15:59:22, msarda wrote: > On 2013/03/29 15:41:25, sky wrote: > > I like the injection route, less dependencies. bookmarks (and chrom/test and > > chrome/chrome_browser.gypi changes) LGTM > > Miranda, Yasskin: PTAL and let me know if this looks good to you as well.
jyasskin, willchan: Ping
Once the owners are happy, you don't need to wait for an l g t m from me. https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/20005/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:81: target_task_runner_->PostNonNestableDelayedTask(task.posted_from, > On 2013/03/27 18:07:37, Jeffrey Yasskin wrote: > > Calling user-controlled functions while |lock_| is held risks deadlock, > although > > I expect that there will be a natural order of the relevant locks that'll > > prevent it in this case. > > > > It would be safer to have this function move (swap+clear) the > > deferred_tasks_queue_ into a local variable under the lock, and then release > the > > lock and iterate that variable into the target_task_runner_. You'd loop to do > > this dump possibly several times until you see the deferred_tasks_queue_ > empty. > > Only then would you set started_ to true to let other Posters take their fast > > path. On 2013/03/29 14:40:03, msarda wrote: > With this design, can we guarantee that if Start is called twice from different > threads at the same time, the tasks are posted in sequence? Or we don't care > about this case? Good catch. You can guarantee that if you use a condition variable to have the second Start() wait for the first to finish. I suspect Start() will only be called from the UI thread though, in which case it can't be called twice concurrently. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner_unittest.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:21: class DeferredSequencedTaskRunnerTest : public testing::Test { You should probably include a test that adds elements from multiple threads, and calls Start() during that set of PostTask()s, so that tsan can catch any races. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:36: std::vector<int> executed_tasks_ids_; s/tasks/task/? https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:57: EXPECT_EQ(MakeVector<1>({1}), executed_tasks_ids_); Try including gmock.h and then using EXPECT_THAT(executed_tasks_ids_, testing::ElementsAre(1)); (https://code.google.com/p/googlemock/wiki/CheatSheet#Container_Matchers) https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:60: TEST_F(DeferredSequencedTaskRunnerTest, StartWithMultipleElements) { You probably don't need to test single and multiple elements separately. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... File chrome/browser/bookmarks/bookmark_model.h (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.h:240: void Load(scoped_refptr<base::SequencedTaskRunner> task_runner); This should generally be a "const scoped_refptr<base::SequencedTaskRunner>&" to save an atomic_increment pair. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.cc:18: if (!bookmark_task_runner_) { Are startup tasks only enqueued onto this task runner from one thread? If not, this isn't thread-safe. If so, please comment the constraint in the header. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.h (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:16: class StartupTaskRunnerService : public ProfileKeyedService { There might be a simpler interface here. What if, instead of returning a literal TaskRunner to post tasks on, you instead had a PostStartupTask method on this class, which took both the TaskRunner to post to, and the arguments that PostTask would normally take? (This class would then be a more metaphorical TaskRunner.) Then the bookmarks service could specify the IOTaskRunner, and other startup tasks could specify the appropriate task runners for themselves, without having to store long-lived objects here or centralize the knowledge of which TaskRunner is appropriate for each service. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:23: scoped_refptr<base::DeferredSequencedTaskRunner> GetBookmarkTaskRunner(); Please include a comment about why different services need separate startup task runners, so that a future maintainer doesn't incorrectly simplify it down to a single runner.
https://codereview.chromium.org/12952005/diff/34002/chrome/browser/profiles/s... File chrome/browser/profiles/startup_task_runner_service.h (right): https://codereview.chromium.org/12952005/diff/34002/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.h:16: class StartupTaskRunnerService : public ProfileKeyedService { On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > There might be a simpler interface here. What if, instead of returning a literal > TaskRunner to post tasks on, you instead had a PostStartupTask method on this > class, which took both the TaskRunner to post to, and the arguments that > PostTask would normally take? (This class would then be a more metaphorical > TaskRunner.) Then the bookmarks service could specify the IOTaskRunner, and > other startup tasks could specify the appropriate task runners for themselves, > without having to store long-lived objects here or centralize the knowledge of > which TaskRunner is appropriate for each service. DelayedTaskQueue at https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex... might argue against my point, or it might not because it's significantly simpler than the DeferredSequencedTaskRunner.
My bad, I have been out most of the week. I'll try to do this tonight. On Mar 26, 2013 10:06 AM, <msarda@chromium.org> wrote: > Reviewers: erikwright, mirandac, > > Message: > Please take a look. > > Description: > Delay bookmarks load while the profile is loading. > > This CL adds a new PausableSequencedtaskRunner that can pause and resume > the execution of its tasks and creates such a task runner for the > execution of bookmarks I/O operations. At profile creation, the bookmarks > task runner is paused and its execution is resumed once the profile has > finished loading. > > BUG=NONE > > > Please review this at https://chromiumcodereview.**appspot.com/12952005/<https://chromiumcodereview... > > SVN Base: https://chromium.googlesource.**com/chromium/src.git@master<https://chromium.... > > Affected files: > M base/base.gypi > A base/deferred_sequenced_task_**runner.h > A base/deferred_sequenced_task_**runner.cc > M chrome/browser/automation/**testing_automation_provider.cc > M chrome/browser/bookmarks/**bookmark_model.cc > M chrome/browser/profiles/off_**the_record_profile_impl.h > M chrome/browser/profiles/off_**the_record_profile_impl.cc > M chrome/browser/profiles/**profile.h > M chrome/browser/profiles/**profile_impl.h > M chrome/browser/profiles/**profile_impl.cc > M chrome/browser/profiles/**profile_manager.cc > M chrome/test/base/testing_**profile.h > M chrome/test/base/testing_**profile.cc > > >
https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:77: for (std::vector<DeferredTask>::iterator i = deferred_tasks_queue_.begin(); There's a really subtle issue here that I'd like to draw your attention to so you can evaluate if it's an issue. I believe it is. You aren't freeing tasks immediately. I think it's a not-uncommon assumption that when you PostTask() to a TaskRunner and don't keep the task around elsewhere in your code, that after that any arguments bound to that task are deleted. Note here that you keep the tasks around longer, which prevents them from getting deleted, since they're internally refcounted. Is that desired? I suspect not. If I'm correct and we should preserve the expectation that we do not artificially extend task lifetime, then please add a test that asserts this. Please upload a patchset with the new test with a tryjob demonstrating its failure. Then upload a patchset with the fix that shows that the new test got fixed. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:14: #include "base/memory/scoped_ptr.h" Why? https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:22: // queues up all requests until the first call to Start is issued. Start() to indicate it's a member function. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:45: // Ignores the call if this task runner is already started. This is all personal style preference, and despite what some people may say there is no consensus in the overall project, but I personally disallowing double calls. I think it's often benignly sloppy, but sometimes is indicative of a bug. I personally err on the side of being a bit annoying and requiring extra code to make sure people don't call this twice, in order to assure ourselves that different subsystems using this shared DeferredSequencedTaskRunner properly cooperate to make sure that it is not started until both subsystems are ready. I leave it up to you, but I wanted to state my preference for you to consider in case you had not considered yet. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:51: struct DeferredTask { Types go before destructors. Move this up. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:77: mutable Lock lock_; Move this to be first. It guards all the other member variables, thus it should outlive all the others. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner_unittest.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:22: public: There's no need for this to be public. GTest will subclass this fixture and call the public constructor for the subclass. You should probably move this to the protected section. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:31: virtual void SetUp() { Since you have a constructor, why not move this into the constructor too? https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:69: EXPECT_EQ(MakeVector<4>({1, 2,3, 4}), executed_tasks_ids_); looks like a formatting typo
jyasskin, willchan: I addressed all your comments. Please take another look. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:77: for (std::vector<DeferredTask>::iterator i = deferred_tasks_queue_.begin(); On 2013/04/05 02:56:41, willchan wrote: > There's a really subtle issue here that I'd like to draw your attention to so > you can evaluate if it's an issue. I believe it is. > > You aren't freeing tasks immediately. I think it's a not-uncommon assumption > that when you PostTask() to a TaskRunner and don't keep the task around > elsewhere in your code, that after that any arguments bound to that task are > deleted. > > Note here that you keep the tasks around longer, which prevents them from > getting deleted, since they're internally refcounted. Is that desired? I suspect > not. > > If I'm correct and we should preserve the expectation that we do not > artificially extend task lifetime, then please add a test that asserts this. > Please upload a patchset with the new test with a tryjob demonstrating its > failure. Then upload a patchset with the fix that shows that the new test got > fixed. I just want to be sure I understand your comment as it may refer to the following cases: 1. I should remove the tasks from deferred_tasks_queue_ as soon as they are posted. 2. The DeferredSequencedTaskRunner is wrong entirely as it keeps taks in a queue until Start() is called, having as a consequence the fact that they live longer. I have to keep the tasks around for a longer period of time as I need to run them later. I don't really see how I can do otherwise. In any case, during start-up there is much contention (at least on mobile), so the tasks should expect to live longer, right? So, what case are you actually talking about? If 1, then I'll try to put up the test. If 2, then this CL has no meaning anymore as the tasks need to live until they are posted, right? https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:14: #include "base/memory/scoped_ptr.h" On 2013/04/05 02:56:41, willchan wrote: > Why? Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:22: // queues up all requests until the first call to Start is issued. On 2013/04/05 02:56:41, willchan wrote: > Start() to indicate it's a member function. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:45: // Ignores the call if this task runner is already started. On 2013/04/05 02:56:41, willchan wrote: > This is all personal style preference, and despite what some people may say > there is no consensus in the overall project, but I personally disallowing > double calls. I think it's often benignly sloppy, but sometimes is indicative of > a bug. I personally err on the side of being a bit annoying and requiring extra > code to make sure people don't call this twice, in order to assure ourselves > that different subsystems using this shared DeferredSequencedTaskRunner properly > cooperate to make sure that it is not started until both subsystems are ready. > > I leave it up to you, but I wanted to state my preference for you to consider in > case you had not considered yet. I did consider it, but I can go either way. I changed its behavior. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:51: struct DeferredTask { On 2013/04/05 02:56:41, willchan wrote: > Types go before destructors. Move this up. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.h:77: mutable Lock lock_; On 2013/04/05 02:56:41, willchan wrote: > Move this to be first. It guards all the other member variables, thus it should > outlive all the others. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner_unittest.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:21: class DeferredSequencedTaskRunnerTest : public testing::Test { On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > You should probably include a test that adds elements from multiple threads, and > calls Start() during that set of PostTask()s, so that tsan can catch any races. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:22: public: On 2013/04/05 02:56:41, willchan wrote: > There's no need for this to be public. GTest will subclass this fixture and call > the public constructor for the subclass. You should probably move this to the > protected section. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:31: virtual void SetUp() { On 2013/04/05 02:56:41, willchan wrote: > Since you have a constructor, why not move this into the constructor too? Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:36: std::vector<int> executed_tasks_ids_; On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > s/tasks/task/? Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:57: EXPECT_EQ(MakeVector<1>({1}), executed_tasks_ids_); On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > Try including gmock.h and then using > > EXPECT_THAT(executed_tasks_ids_, testing::ElementsAre(1)); > > (https://code.google.com/p/googlemock/wiki/CheatSheet#Container_Matchers) Thanks for the tip !!! https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:60: TEST_F(DeferredSequencedTaskRunnerTest, StartWithMultipleElements) { On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > You probably don't need to test single and multiple elements separately. I'll keep them for now. Let me know if you strongly feel I should remove them. https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:69: EXPECT_EQ(MakeVector<4>({1, 2,3, 4}), executed_tasks_ids_); On 2013/04/05 02:56:41, willchan wrote: > looks like a formatting typo Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... File chrome/browser/bookmarks/bookmark_model.h (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.h:240: void Load(scoped_refptr<base::SequencedTaskRunner> task_runner); On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > This should generally be a "const scoped_refptr<base::SequencedTaskRunner>&" to > save an atomic_increment pair. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.cc:18: if (!bookmark_task_runner_) { On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > Are startup tasks only enqueued onto this task runner from one thread? If not, > this isn't thread-safe. If so, please comment the constraint in the header. Done. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.h (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:16: class StartupTaskRunnerService : public ProfileKeyedService { On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > There might be a simpler interface here. What if, instead of returning a literal > TaskRunner to post tasks on, you instead had a PostStartupTask method on this > class, which took both the TaskRunner to post to, and the arguments that > PostTask would normally take? (This class would then be a more metaphorical > TaskRunner.) Then the bookmarks service could specify the IOTaskRunner, and > other startup tasks could specify the appropriate task runners for themselves, > without having to store long-lived objects here or centralize the knowledge of > which TaskRunner is appropriate for each service. I prefer the current interface as it avoids changing the other services (bookmarks in my case). https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:23: scoped_refptr<base::DeferredSequencedTaskRunner> GetBookmarkTaskRunner(); On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > Please include a comment about why different services need separate startup task > runners, so that a future maintainer doesn't incorrectly simplify it down to a > single runner. Done.
https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:77: for (std::vector<DeferredTask>::iterator i = deferred_tasks_queue_.begin(); On 2013/04/08 17:37:18, msarda wrote: > On 2013/04/05 02:56:41, willchan wrote: > > There's a really subtle issue here that I'd like to draw your attention to so > > you can evaluate if it's an issue. I believe it is. > > > > You aren't freeing tasks immediately. I think it's a not-uncommon assumption > > that when you PostTask() to a TaskRunner and don't keep the task around > > elsewhere in your code, that after that any arguments bound to that task are > > deleted. > > > > Note here that you keep the tasks around longer, which prevents them from > > getting deleted, since they're internally refcounted. Is that desired? I > suspect > > not. > > > > If I'm correct and we should preserve the expectation that we do not > > artificially extend task lifetime, then please add a test that asserts this. > > Please upload a patchset with the new test with a tryjob demonstrating its > > failure. Then upload a patchset with the fix that shows that the new test got > > fixed. > > I just want to be sure I understand your comment as it may refer to the > following cases: > > 1. I should remove the tasks from deferred_tasks_queue_ as soon as they are > posted. > > 2. The DeferredSequencedTaskRunner is wrong entirely as it keeps taks in a queue > until Start() is called, having as a consequence the fact that they live longer. > I have to keep the tasks around for a longer period of time as I need to run > them later. I don't really see how I can do otherwise. In any case, during > start-up there is much contention (at least on mobile), so the tasks should > expect to live longer, right? > > > So, what case are you actually talking about? If 1, then I'll try to put up the > test. If 2, then this CL has no meaning anymore as the tasks need to live until > they are posted, right? (1) is what I was talking about.
lgtm. My comments are all nits. https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.h (right): https://chromiumcodereview.appspot.com/12952005/diff/34002/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:16: class StartupTaskRunnerService : public ProfileKeyedService { On 2013/04/08 17:37:18, msarda wrote: > On 2013/04/03 16:25:35, Jeffrey Yasskin wrote: > > There might be a simpler interface here. What if, instead of returning a > literal > > TaskRunner to post tasks on, you instead had a PostStartupTask method on this > > class, which took both the TaskRunner to post to, and the arguments that > > PostTask would normally take? (This class would then be a more metaphorical > > TaskRunner.) Then the bookmarks service could specify the IOTaskRunner, and > > other startup tasks could specify the appropriate task runners for themselves, > > without having to store long-lived objects here or centralize the knowledge of > > which TaskRunner is appropriate for each service. > > I prefer the current interface as it avoids changing the other services > (bookmarks in my case). Sure. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... File base/deferred_sequenced_task_runner_unittest.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:20: public : No space between "public" and ":". https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:27: DeferredSequencedTaskRunnerTest() : loop_(), I believe the style guide says to line up "loop_" and "runner_", which probably implies putting ": loop_()," on the next line. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:44: EXPECT_TRUE(executed_task_ids_.empty()) << executed_task_ids_[0]; You can use "EXPECT_THAT(executed_task_ids_, testing::ElementsAre())" to assert emptiness in a way that'll print the elements if it fails. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:108: EXPECT_THAT(executed_task_ids_, testing::ElementsAre(1, 2,3, 4, 5, 6, 7, 8)); Missing space between 2 and 3. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:127: runner_->Start(); Post this to one of the threads too, or it's likely to run before either of the threads has actually started picking up work. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:133: testing::WhenSorted(testing::ElementsAre(0, 1, 2,3, 4, 5, 6, 7, 8, 9))); Missing space between 2 and 3.
https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... File base/deferred_sequenced_task_runner_unittest.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:20: public : On 2013/04/08 19:58:30, Jeffrey Yasskin wrote: > No space between "public" and ":". Done. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:27: DeferredSequencedTaskRunnerTest() : loop_(), On 2013/04/08 19:58:30, Jeffrey Yasskin wrote: > I believe the style guide says to line up "loop_" and "runner_", which probably > implies putting ": loop_()," on the next line. Done. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:44: EXPECT_TRUE(executed_task_ids_.empty()) << executed_task_ids_[0]; On 2013/04/08 19:58:30, Jeffrey Yasskin wrote: > You can use "EXPECT_THAT(executed_task_ids_, testing::ElementsAre())" to assert > emptiness in a way that'll print the elements if it fails. Done. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:108: EXPECT_THAT(executed_task_ids_, testing::ElementsAre(1, 2,3, 4, 5, 6, 7, 8)); On 2013/04/08 19:58:30, Jeffrey Yasskin wrote: > Missing space between 2 and 3. Done. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:127: runner_->Start(); On 2013/04/08 19:58:30, Jeffrey Yasskin wrote: > Post this to one of the threads too, or it's likely to run before either of the > threads has actually started picking up work. Done. https://chromiumcodereview.appspot.com/12952005/diff/54001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:133: testing::WhenSorted(testing::ElementsAre(0, 1, 2,3, 4, 5, 6, 7, 8, 9))); On 2013/04/08 19:58:30, Jeffrey Yasskin wrote: > Missing space between 2 and 3. Done.
I have tried to put up a unit test for the destruction time of the objects as you explained in a previous review (for 2 hours this morning) . I have used a SimpleThread that post objects and the main run loop to execute them. With the previous patch is always green as start function is too fast - the tasks are posted faster than they start executing and the deferred_tasks_queue_ is cleared. I tried passing a mock SequenceTaskRunner to delay the task execution, but no success there either. At this point the only solution I see is to add extra code to the deferred_tasks_queue_ (like a timeout between tasks or some kind of observer), but this really seems fragile and I would prefer avoiding this. Do you think this unit test is blocking for this CL? If so, then maybe you have a suggestion on how it should be done (I would like to avoid test code in the DeferredSequencedTaskRunner). On 2013/04/08 17:41:09, willchan wrote: > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > File base/deferred_sequenced_task_runner.cc (right): > > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > base/deferred_sequenced_task_runner.cc:77: for > (std::vector<DeferredTask>::iterator i = deferred_tasks_queue_.begin(); > On 2013/04/08 17:37:18, msarda wrote: > > On 2013/04/05 02:56:41, willchan wrote: > > > There's a really subtle issue here that I'd like to draw your attention to > so > > > you can evaluate if it's an issue. I believe it is. > > > > > > You aren't freeing tasks immediately. I think it's a not-uncommon assumption > > > that when you PostTask() to a TaskRunner and don't keep the task around > > > elsewhere in your code, that after that any arguments bound to that task are > > > deleted. > > > > > > Note here that you keep the tasks around longer, which prevents them from > > > getting deleted, since they're internally refcounted. Is that desired? I > > suspect > > > not. > > > > > > If I'm correct and we should preserve the expectation that we do not > > > artificially extend task lifetime, then please add a test that asserts this. > > > Please upload a patchset with the new test with a tryjob demonstrating its > > > failure. Then upload a patchset with the fix that shows that the new test > got > > > fixed. > > > > I just want to be sure I understand your comment as it may refer to the > > following cases: > > > > 1. I should remove the tasks from deferred_tasks_queue_ as soon as they are > > posted. > > > > 2. The DeferredSequencedTaskRunner is wrong entirely as it keeps taks in a > queue > > until Start() is called, having as a consequence the fact that they live > longer. > > I have to keep the tasks around for a longer period of time as I need to run > > them later. I don't really see how I can do otherwise. In any case, during > > start-up there is much contention (at least on mobile), so the tasks should > > expect to live longer, right? > > > > > > So, what case are you actually talking about? If 1, then I'll try to put up > the > > test. If 2, then this CL has no meaning anymore as the tasks need to live > until > > they are posted, right? > > (1) is what I was talking about.
I'm sorry I caused you to spend 2 hours on this :( I was hoping it'd be simple to do. I thought about it and agree with you that it's difficult/impossible to do this without surgery on the production code. I think you should be able to write a test that doesn't necessarily fail on the previous code, but should help catch races when they flakily happen. Let me know if you have that done and I'll review it.
willchan: The unit test is ready. PTAL. On 2013/04/09 18:07:24, willchan wrote: > I'm sorry I caused you to spend 2 hours on this :( I was hoping it'd be simple > to do. I thought about it and agree with you that it's difficult/impossible to > do this without surgery on the production code. I think you should be able to > write a test that doesn't necessarily fail on the previous code, but should help > catch races when they flakily happen. Let me know if you have that done and I'll > review it.
https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... File base/deferred_sequenced_task_runner.cc (right): https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.cc:8: #include "base/threading/thread_checker.h" not needed? https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.cc:31: AutoLock lock(lock_); presumably the same header that has Lock also has AutoLock. Otherwise you need to add an #include for this too. https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.cc:33: DCHECK(deferred_tasks_queue_.empty()); logging.h IIRC https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.cc:52: return target_task_runner_->PostDelayedTask(from_here, task, delay); PostNonNestableDelayedTask? https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.cc:74: DCHECK(!started_); I don't really see any reason to DCHECK in this scenario. Since it could still be called in production I'd rather have it not DCHECK and see a test that verifies that it behaves sensibly (i.e., a no-op). https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... File base/deferred_sequenced_task_runner.h (right): https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:30: TimeDelta delay) OVERRIDE; compiler_specific.h for OVERRIDE https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:31: remove this blank line https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:40: // Start the execution - posts all tasks in the |deferred_tasks_queue_| to "posts all queued tasks to the target executor." rather than referring to internal implementation details. https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:44: // Fails when called a second time. Ignores second and further calls. ? https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:50: ~DeferredTask(); blank line after destructor? https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:66: mutable Lock lock_; Add a comment indicating what members are protected by this lock. https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:66: mutable Lock lock_; missing an #include for Lock https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner.h:68: // True if this DeferredSequencedTaskRunner has already been started. I'm mildly opposed to the presence of these member variable comments. I think they are all self-evident. https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... File base/deferred_sequenced_task_runner_unittest.cc (right): https://codereview.chromium.org/12952005/diff/82001/base/deferred_sequenced_t... base/deferred_sequenced_task_runner_unittest.cc:75: base::Bind(&DeferredSequencedTaskRunnerTest::ExecuteTask, I'd consider wrapping the bind into a member method like Closure CreateTask(int task_id); https://codereview.chromium.org/12952005/diff/82001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_model.h (right): https://codereview.chromium.org/12952005/diff/82001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_model.h:16: #include "base/sequenced_task_runner.h" forward-decl ought to do it here? https://codereview.chromium.org/12952005/diff/82001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_model.h:240: void Load(const scoped_refptr<base::SequencedTaskRunner>& task_runner); need some documentation for this parameter https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... File chrome/browser/profiles/startup_task_runner_service.cc (right): https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.cc:22: new base::DeferredSequencedTaskRunner(profile_->GetIOTaskRunner()); Why not eliminate a dependency on profile.h by taking a STR in the constructor and storing that instead of Profile*? It's reasonable for the factory to do the accessing for you. https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... File chrome/browser/profiles/startup_task_runner_service.h (right): https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.h:8: #include "base/deferred_sequenced_task_runner.h" forward-decl should be sufficient? https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.h:9: #include "base/memory/scoped_ptr.h" not required? https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.h:14: class ProfileDownloader; Not required? https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.h:30: scoped_refptr<base::DeferredSequencedTaskRunner> GetBookmarkTaskRunner(); include ref_counted.h https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.h:39: DISALLOW_COPY_AND_ASSIGN(StartupTaskRunnerService); basic_types.h
Nice way of testing the new functionality. Please address erikwright's comments too and I'll send approval for base/. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner_unittest.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:153: thread1.message_loop()->PostTask(FROM_HERE, Indentation is off here. All the parameters should be aligned. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:186: runner_->PostTask(FROM_HERE, Ditto here on parameter alignment.
I addressed your comments. willchan: PTAL PS: There are a lot of other diffs with Patch 13 as I also synched up the code. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:8: #include "base/threading/thread_checker.h" On 2013/04/15 17:56:25, erikwright wrote: > not needed? Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:31: AutoLock lock(lock_); On 2013/04/15 17:56:25, erikwright wrote: > presumably the same header that has Lock also has AutoLock. Otherwise you need > to add an #include for this too. base/synchronization/lock.h has both Lock and AutoLock https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:33: DCHECK(deferred_tasks_queue_.empty()); On 2013/04/15 17:56:25, erikwright wrote: > logging.h IIRC Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:52: return target_task_runner_->PostDelayedTask(from_here, task, delay); On 2013/04/15 17:56:25, erikwright wrote: > PostNonNestableDelayedTask? Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:74: DCHECK(!started_); On 2013/04/15 17:56:25, erikwright wrote: > I don't really see any reason to DCHECK in this scenario. Since it could still > be called in production I'd rather have it not DCHECK and see a test that > verifies that it behaves sensibly (i.e., a no-op). Start is a one-time method as per previous review from willchan. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:30: TimeDelta delay) OVERRIDE; On 2013/04/15 17:56:25, erikwright wrote: > compiler_specific.h for OVERRIDE Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:31: On 2013/04/15 17:56:25, erikwright wrote: > remove this blank line Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:40: // Start the execution - posts all tasks in the |deferred_tasks_queue_| to On 2013/04/15 17:56:25, erikwright wrote: > "posts all queued tasks to the target executor." rather than referring to > internal implementation details. The comment on what delay means was added following jyasskin's review. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:44: // Fails when called a second time. On 2013/04/15 17:56:25, erikwright wrote: > Ignores second and further calls. ? That was the initial implementation. I changed it to fail on subsequent calls after willchan's review. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:50: ~DeferredTask(); On 2013/04/15 17:56:25, erikwright wrote: > blank line after destructor? Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:66: mutable Lock lock_; On 2013/04/15 17:56:25, erikwright wrote: > Add a comment indicating what members are protected by this lock. Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:66: mutable Lock lock_; On 2013/04/15 17:56:25, erikwright wrote: > missing an #include for Lock Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:68: // True if this DeferredSequencedTaskRunner has already been started. On 2013/04/15 17:56:25, erikwright wrote: > I'm mildly opposed to the presence of these member variable comments. I think > they are all self-evident. I removed them. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner_unittest.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:153: thread1.message_loop()->PostTask(FROM_HERE, On 2013/04/16 01:03:04, willchan wrote: > Indentation is off here. All the parameters should be aligned. Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner_unittest.cc:186: runner_->PostTask(FROM_HERE, On 2013/04/16 01:03:04, willchan wrote: > Ditto here on parameter alignment. Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/boo... File chrome/browser/bookmarks/bookmark_model.h (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.h:16: #include "base/sequenced_task_runner.h" On 2013/04/15 17:56:25, erikwright wrote: > forward-decl ought to do it here? Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/boo... chrome/browser/bookmarks/bookmark_model.h:240: void Load(const scoped_refptr<base::SequencedTaskRunner>& task_runner); On 2013/04/15 17:56:25, erikwright wrote: > need some documentation for this parameter Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.cc:22: new base::DeferredSequencedTaskRunner(profile_->GetIOTaskRunner()); On 2013/04/15 17:56:25, erikwright wrote: > Why not eliminate a dependency on profile.h by taking a STR in the constructor > and storing that instead of Profile*? It's reasonable for the factory to do the > accessing for you. I don't see the fact that this service holds profile as an inconvenient - it is a profile service at the end. We could image that in the future other task runners might be added and they may need the profile. Thus I'll keep it as is for now as mirandac (owner for chrome/browser/profiles) already l g t m -ed this CL. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.h (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:8: #include "base/deferred_sequenced_task_runner.h" On 2013/04/15 17:56:25, erikwright wrote: > forward-decl should be sufficient? Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:9: #include "base/memory/scoped_ptr.h" On 2013/04/15 17:56:25, erikwright wrote: > not required? Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:14: class ProfileDownloader; On 2013/04/15 17:56:25, erikwright wrote: > Not required? Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:30: scoped_refptr<base::DeferredSequencedTaskRunner> GetBookmarkTaskRunner(); On 2013/04/15 17:56:25, erikwright wrote: > include ref_counted.h Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:39: DISALLOW_COPY_AND_ASSIGN(StartupTaskRunnerService); On 2013/04/15 17:56:25, erikwright wrote: > basic_types.h Done.
https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:74: DCHECK(!started_); On 2013/04/17 09:53:55, msarda wrote: > On 2013/04/15 17:56:25, erikwright wrote: > > I don't really see any reason to DCHECK in this scenario. Since it could still > > be called in production I'd rather have it not DCHECK and see a test that > > verifies that it behaves sensibly (i.e., a no-op). > > Start is a one-time method as per previous review from willchan. I agree it is a one-time method, but since there is nothing stopping callers from calling it twice, it would be nice to be clear about what happens and to be able to test it. If someone does call it twice, right now it will be a no-op and it will be fine (unless a test happens to trigger the double-call). But if you later change the implementation of this method to behave less sensibly when called twice, that bug will only be caught in production because you don't (can't) have any tests verifying that the second call is a no-op. So if you remove the DCHECK, you can add a simple test that verifies the behaviour. I'd be perfectly happy to return to the original state ("if (started) { return; }"). I looked through but I couldn't find willchan's comment on this subject. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:40: // Start the execution - posts all tasks in the |deferred_tasks_queue_| to On 2013/04/17 09:53:55, msarda wrote: > On 2013/04/15 17:56:25, erikwright wrote: > > "posts all queued tasks to the target executor." rather than referring to > > internal implementation details. > > The comment on what delay means was added following jyasskin's review. Sorry, the only change I'm referring to is s/tasks in the |deferred_tasks_queue_|/queued tasks/ i.e., don't refer to deferred_tasks_queue_ in the public documentation (this is an implementation detail). I agree that the delay behaviour is an important part of the API. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.cc:22: new base::DeferredSequencedTaskRunner(profile_->GetIOTaskRunner()); On 2013/04/17 09:53:55, msarda wrote: > On 2013/04/15 17:56:25, erikwright wrote: > > Why not eliminate a dependency on profile.h by taking a STR in the constructor > > and storing that instead of Profile*? It's reasonable for the factory to do > the > > accessing for you. > > I don't see the fact that this service holds profile as an inconvenient - it is > a profile service at the end. We could image that in the future other task > runners might be added and they may need the profile. > > Thus I'll keep it as is for now as mirandac (owner for chrome/browser/profiles) > already l g t m -ed this CL. Across the project we are actively trying to reduce coupling to profiles, not increase it. Notice that, if you remove the dependency on Profile, this class goes from very high up the food-chain (depending on browser) to very low in the food chain (depending on base). This is a very good thing for simplifying the browser architecture. If for some reason a future incarnation of this service requires more parameters, let's address it then. Miranda, please chime in if you object. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.h (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:8: #include "base/deferred_sequenced_task_runner.h" On 2013/04/17 09:53:55, msarda wrote: > On 2013/04/15 17:56:25, erikwright wrote: > > forward-decl should be sufficient? > > Done. Oops, not done, you can remove the #include here.
I talked with Miranda. She said she'll review the profile bit again. I address all other comments. willchan: PTAL https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:74: DCHECK(!started_); On 2013/04/17 14:41:19, erikwright wrote: > On 2013/04/17 09:53:55, msarda wrote: > > On 2013/04/15 17:56:25, erikwright wrote: > > > I don't really see any reason to DCHECK in this scenario. Since it could > still > > > be called in production I'd rather have it not DCHECK and see a test that > > > verifies that it behaves sensibly (i.e., a no-op). > > > > Start is a one-time method as per previous review from willchan. > > I agree it is a one-time method, but since there is nothing stopping callers > from calling it twice, it would be nice to be clear about what happens and to be > able to test it. > > If someone does call it twice, right now it will be a no-op and it will be fine > (unless a test happens to trigger the double-call). But if you later change the > implementation of this method to behave less sensibly when called twice, that > bug will only be caught in production because you don't (can't) have any tests > verifying that the second call is a no-op. > > So if you remove the DCHECK, you can add a simple test that verifies the > behaviour. > > I'd be perfectly happy to return to the original state ("if (started) { return; > }"). > > I looked through but I couldn't find willchan's comment on this subject. This is exactly what I did before (for example https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... ). I had to change it following willchan's review (see https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... , comment at line 45). The DCHECK ensures that nobody calls it twice now, right? willchan: I think this is your call - let me know what you prefer here (DCHECK vs redundant calls). https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.h (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.h:40: // Start the execution - posts all tasks in the |deferred_tasks_queue_| to On 2013/04/17 14:41:19, erikwright wrote: > On 2013/04/17 09:53:55, msarda wrote: > > On 2013/04/15 17:56:25, erikwright wrote: > > > "posts all queued tasks to the target executor." rather than referring to > > > internal implementation details. > > > > The comment on what delay means was added following jyasskin's review. > > Sorry, the only change I'm referring to is s/tasks in the > |deferred_tasks_queue_|/queued tasks/ > > i.e., don't refer to deferred_tasks_queue_ in the public documentation (this is > an implementation detail). > > I agree that the delay behaviour is an important part of the API. Done. https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... File chrome/browser/profiles/startup_task_runner_service.h (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/chrome/browser/pro... chrome/browser/profiles/startup_task_runner_service.h:8: #include "base/deferred_sequenced_task_runner.h" On 2013/04/17 14:41:19, erikwright wrote: > On 2013/04/17 09:53:55, msarda wrote: > > On 2013/04/15 17:56:25, erikwright wrote: > > > forward-decl should be sufficient? > > > > Done. > > > Oops, not done, you can remove the #include here. Done for real now.
LGTM with follow-up CL to perform Erik's suggested extraction of Profile, which is correct and important, but shouldn't stop this poor over-worked CL from finally landing. https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... File chrome/browser/profiles/startup_task_runner_service.cc (right): https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... chrome/browser/profiles/startup_task_runner_service.cc:22: new base::DeferredSequencedTaskRunner(profile_->GetIOTaskRunner()); I think Erik is right that this basic structure should be changed, and we should move Profile out of the equation here. However, this CL has been through so much, I think it's fine for Mihai to commit, and then perform this extraction in a follow-up. (We just discussed on IRC, and he agreed to this). On 2013/04/17 14:41:19, erikwright wrote: > On 2013/04/17 09:53:55, msarda wrote: > > On 2013/04/15 17:56:25, erikwright wrote: > > > Why not eliminate a dependency on profile.h by taking a STR in the > constructor > > > and storing that instead of Profile*? It's reasonable for the factory to do > > the > > > accessing for you. > > > > I don't see the fact that this service holds profile as an inconvenient - it > is > > a profile service at the end. We could image that in the future other task > > runners might be added and they may need the profile. > > > > Thus I'll keep it as is for now as mirandac (owner for > chrome/browser/profiles) > > already l g t m -ed this CL. > > Across the project we are actively trying to reduce coupling to profiles, not > increase it. > > Notice that, if you remove the dependency on Profile, this class goes from very > high up the food-chain (depending on browser) to very low in the food chain > (depending on base). This is a very good thing for simplifying the browser > architecture. > > If for some reason a future incarnation of this service requires more > parameters, let's address it then. > > Miranda, please chime in if you object.
Sorry for missing the loop implementation earlier. If I'm correct about the runtime complexity there, I think we should fix it. https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:74: DCHECK(!started_); On 2013/04/17 15:29:10, msarda wrote: > On 2013/04/17 14:41:19, erikwright wrote: > > On 2013/04/17 09:53:55, msarda wrote: > > > On 2013/04/15 17:56:25, erikwright wrote: > > > > I don't really see any reason to DCHECK in this scenario. Since it could > > still > > > > be called in production I'd rather have it not DCHECK and see a test that > > > > verifies that it behaves sensibly (i.e., a no-op). > > > > > > Start is a one-time method as per previous review from willchan. > > > > I agree it is a one-time method, but since there is nothing stopping callers > > from calling it twice, it would be nice to be clear about what happens and to > be > > able to test it. > > > > If someone does call it twice, right now it will be a no-op and it will be > fine > > (unless a test happens to trigger the double-call). But if you later change > the > > implementation of this method to behave less sensibly when called twice, that > > bug will only be caught in production because you don't (can't) have any tests > > verifying that the second call is a no-op. > > > > So if you remove the DCHECK, you can add a simple test that verifies the > > behaviour. > > > > I'd be perfectly happy to return to the original state ("if (started) { > return; > > }"). > > > > I looked through but I couldn't find willchan's comment on this subject. > > This is exactly what I did before (for example > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > ). I had to change it following willchan's review (see > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > , comment at line 45). The DCHECK ensures that nobody calls it twice now, right? > > willchan: I think this is your call - let me know what you prefer here (DCHECK > vs redundant calls). My preference hasn't changed, but I also said before that it was personal style and I wouldn't mandate it. This is one of those places where I don't think it matters too much and I believe back and forth (especially cross-Atlantic) costs more in developer velocity than it leads to any benefit in code quality. https://chromiumcodereview.appspot.com/12952005/diff/109001/base/deferred_seq... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/109001/base/deferred_seq... base/deferred_sequenced_task_runner.cc:80: i = deferred_tasks_queue_.erase(i)) { I just saw this implementation, I missed it before. This looks like an n^2 algorithm, right? It looks to me like it'll erase from the front, which will lead to memmove()'ing all the vector elements forward. Perhaps we should just NULL out (er, I guess it's a struct, so just default construct the struct) the element after posting the task. Then clear the queue at the end of the loop.
https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:74: DCHECK(!started_); I'll keep it as is then. On 2013/04/17 15:40:19, willchan wrote: > On 2013/04/17 15:29:10, msarda wrote: > > On 2013/04/17 14:41:19, erikwright wrote: > > > On 2013/04/17 09:53:55, msarda wrote: > > > > On 2013/04/15 17:56:25, erikwright wrote: > > > > > I don't really see any reason to DCHECK in this scenario. Since it could > > > still > > > > > be called in production I'd rather have it not DCHECK and see a test > that > > > > > verifies that it behaves sensibly (i.e., a no-op). > > > > > > > > Start is a one-time method as per previous review from willchan. > > > > > > I agree it is a one-time method, but since there is nothing stopping callers > > > from calling it twice, it would be nice to be clear about what happens and > to > > be > > > able to test it. > > > > > > If someone does call it twice, right now it will be a no-op and it will be > > fine > > > (unless a test happens to trigger the double-call). But if you later change > > the > > > implementation of this method to behave less sensibly when called twice, > that > > > bug will only be caught in production because you don't (can't) have any > tests > > > verifying that the second call is a no-op. > > > > > > So if you remove the DCHECK, you can add a simple test that verifies the > > > behaviour. > > > > > > I'd be perfectly happy to return to the original state ("if (started) { > > return; > > > }"). > > > > > > I looked through but I couldn't find willchan's comment on this subject. > > > > This is exactly what I did before (for example > > > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > > ). I had to change it following willchan's review (see > > > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > > , comment at line 45). The DCHECK ensures that nobody calls it twice now, > right? > > > > willchan: I think this is your call - let me know what you prefer here (DCHECK > > vs redundant calls). > > My preference hasn't changed, but I also said before that it was personal style > and I wouldn't mandate it. This is one of those places where I don't think it > matters too much and I believe back and forth (especially cross-Atlantic) costs > more in developer velocity than it leads to any benefit in code quality. https://chromiumcodereview.appspot.com/12952005/diff/109001/base/deferred_seq... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/109001/base/deferred_seq... base/deferred_sequenced_task_runner.cc:80: i = deferred_tasks_queue_.erase(i)) { On 2013/04/17 15:40:19, willchan wrote: > I just saw this implementation, I missed it before. This looks like an n^2 > algorithm, right? It looks to me like it'll erase from the front, which will > lead to memmove()'ing all the vector elements forward. Perhaps we should just > NULL out (er, I guess it's a struct, so just default construct the struct) the > element after posting the task. Then clear the queue at the end of the loop. Done.
LGTM. On 2013/04/17 15:35:11, Miranda Callahan wrote: > LGTM with follow-up CL to perform Erik's suggested extraction of Profile, which > is correct and important, but shouldn't stop this poor over-worked CL from > finally landing. It doesn't seem like a big deal to include here, but I don't mind it being a follow-up CL. > > https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... > File chrome/browser/profiles/startup_task_runner_service.cc (right): > > https://codereview.chromium.org/12952005/diff/82001/chrome/browser/profiles/s... > chrome/browser/profiles/startup_task_runner_service.cc:22: new > base::DeferredSequencedTaskRunner(profile_->GetIOTaskRunner()); > I think Erik is right that this basic structure should be changed, and we should > move Profile out of the equation here. However, this CL has been through so > much, I think it's fine for Mihai to commit, and then perform this extraction in > a follow-up. (We just discussed on IRC, and he agreed to this). > > On 2013/04/17 14:41:19, erikwright wrote: > > On 2013/04/17 09:53:55, msarda wrote: > > > On 2013/04/15 17:56:25, erikwright wrote: > > > > Why not eliminate a dependency on profile.h by taking a STR in the > > constructor > > > > and storing that instead of Profile*? It's reasonable for the factory to > do > > > the > > > > accessing for you. > > > > > > I don't see the fact that this service holds profile as an inconvenient - it > > is > > > a profile service at the end. We could image that in the future other task > > > runners might be added and they may need the profile. > > > > > > Thus I'll keep it as is for now as mirandac (owner for > > chrome/browser/profiles) > > > already l g t m -ed this CL. > > > > Across the project we are actively trying to reduce coupling to profiles, not > > increase it. > > > > Notice that, if you remove the dependency on Profile, this class goes from > very > > high up the food-chain (depending on browser) to very low in the food chain > > (depending on base). This is a very good thing for simplifying the browser > > architecture. > > > > If for some reason a future incarnation of this service requires more > > parameters, let's address it then. > > > > Miranda, please chime in if you object.
https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... File base/deferred_sequenced_task_runner.cc (right): https://chromiumcodereview.appspot.com/12952005/diff/82001/base/deferred_sequ... base/deferred_sequenced_task_runner.cc:74: DCHECK(!started_); On 2013/04/17 15:29:10, msarda wrote: > On 2013/04/17 14:41:19, erikwright wrote: > > On 2013/04/17 09:53:55, msarda wrote: > > > On 2013/04/15 17:56:25, erikwright wrote: > > > > I don't really see any reason to DCHECK in this scenario. Since it could > > still > > > > be called in production I'd rather have it not DCHECK and see a test that > > > > verifies that it behaves sensibly (i.e., a no-op). > > > > > > Start is a one-time method as per previous review from willchan. > > > > I agree it is a one-time method, but since there is nothing stopping callers > > from calling it twice, it would be nice to be clear about what happens and to > be > > able to test it. > > > > If someone does call it twice, right now it will be a no-op and it will be > fine > > (unless a test happens to trigger the double-call). But if you later change > the > > implementation of this method to behave less sensibly when called twice, that > > bug will only be caught in production because you don't (can't) have any tests > > verifying that the second call is a no-op. > > > > So if you remove the DCHECK, you can add a simple test that verifies the > > behaviour. > > > > I'd be perfectly happy to return to the original state ("if (started) { > return; > > }"). > > > > I looked through but I couldn't find willchan's comment on this subject. > > This is exactly what I did before (for example > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > ). I had to change it following willchan's review (see > https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ... > , comment at line 45). The DCHECK ensures that nobody calls it twice now, right? > > willchan: I think this is your call - let me know what you prefer here (DCHECK > vs redundant calls). I agree that this is far from a critical point in this CL. The code in this case is visibly safe even if the DCHECK fails.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msarda@chromium.org/12952005/105003
Retried try job too often on linux_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msarda@chromium.org/12952005/133001
On 2013/04/18 12:35:19, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/msarda%40chromium.org/12952005/133001 LGTM, thank you!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msarda@chromium.org/12952005/145003
Message was sent while issue was closed.
Change committed as 194956 |
