Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
787903003:
Upstream image_fetcher::ImageFetcher (Closed)
DescriptionUpstream image_fetcher::ImageFetcher
This class is a wrapper around net::URLFetcherDelegate that is designed
specifically to fetch images. It will transcode WebP encoded images.
BUG=436897, 441327
Committed: https://crrev.com/80483075c0039a6c312ed99891bcefe52f79cb92
Cr-Commit-Position: refs/heads/master@{#308116}
Patch Set 1 #Patch Set 2 : git cl upload --no-find-copies, fix indentation and minor cleanup #
Total comments: 1
Patch Set 3 : Address comments #Patch Set 4 : Fix compilation with Xcode 5.1 by adding ugly casts #
Total comments: 37
Messages
Total messages: 23 (8 generated)
sdefresne@chromium.org changed reviewers: + droger@chromium.org, rsleevi@chromium.org
droger: please take a look rsleevi: OWNERS for new dependency on //net
LGTM I did not review the image_fetcher code in detail, since it is direct upstreaming. I did review the other files. The unit tests will not be run by the bots yet, right? https://codereview.chromium.org/787903003/diff/20001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher_unittest.mm (right): https://codereview.chromium.org/787903003/diff/20001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:23: static unsigned char jpgImage[] = { Nit: kJPGImage, and same below.
On 2014/12/11 at 16:28:32, droger wrote: > LGTM > > I did not review the image_fetcher code in detail, since it is direct upstreaming. > I did review the other files. > > The unit tests will not be run by the bots yet, right? No, created http://crbug.com/441327 to ask the test to be added to the bots once this CL land. > https://codereview.chromium.org/787903003/diff/20001/ios/chrome/browser/net/i... > File ios/chrome/browser/net/image_fetcher_unittest.mm (right): > > https://codereview.chromium.org/787903003/diff/20001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher_unittest.mm:23: static unsigned char jpgImage[] = { > Nit: kJPGImage, and same below. Done, and also removed a static GURL.
sdefresne@chromium.org changed reviewers: + mef@chromium.org - rsleevi@chromium.org
-rsleevi (overloaded), +mef: need OWNERS approval for adding DEPS on //net
lgtm for adding DEPS on //net
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787903003/40001
The CQ bit was checked by sdefresne@chromium.org
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787903003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787903003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/80483075c0039a6c312ed99891bcefe52f79cb92 Cr-Commit-Position: refs/heads/master@{#308116}
Message was sent while issue was closed.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Message was sent while issue was closed.
A number of issues for follow-up. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher.h (right): https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:26: typedef void (^Callback)(const GURL& url, int http_response_code, NSData* data); nit: Naming - while it's totally ok to call this callback because it's namespaced in image_fetcher, this sort of shadowing may be overloading the ubiquitous base::Callback when people read the code. I'd nominally nit to suggest a different name, but this is legal syntax, just... a tad confusing. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:31: // processing with the exception of WebP encoded images. Those are decoded and s/processing with/processing, with/ https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:38: const scoped_refptr<base::SequencedWorkerPool> decoding_pool); Pass as const-ref, not const. Avoid refcount churn. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:38: const scoped_refptr<base::SequencedWorkerPool> decoding_pool); DESIGN: Prefer passing in a SequencedTaskRunner, rather than a worker pool. Only use pools if you're creating them yourself. You can get a SequencedTaskRunner from a given pool by creating an STR for a token you supply. This allows callers to avoid instantiating a pool if they would rather use a dedicated/common thread, and makes it easier for tests (which can then just test on the main thread). https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:46: // (virtual for testing) None of your tests appear to override these methods. Unnecessary, it would seem? https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:59: net::URLRequestContextGetter* request_context_getter); Since you're going to take a reference to the URCG, you should pass the URCG as a "const scoped_refptr<>&" to better express the ownership transfer-share happening here. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher.mm (right): https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:93: net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES); | net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN ? https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:102: url, callback, "", net::URLRequest::NEVER_CLEAR_REFERRER); s/""/std::string()/ https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:114: // Ensures that |fetcher| will be deleted even if we return early. nit: Pronouns in comments considered harmful https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... // Ensure that |fetcher| will be deleted in the event of early return. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:118: // return early. https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... // Retrieve the callback and ensure it will be deleted in the event of early return. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:126: // directly in the URL). In that case, we set the response code to 200. https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... In that case, set the response code to 200 (OK). https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:149: if (mime_type == "image/webp") { Should the constant be a static const char[] kConstant ? https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:161: (callback.get())(original_url, http_response_code, data); My gut instinct is a little nervous here about avoiding any potential reentrancy issues, which URLFetcher is prone to causing people to introduce. My typical mitigation is to force a PostTask - but I'm not sure if the ^Callback semantics effectively ensure that? https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher_unittest.mm (right): https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:62: static char kTestUrl[] = "http://www.img.com"; s/char/const char/ https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:69: : pool_(new base::SequencedWorkerPool(1, "TestPool")), Do you need to actually spin up a dedicated pool for this test? Can you not just pass in your own SequencedTaskRunner? https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:80: base::MessageLoopProxy::current())); DESIGN: prefer base::ThreadTaskRunnerHandle::Get() to MessageLoopProxy::current() One of the biggest differences between these two is that MLP::current() will return NULL if no associated message loop, whereas TTRH will DCHECK. I have seen countless bugs in test and production come up with MLP::current() is NULL, so it's much easier to ensure a guarantee. Plus, it fits with deprecating the MLP in favour of the TTRH (which is primarily about ensuring Chromium code runs in a variety of environments, such as NaCl, ChromeOS, etc). Yes, not strictly applicable here, but better as a pattern to avoid the deprecated thing. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:87: EXPECT_EQ(static_cast<UIImage*>(nil), result_); nullptr rather than nil here? Or is the difference semantically meaningful? nullptr let's you avoid the cast, IINM, because it's a distinguishable type. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:170: std::string kZero = std::string("\0", 1); s/kZero/kNull However, the construction of the header string is needlessly complex. std::string header_string("HTTP/1.1 200 OK\0" "Content-type: image/webp\0" "\0"); avoid unnecessary concats. Ditto line 153-154, 136-137, etc.
Message was sent while issue was closed.
On 2014/12/12 at 23:32:01, rsleevi wrote: > A number of issues for follow-up. > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > File ios/chrome/browser/net/image_fetcher.h (right): > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.h:26: typedef void (^Callback)(const GURL& url, int http_response_code, NSData* data); > nit: Naming - while it's totally ok to call this callback because it's namespaced in image_fetcher, this sort of shadowing may be overloading the ubiquitous base::Callback when people read the code. I'd nominally nit to suggest a different name, but this is legal syntax, just... a tad confusing. > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.h:31: // processing with the exception of WebP encoded images. Those are decoded and > s/processing with/processing, with/ > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.h:38: const scoped_refptr<base::SequencedWorkerPool> decoding_pool); > Pass as const-ref, not const. Avoid refcount churn. > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.h:38: const scoped_refptr<base::SequencedWorkerPool> decoding_pool); > DESIGN: Prefer passing in a SequencedTaskRunner, rather than a worker pool. Only use pools if you're creating them yourself. > > You can get a SequencedTaskRunner from a given pool by creating an STR for a token you supply. > > This allows callers to avoid instantiating a pool if they would rather use a dedicated/common thread, and makes it easier for tests (which can then just test on the main thread). > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.h:46: // (virtual for testing) > None of your tests appear to override these methods. Unnecessary, it would seem? > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.h:59: net::URLRequestContextGetter* request_context_getter); > Since you're going to take a reference to the URCG, you should pass the URCG as a "const scoped_refptr<>&" to better express the ownership transfer-share happening here. > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > File ios/chrome/browser/net/image_fetcher.mm (right): > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.mm:93: net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES); > | net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN ? > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.mm:102: url, callback, "", net::URLRequest::NEVER_CLEAR_REFERRER); > s/""/std::string()/ > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.mm:114: // Ensures that |fetcher| will be deleted even if we return early. > nit: Pronouns in comments considered harmful > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > // Ensure that |fetcher| will be deleted in the event of early return. > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.mm:118: // return early. > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > // Retrieve the callback and ensure it will be deleted in the event of early return. > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.mm:126: // directly in the URL). In that case, we set the response code to 200. > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > In that case, set the response code to 200 (OK). > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.mm:149: if (mime_type == "image/webp") { > Should the constant be a static const char[] kConstant ? > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher.mm:161: (callback.get())(original_url, http_response_code, data); > My gut instinct is a little nervous here about avoiding any potential reentrancy issues, which URLFetcher is prone to causing people to introduce. > > My typical mitigation is to force a PostTask - but I'm not sure if the ^Callback semantics effectively ensure that? > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > File ios/chrome/browser/net/image_fetcher_unittest.mm (right): > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher_unittest.mm:62: static char kTestUrl[] = "http://www.img.com"; > s/char/const char/ > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher_unittest.mm:69: : pool_(new base::SequencedWorkerPool(1, "TestPool")), > Do you need to actually spin up a dedicated pool for this test? Can you not just pass in your own SequencedTaskRunner? > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher_unittest.mm:80: base::MessageLoopProxy::current())); > DESIGN: prefer base::ThreadTaskRunnerHandle::Get() to MessageLoopProxy::current() > > One of the biggest differences between these two is that MLP::current() will return NULL if no associated message loop, whereas TTRH will DCHECK. I have seen countless bugs in test and production come up with MLP::current() is NULL, so it's much easier to ensure a guarantee. Plus, it fits with deprecating the MLP in favour of the TTRH (which is primarily about ensuring Chromium code runs in a variety of environments, such as NaCl, ChromeOS, etc). Yes, not strictly applicable here, but better as a pattern to avoid the deprecated thing. > > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher_unittest.mm:87: EXPECT_EQ(static_cast<UIImage*>(nil), result_); > nullptr rather than nil here? Or is the difference semantically meaningful? > > nullptr let's you avoid the cast, IINM, because it's a distinguishable type. iOS code downstream style is to use "nil" for null pointers to Objective-C objects and "nullptr" for null pointers to C++ objects. This help make it clear which memory management model to use for the variable and to keep consistent. With Xcode 6.1 "nil" is defined to be equal to "nullptr" and the code compiled cleanly in the downstream tree. However, when I upstreamed this code, I discovered that iOS bots upstream were running Xcode 5.1 where "nil" is a macro expanding to "NULL" i.e. 0 which is not distinguishable. I've opted for the static cast so that the code compiled on the bots util they have been updated to keep consistency in the usage of "nil" / "nullptr". I'll put a // TODO(ios): remove those casts once https://code.google.com/p/chromium/issues/detail?id=440857 is fixed. > https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... > ios/chrome/browser/net/image_fetcher_unittest.mm:170: std::string kZero = std::string("\0", 1); > s/kZero/kNull > > However, the construction of the header string is needlessly complex. > > std::string header_string("HTTP/1.1 200 OK\0" > "Content-type: image/webp\0" > "\0"); > > avoid unnecessary concats. Ditto line 153-154, 136-137, etc. Thank you for all the comments. I'll address them in a followup CL once this CL has been merged downstream (so that we don't make the merge more complex).
Message was sent while issue was closed.
Followup CL: https://codereview.chromium.org/805713004 https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher.h (right): https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:26: typedef void (^Callback)(const GURL& url, int http_response_code, NSData* data); On 2014/12/12 at 23:32:00, Ryan Sleevi wrote: > nit: Naming - while it's totally ok to call this callback because it's namespaced in image_fetcher, this sort of shadowing may be overloading the ubiquitous base::Callback when people read the code. I'd nominally nit to suggest a different name, but this is legal syntax, just... a tad confusing. Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:31: // processing with the exception of WebP encoded images. Those are decoded and On 2014/12/12 at 23:32:00, Ryan Sleevi wrote: > s/processing with/processing, with/ Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:38: const scoped_refptr<base::SequencedWorkerPool> decoding_pool); On 2014/12/12 at 23:32:00, Ryan Sleevi wrote: > Pass as const-ref, not const. Avoid refcount churn. Done. > DESIGN: Prefer passing in a SequencedTaskRunner, rather than a worker pool. Only use pools if you're creating them yourself. > > You can get a SequencedTaskRunner from a given pool by creating an STR for a token you supply. > > This allows callers to avoid instantiating a pool if they would rather use a dedicated/common thread, and makes it easier for tests (which can then just test on the main thread). We want a pool since some of our clients uses a single ImageFetcher to fetch multiple images and we'd like the decoding to be done in parallel if possible (i.e. if the pool has multiple thread). We cannot use web::WebThread::GetBlockingPool() since it is not initialized during unit tests (need to create web::TestWebThread and web::TestWebThreadBundle for that). Created http://crbug.com/442292 to track this. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:46: // (virtual for testing) On 2014/12/12 at 23:32:00, Ryan Sleevi wrote: > None of your tests appear to override these methods. Unnecessary, it would seem? The methods are overridden in downstream tests that want to mock ImageFetcher. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:59: net::URLRequestContextGetter* request_context_getter); On 2014/12/12 at 23:32:00, Ryan Sleevi wrote: > Since you're going to take a reference to the URCG, you should pass the URCG as a "const scoped_refptr<>&" to better express the ownership transfer-share happening here. Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher.mm (right): https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:93: net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES); On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > | net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN ? Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:93: net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES); On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > | net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN ? Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:102: url, callback, "", net::URLRequest::NEVER_CLEAR_REFERRER); On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > s/""/std::string()/ Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:114: // Ensures that |fetcher| will be deleted even if we return early. On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > nit: Pronouns in comments considered harmful > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > // Ensure that |fetcher| will be deleted in the event of early return. Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:114: // Ensures that |fetcher| will be deleted even if we return early. On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > nit: Pronouns in comments considered harmful > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > // Ensure that |fetcher| will be deleted in the event of early return. Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:118: // return early. On 2014/12/12 at 23:32:00, Ryan Sleevi wrote: > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > // Retrieve the callback and ensure it will be deleted in the event of early return. Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:126: // directly in the URL). In that case, we set the response code to 200. On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > In that case, set the response code to 200 (OK). Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.mm:149: if (mime_type == "image/webp") { On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > Should the constant be a static const char[] kConstant ? Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher_unittest.mm (right): https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:62: static char kTestUrl[] = "http://www.img.com"; On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > s/char/const char/ Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:69: : pool_(new base::SequencedWorkerPool(1, "TestPool")), On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > Do you need to actually spin up a dedicated pool for this test? Can you not just pass in your own SequencedTaskRunner? //ios/web does not have a test helper that create the web::WebThread blocking pool, so for the moment it is simpler to create our own pool. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:80: base::MessageLoopProxy::current())); On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > DESIGN: prefer base::ThreadTaskRunnerHandle::Get() to MessageLoopProxy::current() > > One of the biggest differences between these two is that MLP::current() will return NULL if no associated message loop, whereas TTRH will DCHECK. I have seen countless bugs in test and production come up with MLP::current() is NULL, so it's much easier to ensure a guarantee. Plus, it fits with deprecating the MLP in favour of the TTRH (which is primarily about ensuring Chromium code runs in a variety of environments, such as NaCl, ChromeOS, etc). Yes, not strictly applicable here, but better as a pattern to avoid the deprecated thing. Done. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:87: EXPECT_EQ(static_cast<UIImage*>(nil), result_); On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > nullptr rather than nil here? Or is the difference semantically meaningful? > > nullptr let's you avoid the cast, IINM, because it's a distinguishable type. iOS downstream convention is to use "nil" for pointers to Objective-C types and "nullptr" for pointers to C++ types. Here we have a pointer to a UIImage, so an Objective-C object. When compiled with Xcode 6.0 or later, this is a distinguishable type too (and the code compile cleanly), however some bots are still running Xcode 5.1 (http://crbug.com/440857) so until they have been upgrade I need the casts. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher_unittest.mm:170: std::string kZero = std::string("\0", 1); On 2014/12/12 at 23:32:01, Ryan Sleevi wrote: > s/kZero/kNull > > However, the construction of the header string is needlessly complex. > > std::string header_string("HTTP/1.1 200 OK\0" > "Content-type: image/webp\0" > "\0"); > > avoid unnecessary concats. Ditto line 153-154, 136-137, etc. Done using std::string(const char*, size_t) constructor and arraysize() to get the length of the string containing NUL bytes (as std::string(const char*) stops at the first NUL byte).
Message was sent while issue was closed.
Consider one suggestion, below, in response to the use of SequencedWorkerPool. This is more a design nit than anything, but I just wanted to make sure the justification was understood. https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... File ios/chrome/browser/net/image_fetcher.h (right): https://codereview.chromium.org/787903003/diff/60001/ios/chrome/browser/net/i... ios/chrome/browser/net/image_fetcher.h:38: const scoped_refptr<base::SequencedWorkerPool> decoding_pool); On 2014/12/15 13:11:58, sdefresne wrote: > On 2014/12/12 at 23:32:00, Ryan Sleevi wrote: > > Pass as const-ref, not const. Avoid refcount churn. > > Done. > > > DESIGN: Prefer passing in a SequencedTaskRunner, rather than a worker pool. > Only use pools if you're creating them yourself. > > > > You can get a SequencedTaskRunner from a given pool by creating an STR for a > token you supply. > > > > This allows callers to avoid instantiating a pool if they would rather use a > dedicated/common thread, and makes it easier for tests (which can then just test > on the main thread). > > We want a pool since some of our clients uses a single ImageFetcher to fetch > multiple images and we'd like the decoding to be done in parallel if possible > (i.e. if the pool has multiple thread). We cannot use > web::WebThread::GetBlockingPool() since it is not initialized during unit tests > (need to create web::TestWebThread and web::TestWebThreadBundle for that). > Created http://crbug.com/442292 to track this. If you want requests to happen serially, but on any thread, you can use the SequencedTaskRunner from the SWP with a given token. If you want requests to happen in parallel, then you can just pass in a TaskRunner (which a SequencedWorkerPool is). Tasks may be started at any time (e.g. while other tasks are running), which matches your stated preference. Both of these interfaces avoid needing to directly depend on a SequencedWorkerPool. That is, you should consider SequencedWorkerPool to be an implementation detail, and instead code against the interfaces that express the API contract you want. SequencedWorkerPool may be one way to fill that, but it doesn't have to be the only way. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
