Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1122103003:
[Large Icon Service] Move icon resizing into worker thread. (Closed)
Description[Large Icon Service] Move icon resizing into worker thread.
Using https://crrev.com/15679025 as template, but with some complication
involving base::CancelableTaskTracker.
BUG=467712
Committed: https://crrev.com/7f7237f4f12af522dafdadb0a0f789f88f579821
Cr-Commit-Position: refs/heads/master@{#329248}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Refactoring: extract Session inner class. #
Total comments: 12
Patch Set 3 : Comment and DCHECK() fixes. #
Total comments: 15
Patch Set 4 : Make Session a class; make using callback chain functions static. #
Total comments: 22
Patch Set 5 : Make Session manage the callback chain. #Patch Set 6 : Sync and merge (need to fix tests still). #Patch Set 7 : Inject caller's task runner to Session; fix tests by mocking GetBackgroundTaskRunner(). #
Total comments: 4
Patch Set 8 : Rename Session to LargeIconWorker and move to anonymous namespace; use PostTaskAndReply(). #
Total comments: 14
Patch Set 9 : Comment cleanup; making ResizeLargeIconOnBackgroundThreadIfValid() a member function. #
Total comments: 1
Messages
Total messages: 40 (11 generated)
huangs@chromium.org changed reviewers: + beaudoin@chromium.org, pkotwicz@chromium.org
Follow up CL to https://crrev.com/1107283006/, as said I'd do. PTAL.
https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... components/favicon/core/large_icon_service.cc:116: callback.Run(result); Shouldn't that callback go back to the initial thread? The invoker may not be thread safe or may expect to be on the UI thread. (Not entirely sure how to go back to the owner's thread...) https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... components/favicon/core/large_icon_service.cc:125: DCHECK(thread_checker_.CalledOnValidThread()); From reading CalledOnValidThread it looks like this first call will take a reference to the current thread ID which will later be checked against the ThreadID in ResizeLargeIconIfValid. As a result I would expect the second call to always fail since it's in a different thread. I wonder if there is a way to check that the threads are actually different, or if DCHECKing this is actually desirable. https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... components/favicon/core/large_icon_service.h:68: // Intermediate callback for GetLargeIconOrFallbackStyle(). // Meant to be invoked on the UI thread, this should not perform complex image operations.
Updated, PTAL. https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... components/favicon/core/large_icon_service.cc:116: callback.Run(result); On 2015/05/05 00:00:52, beaudoin wrote: > Shouldn't that callback go back to the initial thread? The invoker may not be > thread safe or may expect to be on the UI thread. (Not entirely sure how to go > back to the owner's thread...) Done. https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... components/favicon/core/large_icon_service.cc:125: DCHECK(thread_checker_.CalledOnValidThread()); Using a different way to check. https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/1/components/favicon/core/lar... components/favicon/core/large_icon_service.h:68: // Intermediate callback for GetLargeIconOrFallbackStyle(). On 2015/05/05 00:00:52, beaudoin wrote: > // Meant to be invoked on the UI thread, this should not perform complex image > operations. Done.
huangs@chromium.org changed reviewers: + jochen@chromium.org - beaudoin@chromium.org
huangs@chromium.org changed reviewers: + beaudoin@chromium.org
OWNER review to jochen@, PTAL.
https://chromiumcodereview.appspot.com/1122103003/diff/20001/components/favic... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1122103003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:41: // Main entry point, runs on the UI thread. NIT: must run on the UI thread. That comment should probably be in the header file. https://chromiumcodereview.appspot.com/1122103003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:48: CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); This is typically a DCHECK in Chrome, any reason to use a CHECK here? https://chromiumcodereview.appspot.com/1122103003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:72: // if successful. This runs on a background thread. The method comment should be in the .h, except // static https://chromiumcodereview.appspot.com/1122103003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:77: CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); DCHECK? https://chromiumcodereview.appspot.com/1122103003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:135: session->bitmap_result, &resized_bitmap_result)) { Indentation looks wrong. https://chromiumcodereview.appspot.com/1122103003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:155: session)); Would it be possible to skip the intermediate function? Something like: base::Bind(&base::Callback<void(const LargeIconResult&)>::Run, base::Unretained(session), *session->result)
Updated, PTAL. Ping pkotwicz@, who suggested the worker thread idea. https://codereview.chromium.org/1122103003/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:41: // Main entry point, runs on the UI thread. On 2015/05/05 19:44:40, beaudoin wrote: > NIT: must run on the UI thread. That comment should probably be in the header > file. Done. https://codereview.chromium.org/1122103003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:48: CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); Because I'm testing with release build. Changed to DCHECK. https://codereview.chromium.org/1122103003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:72: // if successful. This runs on a background thread. I briefly made this local, forgot to remove when I undid the change. https://codereview.chromium.org/1122103003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:77: CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2015/05/05 19:44:40, beaudoin wrote: > DCHECK? Done. https://codereview.chromium.org/1122103003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:135: session->bitmap_result, &resized_bitmap_result)) { Ran "git cl format", let's see how this changes things. https://codereview.chromium.org/1122103003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:155: session)); Yes but I think it's cleaner this way.
https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/BUILD.gn (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/BUILD.gn:32: "//content", //content/public/browser https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/DEPS (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/DEPS:5: "+content/public", you shouldn't depend on all of content/public, but just the dirs you need (here c/p/browser, right?) https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:120: void LargeIconService::ProcessIconOnBackgroundThread( how can you ensure that "this" is still a valid pointer here? should this maybe be a static method? or a method on Session which is properly refcounted? https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:149: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); same here https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.h:28: struct Session : base::RefCountedThreadSafe<Session> { this should be a class
Have a question on the general case (see comment). https://chromiumcodereview.appspot.com/1122103003/diff/40001/components/favic... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1122103003/diff/40001/components/favic... components/favicon/core/large_icon_service.cc:120: void LargeIconService::ProcessIconOnBackgroundThread( So making OnIconLookupComplete() ProcessIconOnBackgroundThread() OnIconProcessingComplete() all static, seeing that they don't use any instance variables? In this case it would work. But for the general case (if instance variable are used) what would be the preferred way? Also, how about potential use-after-free with caller's |callback|? Or is that taken care of in Callback? Thanks!
https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:41: // Main entry point. Must run on the UI thread. Move comment to .h? https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:108: // Must run on the UI thread. I get the impression these comments are typically in the .h. Comments on implementation of methods just look strange to me.
On 2015/05/06 at 13:28:08, huangs wrote: > Have a question on the general case (see comment). > > https://chromiumcodereview.appspot.com/1122103003/diff/40001/components/favic... > File components/favicon/core/large_icon_service.cc (right): > > https://chromiumcodereview.appspot.com/1122103003/diff/40001/components/favic... > components/favicon/core/large_icon_service.cc:120: void LargeIconService::ProcessIconOnBackgroundThread( > So making > OnIconLookupComplete() > ProcessIconOnBackgroundThread() > OnIconProcessingComplete() > all static, seeing that they don't use any instance variables? In this case it would work. But for the general case (if instance variable are used) what would be the preferred way? you can use static methods, or make the class RefCountedThreadSafe, or use weak pointers. > > Also, how about potential use-after-free with caller's |callback|? Or is that taken care of in Callback? Thanks! The caller has to use the appropriate mechanism to ensure that the callback stays valid (basically same choice as above)
Updated, PTAL. https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/BUILD.gn (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/BUILD.gn:32: "//content", On 2015/05/06 12:58:48, jochen wrote: > //content/public/browser Done. https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/DEPS (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/DEPS:5: "+content/public", On 2015/05/06 12:58:48, jochen wrote: > you shouldn't depend on all of content/public, but just the dirs you need (here > c/p/browser, right?) Done. https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:41: // Main entry point. Must run on the UI thread. On 2015/05/06 14:06:53, beaudoin wrote: > Move comment to .h? Done. https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:108: // Must run on the UI thread. On 2015/05/06 14:06:53, beaudoin wrote: > I get the impression these comments are typically in the .h. Comments on > implementation of methods just look strange to me. Done. https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:120: void LargeIconService::ProcessIconOnBackgroundThread( Going with static to keep things simple. https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.cc:149: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2015/05/06 12:58:48, jochen wrote: > same here Done. https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/40001/components/favicon/core... components/favicon/core/large_icon_service.h:28: struct Session : base::RefCountedThreadSafe<Session> { On 2015/05/06 12:58:49, jochen wrote: > this should be a class Done.
LGTM
Looks good. A few nits https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.cc:20: // Return a TaskRunner used to execute background task. Nit: "Return TaskRunner" (no "a") https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.cc:73: bool LargeIconService::ResizeLargeIconIfValid( Can you please add the "OnBackgroundThread" suffix to the method name for the sake of consistency? https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.cc:147: base::Bind(&LargeIconService::OnIconProcessingComplete, session)); Is there a reason that you pass the session object to OnIconProcessingComplete() instead of LargeIconResult? https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:22: class LargeIconResult; The forward declaration is unnecessary because you are including favicon_types.h? https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:31: ~Session(); Nit: Please add a new line https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:75: // Must run on a background thread Nit: missing period "thread." https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:78: // |callback|. The runs on a background thread since image resizing and How about: "The runs on a background thread since" -> "This must be run on a background thread because" https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:83: // Callback to be invoked when ProcessIconOnBackgroundThread() is ready. How about: "Invoked when ProcessIconOnBackgroundThread() is done."
https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... File components/favicon/core/BUILD.gn (right): https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/BUILD.gn:32: "//content/public/browser", why does the gypi version not also need this dependency? https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:31: ~Session(); ref counted dtor should be private and friend RefCountedThreadSafe (see the comment in ref_counted.h for an example) https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:32: int desired_size_in_pixel; classes should have getters/setters
Updated with some reshuffling, PTAL. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... File components/favicon/core/BUILD.gn (right): https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/BUILD.gn:32: "//content/public/browser", Added. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.cc:20: // Return a TaskRunner used to execute background task. Done, also using "Returns". https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.cc:73: bool LargeIconService::ResizeLargeIconIfValid( Done, but "IfValid" wins as suffix. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.cc:147: base::Bind(&LargeIconService::OnIconProcessingComplete, session)); Using session->callback and session->result. And even if one is used I'd still pass |session| for consistency. Moot now due to recent update. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:22: class LargeIconResult; On 2015/05/07 14:52:14, pkotwicz wrote: > The forward declaration is unnecessary because you are including > favicon_types.h? Done. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:31: ~Session(); On 2015/05/07 14:52:14, pkotwicz wrote: > Nit: Please add a new line Done. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:31: ~Session(); On 2015/05/07 14:52:14, pkotwicz wrote: > Nit: Please add a new line Done. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:32: int desired_size_in_pixel; This would add bunch of boiler-plates, and I think it makes the code less readable. struct would make more sense, but that's blocked. So I'm making Session manage the callback chain => everything is internal, and params injected by ctor. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:75: // Must run on a background thread On 2015/05/07 14:52:14, pkotwicz wrote: > Nit: missing period "thread." Done. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:78: // |callback|. The runs on a background thread since image resizing and On 2015/05/07 14:52:14, pkotwicz wrote: > How about: "The runs on a background thread since" -> "This must be run on a > background thread because" Done. https://codereview.chromium.org/1122103003/diff/60001/components/favicon/core... components/favicon/core/large_icon_service.h:83: // Callback to be invoked when ProcessIconOnBackgroundThread() is ready. On 2015/05/07 14:52:14, pkotwicz wrote: > How about: "Invoked when ProcessIconOnBackgroundThread() is done." Done.
lgtm
Fixed tests. pkotwicz@: Another look?
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1122103003/#ps120001 (title: "Inject caller's task runner to Session; fix tests by mocking GetBackgroundTaskRunner().")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1122103003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
A few comments w.r.t to your new approach (I actually liked the static methods but this approach is fine too) https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:48: this)); Shouldn't you use CancelableTaskTracker::PostTaskAndReply() https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.h:33: class Session : public base::RefCountedThreadSafe<Session> { I wonder whether: - This class can be called LargeIconProcessor - Session::OnIconLookupComplete() can be renamed to LargeIconProcessor::Run() - Live in the anonymous namespace - Not be ref counted (Deletes itself when it is done its processing) e.g. UpdateShortcutWorker does this. I think that the lifetime of Session is pretty subtle
PTAL. https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:48: this)); Nice! Done. https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.h:33: class Session : public base::RefCountedThreadSafe<Session> { - Renamed to LargeIconWorker. - Kept Session::OnIconLookupComplete() name per offline discussion. - Moved to anonymous namespace, and updated #include. - Kept ref counted, per discussion.
LGTM with nits https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:23: // the callback chain on different threads. How about: "Processes the bitmap data returned from the FaviconService as part of a LargeIconService request." https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:45: // Tries to resize |bitmap_result| and pass the output to |callback|. If |bitmap_result| -> |bitmap_result_| |callback| -> |callback_| https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:47: // invoke |callback|. This must be run on a background thread because image |callback| -> |callback_| https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:109: // Failed to resize |bitmap_result|, so compute fallback icon style. Nit: |bitmap_result| -> |bitmap_result_| https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:122: bool LargeIconWorker::ResizeLargeIconOnBackgroundThreadIfValid( Nit: Can't you grab |min_source_size_in_pixel|, |desired_size_in_pixel| and |bitmap_result| from the members of LargeIconWorker? https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:192: scoped_refptr<LargeIconWorker> session = Nit: session -> worker https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.h:10: #include "base/memory/ref_counted.h" Nit: The include can now be in the .cc file?
Updated. https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:23: // the callback chain on different threads. On 2015/05/11 18:57:39, pkotwicz wrote: > How about: "Processes the bitmap data returned from the FaviconService as part > of a LargeIconService request." Done. https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:45: // Tries to resize |bitmap_result| and pass the output to |callback|. If On 2015/05/11 18:57:39, pkotwicz wrote: > |bitmap_result| -> |bitmap_result_| > |callback| -> |callback_| Done. Also below since we're making static function a member function. https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:47: // invoke |callback|. This must be run on a background thread because image On 2015/05/11 18:57:39, pkotwicz wrote: > |callback| -> |callback_| Done. https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:109: // Failed to resize |bitmap_result|, so compute fallback icon style. On 2015/05/11 18:57:39, pkotwicz wrote: > Nit: |bitmap_result| -> |bitmap_result_| Done. https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:122: bool LargeIconWorker::ResizeLargeIconOnBackgroundThreadIfValid( Makes since now that the routine is not exposed. Done. https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.cc:192: scoped_refptr<LargeIconWorker> session = On 2015/05/11 18:57:39, pkotwicz wrote: > Nit: session -> worker Done. https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1122103003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service.h:10: #include "base/memory/ref_counted.h" On 2015/05/11 18:57:39, pkotwicz wrote: > Nit: The include can now be in the .cc file? Done.
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, jochen@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1122103003/#ps160001 (title: "Comment cleanup; making ResizeLargeIconOnBackgroundThreadIfValid() a member function.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1122103003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by huangs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1122103003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7f7237f4f12af522dafdadb0a0f789f88f579821 Cr-Commit-Position: refs/heads/master@{#329248}
Message was sent while issue was closed.
blundell@chromium.org changed reviewers: + blundell@chromium.org
Message was sent while issue was closed.
Sylvain happened to see this code today in the codebase and trace it back to this CL. //components/favicon/core shouldn't depend on //content. https://codereview.chromium.org/1122103003/diff/160001/components/favicon/cor... File components/favicon/core/DEPS (right): https://codereview.chromium.org/1122103003/diff/160001/components/favicon/cor... components/favicon/core/DEPS:5: "+content/public/browser", This is actually a dependency that shouldn't have been introduced here: see the README in //components/favicon. It should be easy to inject the SequencedWorkerPool into LargeIconService via the //chrome-level factory instead of using content::BrowserThread directly here. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
