Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1159363002:
[Download Notification] Show preview if downloaded file is image (Closed)
Description[Download Notification] Show preview if downloaded file is image
This patch adds a preview to completion notification if the downloaded file is an png or jepg image.
BUG=468561
TEST=unit_tests and browser_tests pass
Committed: https://crrev.com/8719742f50d58e17c81fdf2fd427f3a2fbb847e9
Cr-Commit-Position: refs/heads/master@{#335023}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Replace DCHECK(CurrentlyON(...)) -> DCHECK_CURRENTLY_ON(...) #Patch Set 3 : Use ImageLoader #
Total comments: 10
Patch Set 4 : Adressed comment #10 #
Total comments: 14
Patch Set 5 : Addressed comments #12 #
Total comments: 1
Patch Set 6 : Read file content in the browser process #
Total comments: 17
Patch Set 7 : Addressed comments #27 #
Total comments: 2
Patch Set 8 : Addressed comment #31 #
Total comments: 2
Patch Set 9 : . #
Total comments: 3
Patch Set 10 : Addressed comments #41 #
Total comments: 10
Patch Set 11 : Addressed comment #44 and #45 #Patch Set 12 : rebase & use embedded server #
Messages
Total messages: 51 (17 generated)
Patchset #1 (id:1) has been deleted
yoshiki@chromium.org changed reviewers: + asanka@chromium.org
Asanka, could you take a look? Thanks.
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
This is a nice feature, but we generally go out of our way not to do any image decoding in the browser process due to security reasons. An exploit of our image decoding logic would effectively bypass all our sandboxing. I'll work with you to figure out a better solution tomorrow when I get in. https://codereview.chromium.org/1159363002/diff/60001/chrome/browser/download... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_item.cc:45: CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); Use DCHECK_CURRENTLY_ON instead.
I agree that avoiding potential security vulnerability is generally better. I assume other image decoding (eg. decoding favicon, extension icon, etc) should be done out of the browser process and we can do the same way. I'll check the code. Can you let me know if you know or find a good way? Thanks. https://codereview.chromium.org/1159363002/diff/60001/chrome/browser/download... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_item.cc:45: CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); On 2015/06/03 03:02:04, asanka wrote: > Use DCHECK_CURRENTLY_ON instead. Done.
On 2015/06/04 at 04:13:22, yoshiki wrote: > I agree that avoiding potential security vulnerability is generally better. I assume other image decoding (eg. decoding favicon, extension icon, etc) should be done out of the browser process and we can do the same way. I'll check the code. > Favicons are special since they tend to have an associated web_contents(). Favicon decoded is done in the renderer process and the resulting bitmaps are passed over to the browser process. > Can you let me know if you know or find a good way? Thanks. See chrome/browser/image_decoder.h. It uses the utility process to decode images in a sandbox. > https://codereview.chromium.org/1159363002/diff/60001/chrome/browser/download... > File chrome/browser/download/notification/download_notification_item.cc (right): > > https://codereview.chromium.org/1159363002/diff/60001/chrome/browser/download... > chrome/browser/download/notification/download_notification_item.cc:45: CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); > On 2015/06/03 03:02:04, asanka wrote: > > Use DCHECK_CURRENTLY_ON instead. > > Done.
> Favicons are special since they tend to have an associated web_contents(). > Favicon decoded is done in the renderer process and the resulting bitmaps are > passed over to the browser process. > > > Can you let me know if you know or find a good way? Thanks. > > See chrome/browser/image_decoder.h. It uses the utility process to decode images > in a sandbox. Thank you for letting me know that. I changed the code using the ImageDecoder. PTAL again?
https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:54: if (data.size() > kMaxImagePreviewSize) Check the size of the file rather than checking the size of |data|. https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:353: if (net::ParseMimeTypeWithoutParameter( Use mime_util::IsSupportedImageMimeType() https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:360: std::string dot_extension = item_->GetTargetFilePath().FinalExtension(); Use net::GetMimeTypeFromFile(). Note that this can cause IO. So ideally, you'd pass the MIMe type to ReadNotificationImage() and have it deal with MIME types. https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:415: // TODO(yoshiki): Set option to reduce the image size to supress memory usage. No. We should be passing a file handle over to the utility process and have it read + decode + resize the image before sending us the image bits. See SandboxedZipAnalyzer for example. https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:416: ImageDecoder::Start(this, image_data); You should call ImageDecoder::Cancel() if the notification item is going away.
Asanka, PTAL. Thanks. https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:54: if (data.size() > kMaxImagePreviewSize) On 2015/06/09 00:13:38, asanka wrote: > Check the size of the file rather than checking the size of |data|. The file size is checked at l.336 in the patchset #4. https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:353: if (net::ParseMimeTypeWithoutParameter( On 2015/06/09 00:13:38, asanka wrote: > Use mime_util::IsSupportedImageMimeType() Done. https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:360: std::string dot_extension = item_->GetTargetFilePath().FinalExtension(); On 2015/06/09 00:13:38, asanka wrote: > Use net::GetMimeTypeFromFile(). Note that this can cause IO. So ideally, you'd > pass the MIMe type to ReadNotificationImage() and have it deal with MIME types. I think we don't need to check the file contents for all downloaded file. Just checking the extension is enough. WDYT? https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:415: // TODO(yoshiki): Set option to reduce the image size to supress memory usage. On 2015/06/09 00:13:38, asanka wrote: > No. We should be passing a file handle over to the utility process and have it > read + decode + resize the image before sending us the image bits. > > See SandboxedZipAnalyzer for example. Done. Resizing will be implemented later. https://codereview.chromium.org/1159363002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:416: ImageDecoder::Start(this, image_data); On 2015/06/09 00:13:38, asanka wrote: > You should call ImageDecoder::Cancel() if the notification item is going away. Done.
Getting close. Thanks! And sorry about the delay in review. You should add a suitable owner for the ImageDecoder bits. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_browsertest.cc:505: ASSERT_TRUE(test_server()->Start()); Note that we are trying to get rid of spawned test server. We should start moving these tests over to EmbeddedTestServer. Not for this CL. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:28: #include "third_party/skia/include/core/SkBitmap.h" Nit: Already included in download_notification_item.h. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:120: DownloadNotificationItem::~DownloadNotificationItem() { It seems OnNotificationClose() must always be received before this object is destroyed. Correct? If so, we could DCHECK that there is no image decode request active. If there is, we'd get a call through a freed pointer. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:221: CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); DCHECK_CURRENTLY_ON https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... chrome/browser/image_decoder.cc:93: base::File::FLAG_OPEN | base::File::FLAG_READ); Indentation is off. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... File chrome/browser/image_decoder.h (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... chrome/browser/image_decoder.h:106: void DecodeImageInSandbox(int request_id, I'll defer to the owners. However, for both DecodeImageInSandbox() and StartWithOptionsImpl() different subsets of parameters are valid. It's probably a good sign that these should be split. https://codereview.chromium.org/1159363002/diff/120001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:306: file.Read(0, reinterpret_cast<char*>(image_data.data()), file.GetLength()); Fail early if reading failed.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
yoshiki@chromium.org changed reviewers: + glevin@chromium.org
Asanka, PTAL at the while code? Greg, PTAL at changes in ImageDecoder and ChromeContentUtilityClienty? Thanks. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_browsertest.cc:505: ASSERT_TRUE(test_server()->Start()); On 2015/06/11 03:30:51, asanka wrote: > Note that we are trying to get rid of spawned test server. We should start > moving these tests over to EmbeddedTestServer. Not for this CL. Acknowledged. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:28: #include "third_party/skia/include/core/SkBitmap.h" On 2015/06/11 03:30:51, asanka wrote: > Nit: Already included in download_notification_item.h. Done. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:120: DownloadNotificationItem::~DownloadNotificationItem() { On 2015/06/11 03:30:51, asanka wrote: > It seems OnNotificationClose() must always be received before this object is > destroyed. Correct? If so, we could DCHECK that there is no image decode request > active. > > If there is, we'd get a call through a freed pointer. Done. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:221: CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2015/06/11 03:30:51, asanka wrote: > DCHECK_CURRENTLY_ON Done. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... chrome/browser/image_decoder.cc:93: base::File::FLAG_OPEN | base::File::FLAG_READ); On 2015/06/11 03:30:51, asanka wrote: > Indentation is off. Done. https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... File chrome/browser/image_decoder.h (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/browser/image_d... chrome/browser/image_decoder.h:106: void DecodeImageInSandbox(int request_id, On 2015/06/11 03:30:52, asanka wrote: > I'll defer to the owners. > > However, for both DecodeImageInSandbox() and StartWithOptionsImpl() different > subsets of parameters are valid. It's probably a good sign that these should be > split. Acknowledged. https://codereview.chromium.org/1159363002/diff/120001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/1159363002/diff/120001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:306: file.Read(0, reinterpret_cast<char*>(image_data.data()), file.GetLength()); On 2015/06/11 03:30:52, asanka wrote: > Fail early if reading failed. Done.
/download/ lgtm https://codereview.chromium.org/1159363002/diff/180001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1159363002/diff/180001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_browsertest.cc:504: IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, DownloadImageFile) { In addition, we'd need tests for a file with an image/.. MIME type, which doesn't have the correct extension for that MIME type. The image preview logic works for this case, but interestingly, the resulting file may not open.
Hi Yoshiki. Seems okay as far as I can tell, but since I'm not an owner for these (or even a committer yet), and only worked on these files once, I'm not sure I'm a good choice for a review.
yoshiki@chromium.org changed reviewers: + sky@chromium.org
yoshiki@chromium.org changed reviewers: + thestig@chromium.org - sky@chromium.org
Greg, Thanks you for your comment. I asked review because you have originally introduced this feature. Lei, PTAL at image_decoder.[h|cc] and chrome_content_utility_client.[h|cc]? Thanks.
On 2015/06/12 01:33:15, yoshiki wrote: > Greg, Thanks you for your comment. I asked review because you have originally > introduced this feature. > > Lei, PTAL at image_decoder.[h|cc] and chrome_content_utility_client.[h|cc]? > Thanks. Why add additional complexity into ImageDecoder? You'll ultimately read the file anyway, so why not just read it in the browser on an appropriate thread and pass it off to the utility process like everyone else?
On 2015/06/12 01:47:16, Lei Zhang wrote: > On 2015/06/12 01:33:15, yoshiki wrote: > > Greg, Thanks you for your comment. I asked review because you have originally > > introduced this feature. > > > > Lei, PTAL at image_decoder.[h|cc] and chrome_content_utility_client.[h|cc]? > > Thanks. > > Why add additional complexity into ImageDecoder? You'll ultimately read the file > anyway, so why not just read it in the browser on an appropriate thread and pass > it off to the utility process like everyone else? Ok, I'll change the code to read the file content in the FILE thread in the browser process. And I have a question. Later, I'm planning to add resizing the image after reading it. Can we do image-resizing in the browser process? Or should it be in the separated process by modifying ImageDecoder and ChromeContentUtility?
On 2015/06/12 02:43:22, yoshiki wrote: > And I have a question. Later, I'm planning to add resizing the image after > reading it. Can we do image-resizing in the browser process? Or should it be in > the separated process by modifying ImageDecoder and ChromeContentUtility? Utility process. Don't deal with non-trivial untrusted data in the browser.
On 2015/06/12 04:12:03, Lei Zhang wrote: > On 2015/06/12 02:43:22, yoshiki wrote: > > And I have a question. Later, I'm planning to add resizing the image after > > reading it. Can we do image-resizing in the browser process? Or should it be > in > > the separated process by modifying ImageDecoder and ChromeContentUtility? > > Utility process. Don't deal with non-trivial untrusted data in the browser. Got it. Thnak you!
On 2015/06/12 04:14:32, yoshiki wrote: > On 2015/06/12 04:12:03, Lei Zhang wrote: > > On 2015/06/12 02:43:22, yoshiki wrote: > > > And I have a question. Later, I'm planning to add resizing the image after > > > reading it. Can we do image-resizing in the browser process? Or should it be > > in > > > the separated process by modifying ImageDecoder and ChromeContentUtility? > > > > Utility process. Don't deal with non-trivial untrusted data in the browser. > > Got it. Thnak you! Asanka, I changed it back to read the file content in the browser process instead of separated process, in order to reduce the unnecessary change, according to the comment #24. PTAL again? Thanks.
https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:43: std::string ReadNotificationImage(const base::FilePath file_path) { const ref. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:46: gfx::Image image; Unused? https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:53: if (data.size() > kMaxImagePreviewSize) Reading this function by itself, I want to say: You may want to just check the file size first before reading it. Otherwise, you can end up reading a very large file, only to throw it away. However, it looks like you are trying to only do this for download items that are less than 10 MB, so, in practice, this should never happen? https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:375: content::BrowserThread::FILE, Use the blocking thread pool instead of the FILE thread? (Use base::PostTaskAndReplyWithResult()) https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.h (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:106: void OnImageLoaded(std::string image_data); pass by const ref. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:71: static base::FilePath kDownloadItemTargetPath("/tmp/TITLE.bin"); You don't need static in an anonymous namespace. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:71: static base::FilePath kDownloadItemTargetPath("/tmp/TITLE.bin"); This generates a static initializer. If this wasn't a test, the sizes step on the buildbots would fail. The solution here is to use a POD string of type base::FilePath::StringType, and wrapper it in base::FilePath() when used below.
Thanks. PTAL. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:43: std::string ReadNotificationImage(const base::FilePath file_path) { On 2015/06/12 07:26:37, Lei Zhang wrote: > const ref. Done. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:46: gfx::Image image; On 2015/06/12 07:26:37, Lei Zhang wrote: > Unused? Done. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:53: if (data.size() > kMaxImagePreviewSize) On 2015/06/12 07:26:37, Lei Zhang wrote: > Reading this function by itself, I want to say: You may want to just check the > file size first before reading it. Otherwise, you can end up reading a very > large file, only to throw it away. Same check has been done before (in l.343 in this patchset), so I made this DCHECK instead of "if". > However, it looks like you are trying to only do this for download items that > are less than 10 MB, so, in practice, this should never happen? This checks only for an image and I think 10+ MB image files are rare. I'll add a metrics to verity if this threshold is good. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:375: content::BrowserThread::FILE, On 2015/06/12 07:26:37, Lei Zhang wrote: > Use the blocking thread pool instead of the FILE thread? (Use > base::PostTaskAndReplyWithResult()) Done. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.h (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:106: void OnImageLoaded(std::string image_data); On 2015/06/12 07:26:37, Lei Zhang wrote: > pass by const ref. Done. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:71: static base::FilePath kDownloadItemTargetPath("/tmp/TITLE.bin"); On 2015/06/12 07:26:37, Lei Zhang wrote: > You don't need static in an anonymous namespace. Done.
On 2015/06/12 at 01:47:16, thestig wrote: > On 2015/06/12 01:33:15, yoshiki wrote: > > Greg, Thanks you for your comment. I asked review because you have originally > > introduced this feature. > > > > Lei, PTAL at image_decoder.[h|cc] and chrome_content_utility_client.[h|cc]? > > Thanks. > > Why add additional complexity into ImageDecoder? You'll ultimately read the file anyway, so why not just read it in the browser on an appropriate thread and pass it off to the utility process like everyone else? Hi Lei, The expected result of the decoding process for the download notification is a resized/cropped bitmap of a constrained reasonable size. The source file, on the other hand, could be huge. While the entire file passed through the browser process at some point, none of the APIs up to here required the entire file to be loaded into a contiguous chunk of memory. The code currently tries to avoid disaster by not attempting to decode the image if it is larger than a comfortable limit (10MB?). This limit it arbitrary and isn't sensitive to actual resource availability. It is *probbaly* enough most of the time. But it might be too large if the system is already sputtering, and unnecessarily small in other cases. Moving both reading and resizing to the utility process would give us a disposable address space to pollute with impunity without risk to the browser process. This is why I advised yoshiki@ to go in that direction. Of course it's important that both reading and resizing be in the utility process, otherwise it's a net loss. Does this seem reasonable or am I overly paranoid? (I'm willing the concede the latter, but I wanted to present my reasoning first).
I'll pipe in here with my tiny bit of knowledge, since it seems relevant. My one trip into this code involved adding the option to reduce the image size (dimensions) in the utility process. We were having an issue (with wallpapers, specifically), where a compressed image would fit into the utility process, but the decompressed image would not fit back out, due to the 128MB size limit of IPC messages (see kMaximumMessageSize). Loading the image in the browser process and passing the data to the utility process would prevent this feature from working on files over 128MB, which seem to be very rare, but not unheard of. I don't know the code or use cases well enough to have an opinion on supporting huge image files vs. keeping the interface simple vs. not moving more data than necessary through IPC.
https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:375: content::BrowserThread::FILE, On 2015/06/12 08:00:20, yoshiki wrote: > On 2015/06/12 07:26:37, Lei Zhang wrote: > > Use the blocking thread pool instead of the FILE thread? (Use > > base::PostTaskAndReplyWithResult()) > > Done. Line 44 still says DCHECK_CURRENTLY_ON(content::BrowserThread::FILE); https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:71: static base::FilePath kDownloadItemTargetPath("/tmp/TITLE.bin"); On 2015/06/12 08:00:20, yoshiki wrote: > On 2015/06/12 07:26:37, Lei Zhang wrote: > > You don't need static in an anonymous namespace. > > Done. Please remove the static keyword. https://codereview.chromium.org/1159363002/diff/220001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/220001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:51: DCHECK_NE(data.size(), static_cast<size_t>(kMaxImagePreviewSize)); Did you mean DCHECK_LE() ?
On 2015/06/12 15:21:35, asanka wrote: > Hi Lei, > > The expected result of the decoding process for the download notification is a > resized/cropped bitmap of a constrained reasonable size. The source file, on the > other hand, could be huge. While the entire file passed through the browser > process at some point, none of the APIs up to here required the entire file to > be loaded into a contiguous chunk of memory. The code currently tries to avoid > disaster by not attempting to decode the image if it is larger than a > comfortable limit (10MB?). This limit it arbitrary and isn't sensitive to actual > resource availability. It is *probbaly* enough most of the time. But it might be > too large if the system is already sputtering, and unnecessarily small in other > cases. > > Moving both reading and resizing to the utility process would give us a > disposable address space to pollute with impunity without risk to the browser > process. This is why I advised yoshiki@ to go in that direction. Of course it's > important that both reading and resizing be in the utility process, otherwise > it's a net loss. > > Does this seem reasonable or am I overly paranoid? (I'm willing the concede the > latter, but I wanted to present my reasoning first). Can we take a step back and define what the desired result is? Are we trying to generate thumbnails for all downloads where the file looks like an image? Do we care that it may fail, or fail for files > 10 MB? We are only decoding what Chrome can decode, so we don't care about RAW formats, right? If we are mostly dealing with JPGs, 10 MB is a fairly a fairly comfortable size, as all by 1 sample for a very high end DSLR [1] are under that size. I'd prefer now to add additional complexity unless we really need it. Also, is this a ChromeOS only feature? [1] http://web.canon.jp/imaging/eosd/samples/eos5dmk3/
On 2015/06/13 at 01:50:29, thestig wrote: > On 2015/06/12 15:21:35, asanka wrote: > > Hi Lei, > > > > The expected result of the decoding process for the download notification is a > > resized/cropped bitmap of a constrained reasonable size. The source file, on the > > other hand, could be huge. While the entire file passed through the browser > > process at some point, none of the APIs up to here required the entire file to > > be loaded into a contiguous chunk of memory. The code currently tries to avoid > > disaster by not attempting to decode the image if it is larger than a > > comfortable limit (10MB?). This limit it arbitrary and isn't sensitive to actual > > resource availability. It is *probbaly* enough most of the time. But it might be > > too large if the system is already sputtering, and unnecessarily small in other > > cases. > > > > Moving both reading and resizing to the utility process would give us a > > disposable address space to pollute with impunity without risk to the browser > > process. This is why I advised yoshiki@ to go in that direction. Of course it's > > important that both reading and resizing be in the utility process, otherwise > > it's a net loss. > > > > Does this seem reasonable or am I overly paranoid? (I'm willing the concede the > > latter, but I wanted to present my reasoning first). > > Can we take a step back and define what the desired result is? Are we trying to generate thumbnails for all downloads where the file looks like an image? Do we care that it may fail, or fail for files > 10 MB? We are only decoding what Chrome can decode, so we don't care about RAW formats, right? > Correct. The desired result is to show a preview image for a download if it's an image. Failure isn't terrible, and we are only going to decode what Chrome can decode. > If we are mostly dealing with JPGs, 10 MB is a fairly a fairly comfortable size, as all by 1 sample for a very high end DSLR [1] are under that size. I'd prefer now to add additional complexity unless we really need it. > Yeah, I imagine we'll be tuning this limit as we go as well. I'm less concerned with the failure rate due to hitting the limit than the fact that we are slurping in potentially large files into the browser processes' address space. This may end up not being a big deal (it's capped, after all). So let's go ahead with the current API. We can always change it if it turns out to be an issue. > Also, is this a ChromeOS only feature? > AIUI it's being developed on ChromeOS because that's where the notifications UI is most robust right now. But in theory it should work anywhere rich notifications work. yoshiki: Can you clarify what the plan is for other platforms? > > [1] http://web.canon.jp/imaging/eosd/samples/eos5dmk3/
On 2015/06/13 04:06:23, asanka wrote: > On 2015/06/13 at 01:50:29, thestig wrote: > > On 2015/06/12 15:21:35, asanka wrote: > > > Hi Lei, > > > > > > The expected result of the decoding process for the download notification is > a > > > resized/cropped bitmap of a constrained reasonable size. The source file, on > the > > > other hand, could be huge. While the entire file passed through the browser > > > process at some point, none of the APIs up to here required the entire file > to > > > be loaded into a contiguous chunk of memory. The code currently tries to > avoid > > > disaster by not attempting to decode the image if it is larger than a > > > comfortable limit (10MB?). This limit it arbitrary and isn't sensitive to > actual > > > resource availability. It is *probbaly* enough most of the time. But it > might be > > > too large if the system is already sputtering, and unnecessarily small in > other > > > cases. > > > > > > Moving both reading and resizing to the utility process would give us a > > > disposable address space to pollute with impunity without risk to the > browser > > > process. This is why I advised yoshiki@ to go in that direction. Of course > it's > > > important that both reading and resizing be in the utility process, > otherwise > > > it's a net loss. > > > > > > Does this seem reasonable or am I overly paranoid? (I'm willing the concede > the > > > latter, but I wanted to present my reasoning first). > > > > Can we take a step back and define what the desired result is? Are we trying > to generate thumbnails for all downloads where the file looks like an image? Do > we care that it may fail, or fail for files > 10 MB? We are only decoding what > Chrome can decode, so we don't care about RAW formats, right? > > > > Correct. The desired result is to show a preview image for a download if it's an > image. > > Failure isn't terrible, and we are only going to decode what Chrome can decode. This feature show the preview image of the downloaded file just for users' information. Not showing a preview for either unsupported or very-large image can be acceptable. > > If we are mostly dealing with JPGs, 10 MB is a fairly a fairly comfortable > size, as all by 1 sample for a very high end DSLR [1] are under that size. I'd > prefer now to add additional complexity unless we really need it. > > Yeah, I imagine we'll be tuning this limit as we go as well. > > I'm less concerned with the failure rate due to hitting the limit than the fact > that we are slurping in potentially large files into the browser processes' > address space. This may end up not being a big deal (it's capped, after all). So > let's go ahead with the current API. We can always change it if it turns out to > be an issue. > > > Also, is this a ChromeOS only feature? > > > > AIUI it's being developed on ChromeOS because that's where the notifications UI > is most robust right now. But in theory it should work anywhere rich > notifications work. > > yoshiki: Can you clarify what the plan is for other platforms? We are targeting only to chromeos. We may be going to support other platforms in near feature, but currently we don't have a plan to do that. > > [1] http://web.canon.jp/imaging/eosd/samples/eos5dmk3/
Patchset #10 (id:280001) has been deleted
Patchset #9 (id:260001) has been deleted
Patchset #8 (id:240001) has been deleted
Patchset #8 (id:300001) has been deleted
If this is ChromeOS only, are we doing extra work on other platforms? Or are the new code paths simply dead code? https://codereview.chromium.org/1159363002/diff/320001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/320001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:351: // TODO(yoshiki): Add a metrics to take the satistics of image file sizes. typo: statistics https://codereview.chromium.org/1159363002/diff/320001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/320001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:71: const base::FilePath::StringType Err, sorry, I gave incorrect advice. The preprocessor would turn this into: const std::string kDownloadItemTargetPathString = "/tmp/TITLE.bin"; and that's still not POD. What you need is: const base::FilePath::CharType kDownloadItemTargetPathString[] = FILE_PATH_LITERAL("/tmp/TITLE.bin"); This is documented in base/files/file_path.h, line 58. I also suspect the code, as is, may not build on Windows. I kicked off a win trybot.
Patchset #10 (id:360001) has been deleted
https://codereview.chromium.org/1159363002/diff/340001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/340001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:365: std::string extension_with_dot = base::FilePath::StringType https://codereview.chromium.org/1159363002/diff/340001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/340001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:106: .WillByDefault(ReturnRef(download_item_target_path)); You are returning a reference to a local variable. Use ReturnRefOfCopy() instead.
Patchset #10 (id:380001) has been deleted
https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:375: content::BrowserThread::FILE, On 2015/06/13 00:27:26, Lei Zhang wrote: > On 2015/06/12 08:00:20, yoshiki wrote: > > On 2015/06/12 07:26:37, Lei Zhang wrote: > > > Use the blocking thread pool instead of the FILE thread? (Use > > > base::PostTaskAndReplyWithResult()) > > > > Done. > > Line 44 still says DCHECK_CURRENTLY_ON(content::BrowserThread::FILE); Done. https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:71: static base::FilePath kDownloadItemTargetPath("/tmp/TITLE.bin"); On 2015/06/13 00:27:26, Lei Zhang wrote: > On 2015/06/12 08:00:20, yoshiki wrote: > > On 2015/06/12 07:26:37, Lei Zhang wrote: > > > You don't need static in an anonymous namespace. > > > > Done. > > Please remove the static keyword. Done. https://codereview.chromium.org/1159363002/diff/220001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/220001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:51: DCHECK_NE(data.size(), static_cast<size_t>(kMaxImagePreviewSize)); On 2015/06/13 00:27:26, Lei Zhang wrote: > Did you mean DCHECK_LE() ? Oh, sorry. Done https://codereview.chromium.org/1159363002/diff/340001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/340001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:365: std::string extension_with_dot = On 2015/06/16 15:10:20, asanka wrote: > base::FilePath::StringType Done.
lgtm with asanka's approval. https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:25: using testing::ReturnRef; nit: no longer used
lgtm % nits and browsertest update to use embedded_test_server(). https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_browsertest.cc:505: ASSERT_TRUE(test_server()->Start()); Didn't you migrate this test suite to use embedded_test_server()? Rebase/merge? https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:351: // TODO(yoshiki): Add a metrics to take the statistics of image file sizes. Nit: grammar https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:382: file_path), Nit: git cl format? https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:434: notification_->set_image(gfx::Image()); Is this necessary?
Thank you for long reviewing! https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_browsertest.cc:505: ASSERT_TRUE(test_server()->Start()); On 2015/06/17 14:35:30, asanka wrote: > Didn't you migrate this test suite to use embedded_test_server()? Rebase/merge? Done. https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:351: // TODO(yoshiki): Add a metrics to take the statistics of image file sizes. On 2015/06/17 14:35:30, asanka wrote: > Nit: grammar Done. https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:382: file_path), On 2015/06/17 14:35:30, asanka wrote: > Nit: git cl format? Done. https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.cc:434: notification_->set_image(gfx::Image()); On 2015/06/17 14:35:30, asanka wrote: > Is this necessary? Removed. (and add DCHECK instead) https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1159363002/diff/400001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:25: using testing::ReturnRef; On 2015/06/16 18:35:08, Lei Zhang wrote: > nit: no longer used Done.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1159363002/#ps440001 (title: "rebase & use embedded server")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159363002/440001
Message was sent while issue was closed.
Committed patchset #12 (id:440001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/8719742f50d58e17c81fdf2fd427f3a2fbb847e9 Cr-Commit-Position: refs/heads/master@{#335023} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
