Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(11)

Issue 2881143002: Switch BookmarkFaviconFetcher to the TaskScheduler API.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by Sébastien Marchand
Modified:
1 month ago
CC:
chromium-reviews, tfarina, browser-components-watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Switch BookmarkFaviconFetcher to the TaskScheduler API. BUG=689520

Patch Set 1 #

Patch Set 2 : Switch BookmarkFaviconFetcher to the TaskScheduler API. #

Patch Set 3 : Switch to a SingleThreadTaskRunner #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -2 lines) Patch
M chrome/browser/bookmarks/bookmark_html_writer.cc View 1 2 2 chunks +7 lines, -2 lines 6 comments Download
Trybot results:  linux_chromium_asan_rel_ng   win_chromium_x64_rel_ng   win_clang   win_chromium_compile_dbg_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   win_chromium_rel_ng   ios-simulator   ios-device-xcode-clang   ios-device   ios-simulator-xcode-clang   linux_chromium_tsan_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   linux_chromium_rel_ng   chromeos_amd64-generic_chromium_compile_only_ng   chromium_presubmit   cast_shell_linux   chromeos_daisy_chromium_compile_only_ng   android_n5x_swarming_rel   cast_shell_android   android_cronet   linux_android_rel_ng   android_arm64_dbg_recipe   android_clang_dbg_recipe   android_compile_dbg   linux_chromium_rel_ng   linux_chromium_tsan_rel_ng   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator-xcode-clang   ios-simulator   ios-device   ios-device-xcode-clang   linux_chromium_rel_ng   linux_chromium_tsan_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_daisy_chromium_compile_only_ng   cast_shell_linux   chromeos_amd64-generic_chromium_compile_only_ng   linux_android_rel_ng   cast_shell_android   android_n5x_swarming_rel   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe   linux_chromium_tsan_rel_ng   linux_chromium_rel_ng   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-device-xcode-clang   ios-simulator-xcode-clang   ios-simulator   linux_chromium_tsan_rel_ng   ios-device   linux_chromium_compile_dbg_ng   linux_chromium_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_android   cast_shell_linux   linux_android_rel_ng   android_compile_dbg   android_n5x_swarming_rel   android_cronet   android_clang_dbg_recipe   android_arm64_dbg_recipe  Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 35 (25 generated)
fdoray
lgtm
1 month, 1 week ago (2017-05-15 17:09:03 UTC) #3
Sébastien Marchand
PTAL.
1 month, 1 week ago (2017-05-15 17:10:39 UTC) #7
Sébastien Marchand
sky: Please hold on before reviewing, some tests are apparently failing under some configurations, investigating.
1 month, 1 week ago (2017-05-15 17:46:31 UTC) #8
Sébastien Marchand
sky: PTAL now, the blocking bug has been fixed.
1 month, 1 week ago (2017-05-20 00:01:14 UTC) #25
sky
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc#newcode454 chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = Forgive my ignorance, but what happens ...
1 month ago (2017-05-22 16:40:46 UTC) #28
Sébastien Marchand
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc#newcode454 chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 16:40:46, sky wrote: > ...
1 month ago (2017-05-22 17:19:17 UTC) #30
robliao
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc#newcode454 chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 17:19:17, Sébastien Marchand wrote: ...
1 month ago (2017-05-22 17:27:25 UTC) #31
sky
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc#newcode454 chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 17:27:25, robliao wrote: > ...
1 month ago (2017-05-22 17:33:55 UTC) #32
Sébastien Marchand
+gab@ and fdoray@ fyi. https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc File chrome/browser/bookmarks/bookmark_html_writer.cc (right): https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmarks/bookmark_html_writer.cc#newcode454 chrome/browser/bookmarks/bookmark_html_writer.cc:454: scoped_refptr<base::SingleThreadTaskRunner> task_runner_ = On 2017/05/22 ...
1 month ago (2017-05-23 16:56:48 UTC) #34
gab (OOO Friday)
1 month ago (2017-05-23 17:17:00 UTC) #35
https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_html_writer.cc (right):

https://codereview.chromium.org/2881143002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_html_writer.cc:454:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_ =
On 2017/05/23 16:56:48, Sébastien Marchand wrote:
> On 2017/05/22 17:33:55, sky wrote:
> > On 2017/05/22 17:27:25, robliao wrote:
> > > On 2017/05/22 17:19:17, Sébastien Marchand wrote:
> > > > On 2017/05/22 16:40:46, sky wrote:
> > > > > Forgive my ignorance, but what happens if ExecuteWriter is called
twice
> in
> > a
> > > > > row. Might they both end up on separate background threads?
> > > > 
> > > > +fdoray@/robliao@ as I'm not entirely sure, one solution would be to add
a
> > > > "SequencedTaskRunner" member to this class and to use it to run these
> tasks.
> > > > (and otherwise I should remove the trailing underscore if I stick with
> this
> > > > solution)
> > > 
> > > As coded here, if ExecuteRunner is executed twice in a row, then the code
> will
> > > be executed on separate background threads.
> > > 
> > > If this is undesirable BookmarkFaviconFetcher will need to hold on to a
task
> > > runner for future use. 
> > > 
> > > If this is okay, instead the code should just post a parallel task since
the
> > > task runner is immediately thrown away.
> > > 
> > > Nit: task_runner_ -> task_runner
> > 
> > While likely rare, we should make sure there aren't two running at the same
> > time.
> 
> One way to do this would be to add a SequencedTaskRunner member to this class
> and to use it to run these tasks in sequence, this'll work if we assume that
> this class is used as a singleton (otherwise 2 different instances will use a
> separate task runner and might run in parallel). This isn't really the case,
> this class could be instantiated directly or used via its global instance (via
> WriteBookmarks).
> 
> There doesn't seem to be any synchronization around |g_fetcher|,
WriteBookmarks
> create it if needed and this function destroys it, this could be racy if 2
> separate threads call WriteBookmarks.
> 
> Another option could be to use a single
> LazyInstance<scoped_refptr<SequencedTaskRunner>> instance in this file, so all
> the write operations will use the same task runner. This is probably the
safest
> approach for now.
> 
> One of the goal of this CL was to dogfood the new task scheduler API, I think
> that this class is kind of hostile to task runners as is and this is probably
> good feedback to the task scheduler team.

I agree with sky that although there's only one call in this file, multiple
calls of it must be sequenced with each other.

Looks like sequencing only needs to happen for same |path_| though so having a
SequencedTaskRunner per BookmarkFaviconFetcher instance sounds like it would
work?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318