Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
155273002:
Repurpose NotificationBitmapFetcher to BitmapFetcher (Closed)
|
Created:
6 years, 10 months ago by gone Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRepurpose NotificationBitmapFetcher to BitmapFetcher
There was nothing specific about this class, so I'm generalizing it and moving it upwards in the tree for more use cases (e.g. app banners).
Code that will use it is not ready for review, but is located at https://chromiumcodereview.appspot.com/156343002/
BUG=341556
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251308
Patch Set 1 #Patch Set 2 : indentation #
Total comments: 2
Patch Set 3 : Repurposing other class instead #Patch Set 4 : Prettifying #
Total comments: 6
Patch Set 5 : Addressing comments #
Total comments: 33
Patch Set 6 : Addressing comments #Patch Set 7 : Appease windows bot #Messages
Total messages: 33 (0 generated)
Hey thakis, newt mentioned that you looked at his image code for grabbing image for the NNNNNTP a while back. Does this CL look like something you'd be comfortable reviewing? It grabs an image, decodes it, and shunts it back to whoever the owns the fetcher. The code that uses this is in Android land, though, so there won't be any hooks from Chromium land :/
There's not much app icon-specific in this class, is there? It just fetches a URL and then decodes that as an image. Maybe you can give this a more general name and put it in a more general place? Maybe you can rename hrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc to the more general name and make clients of that use the more-generally-named thing too. (If you convert an in-tree client, there's a smaller chance someone will delete this due to being unused.) Do you have any plans for fetching different images based on screen resolution / size / display size? https://chromiumcodereview.appspot.com/155273002/diff/40001/chrome/browser/an... File chrome/browser/android/banners/app_banner_icon_fetcher.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/40001/chrome/browser/an... chrome/browser/android/banners/app_banner_icon_fetcher.cc:26: fetcher_.get()->Start(); http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_... https://chromiumcodereview.appspot.com/155273002/diff/40001/chrome/browser/an... chrome/browser/android/banners/app_banner_icon_fetcher.cc:47: ImageDecoder::DEFAULT_CODEC); (`git cl format` formats this as scoped_refptr<ImageDecoder> image_decoder = new ImageDecoder(this, image_data, ImageDecoder::DEFAULT_CODEC); which reads slightly nicer imho)
On 2014/02/05 05:53:17, Nico wrote: > There's not much app icon-specific in this class, is there? It just fetches a > URL and then decodes that as an image. Maybe you can give this a more general > name and put it in a more general place? > > Maybe you can rename > hrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc to the > more general name and make clients of that use the more-generally-named thing > too. (If you convert an in-tree client, there's a smaller chance someone will > delete this due to being unused.) > > Do you have any plans for fetching different images based on screen resolution / > size / display size? I ended up just changing my class to use the one you pointed out wholesale (thanks!). I've moved it upward to chrome/browser and renamed it to just BitmapFetcher. Fetching different images based on screen resolution is actually done higher: the URLs we pass in for app banners are Google FIFE URLs, which take the size parameter somewhere at the end of the URL. By the time it hits the fetcher, the size is already determined.
On 2014/02/05 09:55:46, dfalcantara wrote: > On 2014/02/05 05:53:17, Nico wrote: > > There's not much app icon-specific in this class, is there? It just fetches a > > URL and then decodes that as an image. Maybe you can give this a more general > > name and put it in a more general place? > > > > Maybe you can rename > > hrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc to > the > > more general name and make clients of that use the more-generally-named thing > > too. (If you convert an in-tree client, there's a smaller chance someone will > > delete this due to being unused.) > > > > Do you have any plans for fetching different images based on screen resolution > / > > size / display size? > > I ended up just changing my class to use the one you pointed out wholesale > (thanks!). I've moved it upward to chrome/browser and renamed it to just > BitmapFetcher. Fetching different images based on screen resolution is actually > done higher: the URLs we pass in for app banners are Google FIFE URLs, which > take the size parameter somewhere at the end of the URL. By the time it hits > the fetcher, the size is already determined. Ended up forcing the code that uses this to the Chromium repo, but it's not yet ready for review since everything it depends on is still up for review. https://chromiumcodereview.appspot.com/156343002/
The rietveld subject doesn't make it into the commit log, so please add "Repurpose NotificationBitmapFetcher to BitmapFetcher" as the first line to the description too. pkotwicz: Can you do the first pass at this, since it's moving your stuff around? Thanks!
On 2014/02/06 01:34:48, Nico wrote: > The rietveld subject doesn't make it into the commit log, so please add > "Repurpose NotificationBitmapFetcher to BitmapFetcher" as the first line to the > description too. > > pkotwicz: Can you do the first pass at this, since it's moving your stuff > around? Thanks! Ping?
I do not mind doing a review, however I am not the author of notification_bitmap_fetcher.* Those honors go to petewil@ https://chromiumcodereview.appspot.com/15295018
Overall it looks good to me. Apologies, but there are a few things that I left unfinished that we should probably look at before making this more general, including one bit of research and seeing why some tests are turned off. https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... File chrome/browser/bitmap_fetcher.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:29: void BitmapFetcher::OnURLFetchComplete(const net::URLFetcher* source) { One thing that I never finished investigating that we should probably double check before making this general purpose class. What happens if the fetch times out? I presume that URLFetcher handles the timeout, but I'm not certain, it would be good to make certain. https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... File chrome/browser/bitmap_fetcher.h (right): https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:20: // A delegate interface for users of BitmapFetcher. Should this be in a namespace somewhere? https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... File chrome/browser/bitmap_fetcher_browsertest.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... chrome/browser/bitmap_fetcher_browsertest.cc:77: #define MAYBE_StartTest DISABLED_StartTest Now might be a good time to see why the test fails on Windows.
https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... File chrome/browser/bitmap_fetcher_browsertest.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... chrome/browser/bitmap_fetcher_browsertest.cc:77: #define MAYBE_StartTest DISABLED_StartTest On 2014/02/11 03:41:11, Pete Williamson wrote: > Now might be a good time to see why the test fails on Windows. Not sure if I'm in the best position to debug this... I'm one of the Android devs so I don't actually have a Windows machine to test on :/
https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... File chrome/browser/bitmap_fetcher.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:29: void BitmapFetcher::OnURLFetchComplete(const net::URLFetcher* source) { On 2014/02/11 03:41:11, Pete Williamson wrote: > One thing that I never finished investigating that we should probably double > check before making this general purpose class. What happens if the fetch > times out? I presume that URLFetcher handles the timeout, but I'm not certain, > it would be good to make certain. Should be handled since functions like URLFetcherCore::RetryOrCompleteUrlFetch() exist. It always seems to end up informing the Delegate that it's stopped trying, which is why you check the status on line 32. https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... File chrome/browser/bitmap_fetcher.h (right): https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:20: // A delegate interface for users of BitmapFetcher. On 2014/02/11 03:41:11, Pete Williamson wrote: > Should this be in a namespace somewhere? Put it into the chrome namespace.
On 2014/02/11 05:32:06, dfalcantara wrote: > https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... > File chrome/browser/bitmap_fetcher.cc (right): > > https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... > chrome/browser/bitmap_fetcher.cc:29: void > BitmapFetcher::OnURLFetchComplete(const net::URLFetcher* source) { > On 2014/02/11 03:41:11, Pete Williamson wrote: > > One thing that I never finished investigating that we should probably double > > check before making this general purpose class. What happens if the fetch > > times out? I presume that URLFetcher handles the timeout, but I'm not > certain, > > it would be good to make certain. > > Should be handled since functions like URLFetcherCore::RetryOrCompleteUrlFetch() > exist. It always seems to end up informing the Delegate that it's stopped > trying, which is why you check the status on line 32. > > https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... > File chrome/browser/bitmap_fetcher.h (right): > > https://chromiumcodereview.appspot.com/155273002/diff/110001/chrome/browser/b... > chrome/browser/bitmap_fetcher.h:20: // A delegate interface for users of > BitmapFetcher. > On 2014/02/11 03:41:11, Pete Williamson wrote: > > Should this be in a namespace somewhere? > > Put it into the chrome namespace. Ping~
Sorry for the delay. LGTM
Thakis is apparently away now; +sky for chrome/OWNERS.
https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:15: : url_(url), delegate_(delegate) {} nit: since everything doesn't fit on one line, wrap each arg to its own line. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:20: url_fetcher_.reset(net::URLFetcher::Create(url_, net::URLFetcher::GET, this)); I believe the expectation is this is only invoked once. So, add a DCHECK that url_fetcher_ is NULL. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:22: // TODO(petewil): Make sure this is the right profile to use. Is this comment still relevant? Seems obvious that since a profile is supplied it's relevant to the profile. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:35: OnDecodeImageFailed(NULL); Less error prone if OnDecodeImageFailed and this call into a common method that doesn't take a ImageDecoder. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:41: // Create an ImageDecoder with the data and assign it to the refptr. This comment just describes the code and so isn't particularly helpful. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:42: image_decoder_ = I thought we decoded images in the renderer for two reasons. If there is a security exploit in a decoder it is isolated to the sandboxed renderer. Similarly if decoding crashes its only a renderer. I realize you don't have a renderer here, so it's worth pinging the security folks about this. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.h (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:23: class BitmapFetcherDelegate { nit: move into its own file. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:27: // match up the bitmap with a specific request. Document that bitmap may be NULL for either failure of download or failure to decode. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:34: class BitmapFetcher : public net::URLFetcherDelegate, Description? https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:40: GURL url() const { return url_; } const GURL& https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:42: // Start fetching the URL with the fetcher. The operation will be continued Will be continued? https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:74: scoped_ptr<SkBitmap> bitmap_; Why do we need this as a member? Why not declare it on the stack when needed?
petwil@: Do you know the answers to some of sky's implementation questions? I was simply aiming to move the code upward by two directories and don't know the specifics.
https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:42: image_decoder_ = On 2014/02/13 18:55:05, sky wrote: > I thought we decoded images in the renderer for two reasons. If there is a > security exploit in a decoder it is isolated to the sandboxed renderer. > Similarly if decoding crashes its only a renderer. I realize you don't have a > renderer here, so it's worth pinging the security folks about this. Doesn't the ImageDecoder run in a sandboxed process? At least its header suggests it.
Answers to some of Sky's questions. All of Sky's other feedback seems valid and actionable. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:22: // TODO(petewil): Make sure this is the right profile to use. On 2014/02/13 18:55:05, sky wrote: > Is this comment still relevant? Seems obvious that since a profile is supplied > it's relevant to the profile. It is OK to remove this comment. The feature is multi-profile, and the comment was to make sure we aren't fetching a bitmap for notifications for one profile from a different profile. As Sky points out, we are using a profile here, so that should no longer be a concern. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:42: image_decoder_ = On 2014/02/13 18:55:05, sky wrote: > I thought we decoded images in the renderer for two reasons. If there is a > security exploit in a decoder it is isolated to the sandboxed renderer. > Similarly if decoding crashes its only a renderer. I realize you don't have a > renderer here, so it's worth pinging the security folks about this. By all means check with security. We do in fact run on a browser thread in the calling code: // Call start to begin decoding. The ImageDecoder will call OnImageDecoded // with the data when it is done. scoped_refptr<base::MessageLoopProxy> task_runner = content::BrowserThread::GetMessageLoopProxyForThread( content::BrowserThread::UI); image_decoder_->Start(task_runner); If we are making this class more general, it might be nice to move the logic to call this in the browser process into this new, more general class to reduce the potential for abuse. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.h (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:42: // Start fetching the URL with the fetcher. The operation will be continued On 2014/02/13 18:55:05, sky wrote: > Will be continued? The idea here in the original code this came from was that we do the URL fetch on one thread, and the callback for the URLFetch then calls the image decode on a browser thread (in a browser process). Since that callback implementation is not in this source file, it might have been hard to tell. We should move the callback implementation into this file. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:74: scoped_ptr<SkBitmap> bitmap_; On 2014/02/13 18:55:05, sky wrote: > Why do we need this as a member? Why not declare it on the stack when needed? The intent was to use this object to both fetch and store the bitmap - once it was fetched, I wanted to come back to this object and retrieve it so I did not need to make another copy. If you think that is poor design, it wouldn't hurt the process to get rid of this member variable and just make copies (though it may degrade perf a bit). The general idea is one that I borrowed from the Win7 TCP/IP stack - don't copy the data more than necessary, and never if you can avoid it.
You are right. I didn't realize that. Nice! -Scott On Thu, Feb 13, 2014 at 11:28 AM, <dfalcantara@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... > File chrome/browser/bitmap_fetcher.cc (right): > > https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... > chrome/browser/bitmap_fetcher.cc:42: image_decoder_ = > On 2014/02/13 18:55:05, sky wrote: >> >> I thought we decoded images in the renderer for two reasons. If there > > is a >> >> security exploit in a decoder it is isolated to the sandboxed > > renderer. >> >> Similarly if decoding crashes its only a renderer. I realize you don't > > have a >> >> renderer here, so it's worth pinging the security folks about this. > > > Doesn't the ImageDecoder run in a sandboxed process? At least its > header suggests it. > > https://chromiumcodereview.appspot.com/155273002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.h (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:42: // Start fetching the URL with the fetcher. The operation will be continued On 2014/02/13 21:30:31, Pete Williamson wrote: > On 2014/02/13 18:55:05, sky wrote: > > Will be continued? > > The idea here in the original code this came from was that we do the URL fetch > on one thread, and the callback for the URLFetch then calls the image decode on > a browser thread (in a browser process). Since that callback implementation is > not in this source file, it might have been hard to tell. We should move the > callback implementation into this file. My comment was more that particular sentence is confusing and didn't really help things. I personally like: "Start fetching the URL with the fetcher. The delegate is notified asynchronously when done." https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:74: scoped_ptr<SkBitmap> bitmap_; On 2014/02/13 21:30:31, Pete Williamson wrote: > On 2014/02/13 18:55:05, sky wrote: > > Why do we need this as a member? Why not declare it on the stack when needed? > > The intent was to use this object to both fetch and store the bitmap - once it > was fetched, I wanted to come back to this object and retrieve it so I did not > need to make another copy. > > If you think that is poor design, it wouldn't hurt the process to get rid of > this member variable and just make copies (though it may degrade perf a bit). > > The general idea is one that I borrowed from the Win7 TCP/IP stack - don't copy > the data more than necessary, and never if you can avoid it. When you are not even exposing the bitmap publicly in this class (there is no getter for this) something tells me you are not actually using a design like you outline:) In general I prefer classes as stateless as possible. If the delegate wants to keep around the bitmap, they should copy it. As to the expense of copying skbitmaps, they are designed such that copy is cheap and explicitly support copy semantics. I would have pushed the delegate to take a const SkBitmap&, but you want NULL to be meaningful so it can't do that.
https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.h (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:42: // Start fetching the URL with the fetcher. The operation will be continued On 2014/02/13 23:12:30, sky wrote: > On 2014/02/13 21:30:31, Pete Williamson wrote: > > On 2014/02/13 18:55:05, sky wrote: > > > Will be continued? > > > > The idea here in the original code this came from was that we do the URL fetch > > on one thread, and the callback for the URLFetch then calls the image decode > on > > a browser thread (in a browser process). Since that callback implementation > is > > not in this source file, it might have been hard to tell. We should move the > > callback implementation into this file. > > My comment was more that particular sentence is confusing and didn't really help > things. I personally like: > "Start fetching the URL with the fetcher. The delegate is notified > asynchronously when done." OK, I agree that the new wording would be good. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:74: scoped_ptr<SkBitmap> bitmap_; On 2014/02/13 23:12:30, sky wrote: > On 2014/02/13 21:30:31, Pete Williamson wrote: > > On 2014/02/13 18:55:05, sky wrote: > > > Why do we need this as a member? Why not declare it on the stack when > needed? > > > > The intent was to use this object to both fetch and store the bitmap - once it > > was fetched, I wanted to come back to this object and retrieve it so I did not > > need to make another copy. > > > > If you think that is poor design, it wouldn't hurt the process to get rid of > > this member variable and just make copies (though it may degrade perf a bit). > > > > The general idea is one that I borrowed from the Win7 TCP/IP stack - don't > copy > > the data more than necessary, and never if you can avoid it. > > When you are not even exposing the bitmap publicly in this class (there is no > getter for this) something tells me you are not actually using a design like you > outline:) > > In general I prefer classes as stateless as possible. If the delegate wants to > keep around the bitmap, they should copy it. > > As to the expense of copying skbitmaps, they are designed such that copy is > cheap and explicitly support copy semantics. I would have pushed the delegate to > take a const SkBitmap&, but you want NULL to be meaningful so it can't do that. My bad, I think I was remembering another part of the design elsewhere. In that case, getting rid of the member function is fine.
I think I got most of the comments... https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.cc (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:15: : url_(url), delegate_(delegate) {} On 2014/02/13 18:55:05, sky wrote: > nit: since everything doesn't fit on one line, wrap each arg to its own line. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:20: url_fetcher_.reset(net::URLFetcher::Create(url_, net::URLFetcher::GET, this)); On 2014/02/13 18:55:05, sky wrote: > I believe the expectation is this is only invoked once. So, add a DCHECK that > url_fetcher_ is NULL. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:22: // TODO(petewil): Make sure this is the right profile to use. On 2014/02/13 21:30:31, Pete Williamson wrote: > On 2014/02/13 18:55:05, sky wrote: > > Is this comment still relevant? Seems obvious that since a profile is supplied > > it's relevant to the profile. > > It is OK to remove this comment. The feature is multi-profile, and the comment > was to make sure we aren't fetching a bitmap for notifications for one profile > from a different profile. As Sky points out, we are using a profile here, so > that should no longer be a concern. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:35: OnDecodeImageFailed(NULL); On 2014/02/13 18:55:05, sky wrote: > Less error prone if OnDecodeImageFailed and this call into a common method that > doesn't take a ImageDecoder. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:41: // Create an ImageDecoder with the data and assign it to the refptr. On 2014/02/13 18:55:05, sky wrote: > This comment just describes the code and so isn't particularly helpful. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.cc:42: image_decoder_ = On 2014/02/13 21:30:31, Pete Williamson wrote: > On 2014/02/13 18:55:05, sky wrote: > > I thought we decoded images in the renderer for two reasons. If there is a > > security exploit in a decoder it is isolated to the sandboxed renderer. > > Similarly if decoding crashes its only a renderer. I realize you don't have a > > renderer here, so it's worth pinging the security folks about this. > > By all means check with security. > > We do in fact run on a browser thread in the calling code: > // Call start to begin decoding. The ImageDecoder will call OnImageDecoded > // with the data when it is done. > scoped_refptr<base::MessageLoopProxy> task_runner = > content::BrowserThread::GetMessageLoopProxyForThread( > content::BrowserThread::UI); > image_decoder_->Start(task_runner); > > If we are making this class more general, it might be nice to move the logic to > call this in the browser process into this new, more general class to reduce the > potential for abuse. Should be fine, according to a reply later on. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... File chrome/browser/bitmap_fetcher.h (right): https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:23: class BitmapFetcherDelegate { On 2014/02/13 18:55:05, sky wrote: > nit: move into its own file. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:27: // match up the bitmap with a specific request. On 2014/02/13 18:55:05, sky wrote: > Document that bitmap may be NULL for either failure of download or failure to > decode. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:34: class BitmapFetcher : public net::URLFetcherDelegate, On 2014/02/13 18:55:05, sky wrote: > Description? Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:40: GURL url() const { return url_; } On 2014/02/13 18:55:05, sky wrote: > const GURL& Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:42: // Start fetching the URL with the fetcher. The operation will be continued On 2014/02/13 23:27:06, Pete Williamson wrote: > On 2014/02/13 23:12:30, sky wrote: > > On 2014/02/13 21:30:31, Pete Williamson wrote: > > > On 2014/02/13 18:55:05, sky wrote: > > > > Will be continued? > > > > > > The idea here in the original code this came from was that we do the URL > fetch > > > on one thread, and the callback for the URLFetch then calls the image decode > > on > > > a browser thread (in a browser process). Since that callback implementation > > is > > > not in this source file, it might have been hard to tell. We should move > the > > > callback implementation into this file. > > > > My comment was more that particular sentence is confusing and didn't really > help > > things. I personally like: > > "Start fetching the URL with the fetcher. The delegate is notified > > asynchronously when done." > > OK, I agree that the new wording would be good. Done. https://chromiumcodereview.appspot.com/155273002/diff/310001/chrome/browser/b... chrome/browser/bitmap_fetcher.h:74: scoped_ptr<SkBitmap> bitmap_; On 2014/02/13 23:27:06, Pete Williamson wrote: > On 2014/02/13 23:12:30, sky wrote: > > On 2014/02/13 21:30:31, Pete Williamson wrote: > > > On 2014/02/13 18:55:05, sky wrote: > > > > Why do we need this as a member? Why not declare it on the stack when > > needed? > > > > > > The intent was to use this object to both fetch and store the bitmap - once > it > > > was fetched, I wanted to come back to this object and retrieve it so I did > not > > > need to make another copy. > > > > > > If you think that is poor design, it wouldn't hurt the process to get rid of > > > this member variable and just make copies (though it may degrade perf a > bit). > > > > > > The general idea is one that I borrowed from the Win7 TCP/IP stack - don't > > copy > > > the data more than necessary, and never if you can avoid it. > > > > When you are not even exposing the bitmap publicly in this class (there is no > > getter for this) something tells me you are not actually using a design like > you > > outline:) > > > > In general I prefer classes as stateless as possible. If the delegate wants to > > keep around the bitmap, they should copy it. > > > > As to the expense of copying skbitmaps, they are designed such that copy is > > cheap and explicitly support copy semantics. I would have pushed the delegate > to > > take a const SkBitmap&, but you want NULL to be meaningful so it can't do > that. > > My bad, I think I was remembering another part of the design elsewhere. In that > case, getting rid of the member function is fine. Done.
LGTM
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/155273002/760001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/155273002/760001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/155273002/760001
Message was sent while issue was closed.
Change committed as 251308 |
