Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
792903002:
Image Search: Move thumbnail JPEG encoding to the renderer. (Closed)
DescriptionImage Search: Move thumbnail JPEG encoding to the renderer.
Not only does this do less work in the browser process, but it also
sidesteps issues with SkBitmap pickling for IPC, resolving a crash
when the user attempts to search for an invalid image.
BUG=431418
Committed: https://crrev.com/ca2f35908550076e04961eba77a8f73a3bde121f
Cr-Commit-Position: refs/heads/master@{#308248}
Patch Set 1 #Patch Set 2 : rough browser tests #Patch Set 3 : finished-ish test #Patch Set 4 : #Patch Set 5 : ipc macros unused #
Total comments: 25
Patch Set 6 : pkasting comments (untested -- needs rebase) #Patch Set 7 : rebase #Patch Set 8 : refactor test #
Total comments: 8
Patch Set 9 : #
Total comments: 2
Patch Set 10 : jnd review #Patch Set 11 : two unused includes #
Total comments: 4
Messages
Total messages: 28 (5 generated)
jbroman@chromium.org changed reviewers: + jnd@chromium.org, kenrb@chromium.org, sky@chromium.org
R=sky@chromium.org (general review, especially chrome/browser/ui/). R=kenrb@chromium.org (for render_messages.h) R=jnd@chromium.org (original author of this feature in bug 89945) It turns out that not all SkBitmaps can be sent properly via the current IPC traits (e.g. images that fail lazy decoding may be non-null, but still not have valid pixels). That should probably be fixed, but in the meantime the quickest way to resolve this bug seems to be to move JPEG encoding down to the renderer. I've also added browser tests for both the normal and corrupt cases, though I admit I'm not confident that my approach to "this test passes if there is no crash" is idiomatic.
I have too many questions about why this code was done that way it is. Is there another reviewer you can get that knows why it is the way it is?
jbroman@chromium.org changed reviewers: + pkasting@chromium.org, thakis@chromium.org
On 2014/12/11 16:56:55, sky wrote: > I have too many questions about why this code was done that way it is. Is there > another reviewer you can get that knows why it is the way it is? CLs for this were authored by jnd@ (already reviewer) and jamesr@ (who has already disowned the bug I'm looking at since he's moved on to other things), so there aren't too many more. Adding thakis@ and pkasting@, who reviewed CLs for bug 89945.
I'd kind of like to know what sky's questions were. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:304: static const char kKeyword[] = "test"; Nit: You could probably just pick one of these two constants and use it in both places below (I'd pick kShortName, since having the keyword match the engine name is common practice) https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:326: void DispatchRightClick(content::WebContents* tab, int x, int y) { I feel like there has to be some kind of existing testing helper to do this for us. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:365: if (owner_) { Nit: {} unnecessary https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:376: }; Nit: DISALLOW_COPY_AND_ASSIGN (2 places) https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:395: DCHECK(quit_reason_ != STILL_RUNNING); Nit: DCHECK_NE https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:464: DispatchRightClick(tab, 15, 15); Nit: Basically all the code above here is shared with the non-corrupt image testcase. I suggest making the shared portions a helper function that both tests call. https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:127: if (thumbnail.colorType() == kN32_SkColorType) { Nit: {} unnecessary https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:135: SkAutoLockPixels lock(bitmap); Is it important that this lock be released before the Send() call below? If not, I'd avoid placing this in a new block scope. If so, I'd add a comment as to why. https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:136: if (bitmap.getPixels() != nullptr && Nit: "!= nullptr" unnecessary https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:137: static_cast<int>(bitmap.rowBytes()) > 0) { Why is this cast to int necessary?
https://codereview.chromium.org/792903002/diff/80001/chrome/browser/ui/tab_co... File chrome/browser/ui/tab_contents/core_tab_helper.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/browser/ui/tab_co... chrome/browser/ui/tab_contents/core_tab_helper.cc:224: search_args.image_thumbnail_content = thumbnail_data; My question is why this is a string and not a SkBitmap.
https://codereview.chromium.org/792903002/diff/80001/chrome/browser/ui/tab_co... File chrome/browser/ui/tab_contents/core_tab_helper.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/browser/ui/tab_co... chrome/browser/ui/tab_contents/core_tab_helper.cc:224: search_args.image_thumbnail_content = thumbnail_data; On 2014/12/11 20:26:49, sky wrote: > My question is why this is a string and not a SkBitmap. We're going to pass it to the search engine as textual data (in the URL), so we have to convert it at some point. The question is simply where to do it.
https://codereview.chromium.org/792903002/diff/80001/chrome/browser/ui/tab_co... File chrome/browser/ui/tab_contents/core_tab_helper.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/browser/ui/tab_co... chrome/browser/ui/tab_contents/core_tab_helper.cc:224: search_args.image_thumbnail_content = thumbnail_data; On 2014/12/11 20:55:33, Peter Kasting wrote: > On 2014/12/11 20:26:49, sky wrote: > > My question is why this is a string and not a SkBitmap. > > We're going to pass it to the search engine as textual data (in the URL), so we > have to convert it at some point. The question is simply where to do it. That makes sense. Thanks for the clarification. This is why I thought someone with more knowledge would have less back and forth.
Done. Apologies for the rebase in between addressing some of the comments and addressing others; I had trouble with gclient. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:304: static const char kKeyword[] = "test"; On 2014/12/11 20:11:29, Peter Kasting wrote: > Nit: You could probably just pick one of these two constants and use it in both > places below (I'd pick kShortName, since having the keyword match the engine > name is common practice) Done. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:326: void DispatchRightClick(content::WebContents* tab, int x, int y) { On 2014/12/11 20:11:29, Peter Kasting wrote: > I feel like there has to be some kind of existing testing helper to do this for > us. Right you are. This file didn't previously use it, so I assumed it didn't exist. Using content::SimulateMouseClickAt instead. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:365: if (owner_) { On 2014/12/11 20:11:29, Peter Kasting wrote: > Nit: {} unnecessary Done. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:376: }; On 2014/12/11 20:11:29, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN (2 places) Done. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:395: DCHECK(quit_reason_ != STILL_RUNNING); On 2014/12/11 20:11:29, Peter Kasting wrote: > Nit: DCHECK_NE Done. https://codereview.chromium.org/792903002/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:464: DispatchRightClick(tab, 15, 15); On 2014/12/11 20:11:29, Peter Kasting wrote: > Nit: Basically all the code above here is shared with the non-corrupt image > testcase. I suggest making the shared portions a helper function that both > tests call. I've extracted the common logic into a helper class (arguably this should be a test fixture class; I'm ambivalent). It doesn't quite work as a simple function because the ContextMenuNotificationObserver needs to last until the click has happened (and the condition to wait for varies), and also I need to get at the RenderProcessHost to watch for renderer crashes in the corrupt case. So that needs two functions with a persisted ContextMenuNotificationObserver. https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:127: if (thumbnail.colorType() == kN32_SkColorType) { On 2014/12/11 20:11:30, Peter Kasting wrote: > Nit: {} unnecessary Done. https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:135: SkAutoLockPixels lock(bitmap); On 2014/12/11 20:11:30, Peter Kasting wrote: > Is it important that this lock be released before the Send() call below? If > not, I'd avoid placing this in a new block scope. If so, I'd add a comment as > to why. It's not important, no. There was just no reason for it to continue to be valid during Send. Changed. https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:136: if (bitmap.getPixels() != nullptr && On 2014/12/11 20:11:30, Peter Kasting wrote: > Nit: "!= nullptr" unnecessary Done. I had some vague idea that Chromium style preferred this, but I can find no supporting evidence. :D https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:137: static_cast<int>(bitmap.rowBytes()) > 0) { On 2014/12/11 20:11:30, Peter Kasting wrote: > Why is this cast to int necessary? The rowBytes argument to gfx::JPEGCodec::Encode is an int, but SkBitmap::rowBytes returns a size_t. Other call sites seem to use a cast in the call (but I don't remember whether this is necessary with our warning flags on various platforms). I suppose I can remove this check (it doesn't seem to be done elsewhere), though I'm not certain whether it's actually impossible to have rowBytes between INT_MAX and SIZE_MAX.
LGTM https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:137: static_cast<int>(bitmap.rowBytes()) > 0) { On 2014/12/12 00:08:57, jbroman wrote: > On 2014/12/11 20:11:30, Peter Kasting wrote: > > Why is this cast to int necessary? > > The rowBytes argument to gfx::JPEGCodec::Encode is an int, but > SkBitmap::rowBytes returns a size_t. I see. You need some kind of cast here, then. The question is whether it should be a static_cast or a checked_cast. A checked_cast will terminate execution (basically CHECK(false)) if the source value doesn't fit in the destination type. We use that when we aren't sure the result will succeed, and it might be a security issue if the cast were to pass the wrong value. I think in this case a static_cast is OK, since I think it would just result in the JPEG encoder not reading all the data, but you should make sure of my reasoning. https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:357: DCHECK_NE(quit_reason_, STILL_RUNNING); Nit: (expected, actual) https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:387: public: Nit: I think all these can be protected https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:403: GURL image_url(test_server_->GetURL(image_path)); Nit: I would probably inline this into the next statement, but up to you.
https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/792903002/diff/80001/chrome/renderer/chrome_r... chrome/renderer/chrome_render_frame_observer.cc:137: static_cast<int>(bitmap.rowBytes()) > 0) { On 2014/12/12 00:35:51, Peter Kasting wrote: > On 2014/12/12 00:08:57, jbroman wrote: > > On 2014/12/11 20:11:30, Peter Kasting wrote: > > > Why is this cast to int necessary? > > > > The rowBytes argument to gfx::JPEGCodec::Encode is an int, but > > SkBitmap::rowBytes returns a size_t. > > I see. You need some kind of cast here, then. The question is whether it > should be a static_cast or a checked_cast. A checked_cast will terminate > execution (basically CHECK(false)) if the source value doesn't fit in the > destination type. We use that when we aren't sure the result will succeed, and > it might be a security issue if the cast were to pass the wrong value. > > I think in this case a static_cast is OK, since I think it would just result in > the JPEG encoder not reading all the data, but you should make sure of my > reasoning. If this were to be negative after a cast, it would result in JPEGCodec reading before the beginning of the data (it does computes a row's address as &input[next_scanline * row_byte_width). But Skia does say where SkBitmap::fRowBytes is set that "require[s] that rowBytes fit in 31bits", so I suppose this assumption is reasonable (and we have it elsewhere). So I'm inclined to leave this as-is unless someone has another preference. https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:357: DCHECK_NE(quit_reason_, STILL_RUNNING); On 2014/12/12 00:35:51, Peter Kasting wrote: > Nit: (expected, actual) Done. https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:387: public: On 2014/12/12 00:35:51, Peter Kasting wrote: > Nit: I think all these can be protected If this actually were a fixture class (which raises the questions "should this have a separate fixture class? should it be in a separate file? what should that file be called?"), then these could be protected. At the moment, it isn't, so the test class needs these members to be public. https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:403: GURL image_url(test_server_->GetURL(image_path)); On 2014/12/12 00:35:51, Peter Kasting wrote: > Nit: I would probably inline this into the next statement, but up to you. still doesn't fit on one line, though, so I mildly prefer to give the temporary a name.
https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:387: public: On 2014/12/12 01:10:46, jbroman wrote: > On 2014/12/12 00:35:51, Peter Kasting wrote: > > Nit: I think all these can be protected > > If this actually were a fixture class (which raises the questions "should this > have a separate fixture class? should it be in a separate file? what should that > file be called?"), then these could be protected. > > At the moment, it isn't, so the test class needs these members to be public. Oh duh, somehow I failed to process your comments about that last time. Honestly I think this should probably be a fixture class. You can leave it declared/defined right here. That basically means we'd move the stuff from SetupAndLoadImagePage() to SetUp(), and we wouldn't need to pass in the args to the constructor because that class could call the relevant accessors directly. It doesn't seem like there would be much difference beyond those things, so AFAICT, using a fixture class would mostly just mean a slight reduction in the amount of code here.
https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:387: public: On 2014/12/12 01:14:45, Peter Kasting wrote: > On 2014/12/12 01:10:46, jbroman wrote: > > On 2014/12/12 00:35:51, Peter Kasting wrote: > > > Nit: I think all these can be protected > > > > If this actually were a fixture class (which raises the questions "should this > > have a separate fixture class? should it be in a separate file? what should > that > > file be called?"), then these could be protected. > > > > At the moment, it isn't, so the test class needs these members to be public. > > Oh duh, somehow I failed to process your comments about that last time. > > Honestly I think this should probably be a fixture class. You can leave it > declared/defined right here. That basically means we'd move the stuff from > SetupAndLoadImagePage() to SetUp(), and we wouldn't need to pass in the args to > the constructor because that class could call the relevant accessors directly. > > It doesn't seem like there would be much difference beyond those things, so > AFAICT, using a fixture class would mostly just mean a slight reduction in the > amount of code here. Done. It still needs the filename argument (and you can't pass arguments to SetUp AFAIK), but done nonetheless.
LGTM. You could simulate the arg passing you want by adding either one or two subclasses which simply pass the relevant data to the constructor. (One class if you define a default behavior in the base class, two if you want to be explicit with each test case.) Up to you.
lgtm lgtm https://codereview.chromium.org/792903002/diff/160001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/160001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:376: scoped_refptr<content::MessageLoopRunner> message_loop_runner_; Please include header file base/memory/ref_counted.h
https://codereview.chromium.org/792903002/diff/160001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/160001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:376: scoped_refptr<content::MessageLoopRunner> message_loop_runner_; On 2014/12/12 02:48:02, Johnny(Jianning) Ding wrote: > Please include header file base/memory/ref_counted.h Done.
sky (or thakis), taking another look chrome/ files (in particular the file in chrome/renderer/)? kenrb, mind taking a look at render_messages.h (IPC change)?
lgtm https://codereview.chromium.org/792903002/diff/200001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:320: this)); Do you want to return true in this branch? https://codereview.chromium.org/792903002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:420: static const char kShortName[] = "test"; (note to future self: I was wondering if we usually says static on const char[]s. "[chromium-dev] Non-static const char arrays?" answers this. With clang it doesn't make a difference, but gcc produces better code with static. For details, see that thread)
https://codereview.chromium.org/792903002/diff/200001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:320: this)); On 2014/12/12 18:43:32, Nico wrote: > Do you want to return true in this branch? IIUC, this will block it from being received by CoreTabHelper. I don't have any particular reason to want to treat it as "handled", and in fact if CoreTabHelper /were/ to crash upon receiving the message, that would be a good crash to detect (though it's not what this test is looking for). I'll add a "return true" if you think it's helpful, but I don't think it's necessary.
https://codereview.chromium.org/792903002/diff/200001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/792903002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:320: this)); On 2014/12/12 18:47:03, jbroman wrote: > On 2014/12/12 18:43:32, Nico wrote: > > Do you want to return true in this branch? > > IIUC, this will block it from being received by CoreTabHelper. I don't have any > particular reason to want to treat it as "handled", and in fact if CoreTabHelper > /were/ to crash upon receiving the message, that would be a good crash to detect > (though it's not what this test is looking for). > > I'll add a "return true" if you think it's helpful, but I don't think it's > necessary. Ok, sounds good. Maybe add an "// Not returning true because…" comment, since this looks like it could be an omission to a clueless reader like me. (or keep as is if you think a comment is overkill)
ipc lgtm, and thanks for taking over this bug!
The CQ bit was checked by jbroman@chromium.org
The CQ bit was unchecked by jbroman@chromium.org
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/792903002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ca2f35908550076e04961eba77a8f73a3bde121f Cr-Commit-Position: refs/heads/master@{#308248} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
