Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
780203002:
Fix threading bugs in product label UI. (Closed)
DescriptionFix threading bugs in product label UI.
As mentioned in https://codereview.chromium.org/680393002/#msg6, the current code had some threading issues.
BUG=438953
R=dbeam@chromium.org,satorux@chromium.org
CC=stevenjb@chromium.org
Committed: https://crrev.com/ec5c21a2672ecf15f25ce7020393df21976a2d35
Cr-Commit-Position: refs/heads/master@{#307640}
Patch Set 1 #Patch Set 2 : File path and ImageSource changes #
Total comments: 6
Patch Set 3 : fix ImageSource #
Total comments: 12
Patch Set 4 : address stevenjb's comments #Patch Set 5 : address satorux's comments #
Total comments: 10
Patch Set 6 : #
Messages
Total messages: 40 (12 generated)
lgtm but you probably want satorux@ to review as well
michaelpg@chromium.org changed reviewers: + satorux@chromium.org
michaelpg@chromium.org changed required reviewers: + dbeam@chromium.org
@satorux: mind taking a look? It's basically what you suggested. The FILE thread is deprecated anyway so I don't know what possessed me to write it the way I did originally. Thanks.
To construct a FilePath please do not pasd std::string to the constructor because it's not portable. Please use AppendASCII or FromUTF8Unsafe. Sent from mobile. 2014/12/05 11:57 <michaelpg@chromium.org>: > @satorux: mind taking a look? It's basically what you suggested. The FILE > thread > is deprecated anyway so I don't know what possessed me to write it the way > I did > originally. > Thanks. > https://codereview.chromium.org/780203002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I see. I've found other examples where we use an absolute path, but none of the those are "portable" either. I don't like functions with "unsafe" in them, so what's the most correct way to do this? base::FilePath(const StringType& path) -- how do I get a StringType from my const char* ? StringType(kConstCharPtr) will not compile if StringType is std::wstring. I can't imagine this function should need ifdef's. base::FilePath path; path.AppendASCII(k1); -- won't work because k1 starts at the root, so DCHECK(!IsPathAbsolute(k1)) fails. (Other Chrome OS path constants that start at the root just use base::FilePath(const char*).) I could use "PathService::Get(base::DIR_SOURCE_ROOT, &path)" but DIR_SOURCE_ROOT says it should only be used in testing. There must be an easy way to do this I can't find, or I'm making this harder than it has to be. Sorry for dragging it out. Michael On Thu, Dec 4, 2014 at 7:03 PM, Satoru Takabayashi <satorux@chromium.org> wrote: > To construct a FilePath please do not pasd std::string to the constructor > because it's not portable. Please use AppendASCII or FromUTF8Unsafe. > > Sent from mobile. > 2014/12/05 11:57 <michaelpg@chromium.org>: > > @satorux: mind taking a look? It's basically what you suggested. The FILE >> thread >> is deprecated anyway so I don't know what possessed me to write it the >> way I did >> originally. >> >> Thanks. >> >> https://codereview.chromium.org/780203002/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
See also FILE_PATH_LITERAL. Sent from mobile. 2014/12/05 13:11 "Michael Giuffrida" <michaelpg@chromium.org>: > I see. I've found other examples where we use an absolute path, but none > of the those are "portable" either. I don't like functions with "unsafe" > in > them, so what's the most correct way to do this? > base::FilePath(const StringType& path) -- how do I get a StringType from > my const char* ? StringType(kConstCharPtr) will not compile if StringType > is std::wstring. I can't imagine this function should need ifdef's. > base::FilePath path; path.AppendASCII(k1); -- won't work because k1 starts > at the root, so DCHECK(!IsPathAbsolute(k1)) fails. (Other Chrome OS path > constants that start at the root just use base::FilePath(const char*).) > I could use "PathService::Get(base::DIR_SOURCE_ROOT, &path)" but > DIR_SOURCE_ROOT says it should only be used in testing. > There must be an easy way to do this I can't find, or I'm making this > harder than it has to be. Sorry for dragging it out. > Michael > On Thu, Dec 4, 2014 at 7:03 PM, Satoru Takabayashi <satorux@chromium.org> > wrote: >> To construct a FilePath please do not pasd std::string to the constructor >> because it's not portable. Please use AppendASCII or FromUTF8Unsafe. >> Sent from mobile. >> 2014/12/05 11:57 <michaelpg@chromium.org>: >> @satorux: mind taking a look? It's basically what you suggested. The FILE >>> thread >>> is deprecated anyway so I don't know what possessed me to write it the >>> way I did >>> originally. >>> Thanks. >>> https://codereview.chromium.org/780203002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
michaelpg@chromium.org changed required reviewers: - dbeam@chromium.org
PTAL dbeam@ and satorux@. I've changed the way we load the chromeos-assets path and made changes to how image loading is scheduled in image_source.cc. The multithreading stuff looks a lot better to me now but please check my work. Thanks! https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/image_source.cc (left): https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/image_source.cc:53: BrowserThread::PostTask( So this was also using the deprecated FILE thread, and also not correct. We need to create the UserImageLoader on the thread that manages its lifetime (the UI thread), in part because it retains as reference to the thread it was created on. This becomes more apparent when we switch to using a sequenced task runner. Also we don't want to re-create a new UserImageLoader each time an image is requested. https://codereview.chromium.org/780203002/diff/80001/chromeos/chromeos_paths.h File chromeos/chromeos_paths.h (right): https://codereview.chromium.org/780203002/diff/80001/chromeos/chromeos_paths.... chromeos/chromeos_paths.h:46: DIR_SHARED_ASSETS, // Directory where shared Chrome OS assets are kept. Is it OK to add this here?
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/image_source.cc:46: url_data_callback.Run(NULL); This invokes |url_data_callback| (which is just |callback| below, we should name these all consistently, e.g. |got_data_callback|) on the Worker thread, whereas |image_loader| will call |image_loaded_callback| on the thread it was constructed on (see below) which runs |url_data_callback| on that same thread. To fix this we should probably use PostTaskAndReplyWithResult and return false here (and true above), then invoke |url_data_callback| in the reply if the result is false. https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/image_source.cc:79: task_runner_); Constructing this here means that |callback| will be called on this thread instead of the file/worker thread (see UserImageLoader() and foreground_task_runner_). This is probably what we actually want, but it means that we should ensure that |callback| always gets called from this thread.
PTAL. https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/image_source.cc:46: url_data_callback.Run(NULL); On 2014/12/05 19:31:53, stevenjb wrote: > This invokes |url_data_callback| (which is just |callback| below, we should name > these all consistently, e.g. |got_data_callback|) on the Worker thread, whereas > |image_loader| will call |image_loaded_callback| on the thread it was > constructed on (see below) which runs |url_data_callback| on that same thread. > > To fix this we should probably use PostTaskAndReplyWithResult and return false > here (and true above), then invoke |url_data_callback| in the reply if the > result is false. Done. https://codereview.chromium.org/780203002/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/image_source.cc:79: task_runner_); On 2014/12/05 19:31:53, stevenjb wrote: > Constructing this here means that |callback| will be called on this thread > instead of the file/worker thread (see UserImageLoader() and > foreground_task_runner_). > > This is probably what we actually want, but it means that we should ensure that > |callback| always gets called from this thread. Done.
https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:30: // Returns true if the image is found and false otherwise. More relevant is that it returns true if it starts loading the image. Comment could just be something like: // Searches the shared assets directory for the image. If found, // starts loading the image and returns true, otherwise returns false. https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:109: got_data_callback.Run(NULL); This could be an anonymous non-member function. |got_data_callback| should make its own lifetime assurances (i.e. it shouldn't rely on cancellation on destruction of this class, it should be safe to call got_data_callback.Run() at any time). https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_handler.cc (right): https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.cc:132: // Reads the file containing the FCC label text, if found. nit: Comment that this is called from the blocking pool.
https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_handler.cc (right): https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.cc:136: if (PathService::Get(chromeos::DIR_SHARED_ASSETS, &shared_assets_dir) && Why not just use chrome::kChromeOSAssetPath? Something like below should work. const base::FilePath asset_dir(FILE_PATH_LITERAL(chrome::kChromeOSAssetPath)); const base::FilePath label_file_path = asset_dir.AppendASCII(kFCCLabelTextPath); https://codereview.chromium.org/780203002/diff/100001/chromeos/chromeos_paths.h File chromeos/chromeos_paths.h (right): https://codereview.chromium.org/780203002/diff/100001/chromeos/chromeos_paths... chromeos/chromeos_paths.h:46: DIR_SHARED_ASSETS, // Directory where shared Chrome OS assets are kept. Introduction of this seems unnecessary. If we are to introduce this, we should remove chrome::kChromeOSAssetPath so there won't be duplicate constants.
https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:30: // Returns true if the image is found and false otherwise. On 2014/12/05 23:28:07, stevenjb wrote: > More relevant is that it returns true if it starts loading the image. Comment > could just be something like: > > // Searches the shared assets directory for the image. If found, > // starts loading the image and returns true, otherwise returns false. Done. https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:109: got_data_callback.Run(NULL); On 2014/12/05 23:28:07, stevenjb wrote: > This could be an anonymous non-member function. |got_data_callback| should make > its own lifetime assurances (i.e. it shouldn't rely on cancellation on > destruction of this class, it should be safe to call got_data_callback.Run() at > any time). > Done, along with ImageLoaded. https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_handler.cc (right): https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.cc:132: // Reads the file containing the FCC label text, if found. On 2014/12/05 23:28:07, stevenjb wrote: > nit: Comment that this is called from the blocking pool. Done. https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.cc:136: if (PathService::Get(chromeos::DIR_SHARED_ASSETS, &shared_assets_dir) && On 2014/12/08 00:43:57, satorux1 wrote: > Why not just use chrome::kChromeOSAssetPath? > > Something like below should work. > > const base::FilePath asset_dir(FILE_PATH_LITERAL(chrome::kChromeOSAssetPath)); > const base::FilePath label_file_path = asset_dir.AppendASCII(kFCCLabelTextPath); I don't know whether it would be better to use chrome/common/url_constants.h or chromeos/chromeos_paths.cc. It looked like chromeos_paths is more idiomatic because it relates to path on disk, not URL to get to that path. But I'll do whatever you think is best. https://codereview.chromium.org/780203002/diff/100001/chromeos/chromeos_paths.h File chromeos/chromeos_paths.h (right): https://codereview.chromium.org/780203002/diff/100001/chromeos/chromeos_paths... chromeos/chromeos_paths.h:46: DIR_SHARED_ASSETS, // Directory where shared Chrome OS assets are kept. On 2014/12/08 00:43:57, satorux1 wrote: > Introduction of this seems unnecessary. If we are to introduce this, we should > remove chrome::kChromeOSAssetPath so there won't be duplicate constants. This patch did remove chrome::kChromeOSAssetPath. That constant was added by my initial commit for this feature. So whichever way we go, it will not be redundant.
https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_handler.cc (right): https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.cc:136: if (PathService::Get(chromeos::DIR_SHARED_ASSETS, &shared_assets_dir) && On 2014/12/08 00:58:19, michaelpg wrote: > On 2014/12/08 00:43:57, satorux1 wrote: > > Why not just use chrome::kChromeOSAssetPath? > > > > Something like below should work. > > > > const base::FilePath asset_dir(FILE_PATH_LITERAL(chrome::kChromeOSAssetPath)); > > const base::FilePath label_file_path = > asset_dir.AppendASCII(kFCCLabelTextPath); > > I don't know whether it would be better to use chrome/common/url_constants.h or > chromeos/chromeos_paths.cc. It looked like chromeos_paths is more idiomatic > because it relates to path on disk, not URL to get to that path. > > But I'll do whatever you think is best. I'd suggest to keep using chrome::kChromeOSAssetPath. There are already some other file path constants in url_constants.cc so removing kChromeOSAssetPath alone doesn't make the code much cleaner. Besides, I think it'd be nicer to keep this patch small to make it easier to merge.
lgtm
Patchset #5 (id:140001) has been deleted
PTAL satorux, thanks! https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_handler.cc (right): https://codereview.chromium.org/780203002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.cc:136: if (PathService::Get(chromeos::DIR_SHARED_ASSETS, &shared_assets_dir) && On 2014/12/08 01:12:27, satorux1 wrote: > On 2014/12/08 00:58:19, michaelpg wrote: > > On 2014/12/08 00:43:57, satorux1 wrote: > > > Why not just use chrome::kChromeOSAssetPath? > > > > > > Something like below should work. > > > > > > const base::FilePath > asset_dir(FILE_PATH_LITERAL(chrome::kChromeOSAssetPath)); > > > const base::FilePath label_file_path = > > asset_dir.AppendASCII(kFCCLabelTextPath); > > > > I don't know whether it would be better to use chrome/common/url_constants.h > or > > chromeos/chromeos_paths.cc. It looked like chromeos_paths is more idiomatic > > because it relates to path on disk, not URL to get to that path. > > > > But I'll do whatever you think is best. > > I'd suggest to keep using chrome::kChromeOSAssetPath. There are already some > other file path constants in url_constants.cc so removing kChromeOSAssetPath > alone doesn't make the code much cleaner. Besides, I think it'd be nicer to keep > this patch small to make it easier to merge. Done.
https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:49: bool load_started) { maybe add a DCHECK to ensure this runs on the UI thread? https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:55: void ImageLoaded( maybe add a DCHECK to ensure this runs on the UI thread? image_loader->Start() will run the callback on the thread where the image loader was created, but it's not obvious (in fact I had to read the code of user_image_loader.cc) https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:105: base::Bind(LoadImageAttempted, got_data_callback)); got_data_callback can be called either from LoadImageAttempted() or ImageLoaded() which I found rather confusing. Seems to me that PostTaskAndReplyWithResult() is not a good tool here. How about using PostTask() like: base::PostTask( ... base::Bind(&StartOnBlockingPool, path, got_data_callback, base::MessageLoopProxy::current())); In StartOnBlockingPool(), you can use the message loop proxy to post a task to the original thread, like: if (base::PathExists(image_path)) { image_loader->Start(...); } else { message_loop_proxy->PostTask(..., base::Bind(got_data_dallback, NULL); } https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_handler.h (right): https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.h:95: void OnFCCLabelTextRead(std::string text); const std::string&
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:49: bool load_started) { On 2014/12/09 05:56:40, satorux1 wrote: > maybe add a DCHECK to ensure this runs on the UI thread? fn removed https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:55: void ImageLoaded( On 2014/12/09 05:56:40, satorux1 wrote: > maybe add a DCHECK to ensure this runs on the UI thread? image_loader->Start() > will run the callback on the thread where the image loader was created, but it's > not obvious (in fact I had to read the code of user_image_loader.cc) Done. https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:105: base::Bind(LoadImageAttempted, got_data_callback)); On 2014/12/09 05:56:40, satorux1 wrote: > got_data_callback can be called either from LoadImageAttempted() or > ImageLoaded() which I found rather confusing. Seems to me that > PostTaskAndReplyWithResult() is not a good tool here. How about using PostTask() > like: > > base::PostTask( > ... > base::Bind(&StartOnBlockingPool, path, got_data_callback, > base::MessageLoopProxy::current())); > > In StartOnBlockingPool(), you can use the message loop proxy to post a task to > the original thread, like: > > if (base::PathExists(image_path)) { > image_loader->Start(...); > } else { > message_loop_proxy->PostTask(..., base::Bind(got_data_dallback, NULL); > } I could not find a way to run base::Bind on got_data_callback like in your example. Is it less confusing if I get rid of LoadImageAttempted and pass an empty UserImage (which should be cheap) to ImageLoaded instead? Or, should it be possible to create a Callback by binding another Callback's Run method to itself? Something like: base::Bind(&content::URLDataSource::GotDataCallback::Run, base::Unretained(&got_data_callback), NULL) gives me something I can't really parse: http://pastebin.com/0Tqt3Gx0 https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_handler.h (right): https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_handler.h:95: void OnFCCLabelTextRead(std::string text); On 2014/12/09 05:56:41, satorux1 wrote: > const std::string& Done.
https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:105: base::Bind(LoadImageAttempted, got_data_callback)); On 2014/12/10 00:48:51, michaelpg wrote: > On 2014/12/09 05:56:40, satorux1 wrote: > > got_data_callback can be called either from LoadImageAttempted() or > > ImageLoaded() which I found rather confusing. Seems to me that > > PostTaskAndReplyWithResult() is not a good tool here. How about using > PostTask() > > like: > > > > base::PostTask( > > ... > > base::Bind(&StartOnBlockingPool, path, got_data_callback, > > base::MessageLoopProxy::current())); > > > > In StartOnBlockingPool(), you can use the message loop proxy to post a task to > > the original thread, like: > > > > if (base::PathExists(image_path)) { > > image_loader->Start(...); > > } else { > > message_loop_proxy->PostTask(..., base::Bind(got_data_dallback, NULL); > > } > > I could not find a way to run base::Bind on got_data_callback like in your > example. Try passing nullptr instead of NULL.
On 2014/12/10 01:18:17, satorux1 wrote: > https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chromeos/image_source.cc (right): > > https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/image_source.cc:105: > base::Bind(LoadImageAttempted, got_data_callback)); > On 2014/12/10 00:48:51, michaelpg wrote: > > On 2014/12/09 05:56:40, satorux1 wrote: > > > got_data_callback can be called either from LoadImageAttempted() or > > > ImageLoaded() which I found rather confusing. Seems to me that > > > PostTaskAndReplyWithResult() is not a good tool here. How about using > > PostTask() > > > like: > > > > > > base::PostTask( > > > ... > > > base::Bind(&StartOnBlockingPool, path, got_data_callback, > > > base::MessageLoopProxy::current())); > > > > > > In StartOnBlockingPool(), you can use the message loop proxy to post a task > to > > > the original thread, like: > > > > > > if (base::PathExists(image_path)) { > > > image_loader->Start(...); > > > } else { > > > message_loop_proxy->PostTask(..., base::Bind(got_data_dallback, NULL); > > > } > > > > I could not find a way to run base::Bind on got_data_callback like in your > > example. > > Try passing nullptr instead of NULL. Oh cool, that works. I didn't realize you could bind a Callback that easily.
Patchset #7 (id:220001) has been deleted
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/image_source.cc (right): https://codereview.chromium.org/780203002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/image_source.cc:105: base::Bind(LoadImageAttempted, got_data_callback)); On 2014/12/10 01:18:17, satorux1 wrote: > On 2014/12/10 00:48:51, michaelpg wrote: > > On 2014/12/09 05:56:40, satorux1 wrote: > > > got_data_callback can be called either from LoadImageAttempted() or > > > ImageLoaded() which I found rather confusing. Seems to me that > > > PostTaskAndReplyWithResult() is not a good tool here. How about using > > PostTask() > > > like: > > > > > > base::PostTask( > > > ... > > > base::Bind(&StartOnBlockingPool, path, got_data_callback, > > > base::MessageLoopProxy::current())); > > > > > > In StartOnBlockingPool(), you can use the message loop proxy to post a task > to > > > the original thread, like: > > > > > > if (base::PathExists(image_path)) { > > > image_loader->Start(...); > > > } else { > > > message_loop_proxy->PostTask(..., base::Bind(got_data_dallback, NULL); > > > } > > > > I could not find a way to run base::Bind on got_data_callback like in your > > example. > > Try passing nullptr instead of NULL. Done.
LGTM. I think the complexity in image_source.cc comes from the (IMHO) weird API of UserImageLoader where ::Start() should be called from a blocking pool thread, but the callback is run on the thread where the loader object is created. I think it'd be much simpler if we could call Start() from the UI thread by hiding the PostTaskAndReply details inside of the Start() implementation. This way, you don't need to do the PostTaskAndReply dance in image_source.cc. I'd appreciate it if you could take a look and see if the API of UserImageLoader::Start() can be simplified, and clean it up in a separate patch.
Hmm, I was wrong. you still need to call base::PathExists(image_path) in a blocking pool thread, even if Start() can be called from the UI thread...
But I guess UserImageLoader should be responsible to check if a file exists, and raises an error via the callback, rather than checking this on the caller side.
The CQ bit was checked by michaelpg@chromium.org
Thanks. Yeah, I think all these things are true. I also added a TODO last time to generalize UserImageLoader now that we're using it for more than UserImages, so I can simplify as a part of that process. On Tue, Dec 9, 2014 at 5:59 PM, <satorux@chromium.org> wrote: > But I guess UserImageLoader should be responsible to check if a file > exists, and > raises an error via the callback, rather than checking this on the caller > side. > > https://codereview.chromium.org/780203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/780203002/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ec5c21a2672ecf15f25ce7020393df21976a2d35 Cr-Commit-Position: refs/heads/master@{#307640}
Message was sent while issue was closed.
FWIW correction for #32. > I think the complexity in image_source.cc comes from the (IMHO) weird API of > UserImageLoader where ::Start() should be called from a blocking pool thread, > but the callback is run on the thread where the loader object is created. This is not true. Start() can be called from the UI thread. The reason why this is called from a blocking pool thread in the client code is that it has to call base::PathExists() to see if a file exists. As commented in #34, the existence check should probably be done in UserImageLoader. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
