Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
917333004:
Answers in Suggest icon downloading (Closed)
DescriptionAnswers in Suggest icon downloading
Some answers in suggest results have an associated icon, for example the weather may show a sunny or cloudy-skies image.
This change will download the associated icon and display it in the answer suggested.
BUG=456339
Committed: https://crrev.com/e92708a2c20888698a7308f370dcc04c14c21e9f
Cr-Commit-Position: refs/heads/master@{#319524}
Patch Set 1 #Patch Set 2 : removed some {} #
Total comments: 48
Patch Set 3 : Reworked from cl review; ran clang-format #Patch Set 4 : re-added const on PaintMatch #
Total comments: 18
Patch Set 5 : Removed answer image url member #
Total comments: 1
Patch Set 6 : checking for null service to avoid segfault #Patch Set 7 : fetching image service in constructor #Patch Set 8 : using weak ptr on view #Patch Set 9 : merge from master #Patch Set 10 : added retry logic to fetch image service #
Total comments: 10
Patch Set 11 : setting image service in constructor #
Total comments: 12
Patch Set 12 : changes for nits in review #
Total comments: 21
Patch Set 13 : update for review nits #Patch Set 14 : cancel image fetch even if the next suggestion has no answer record #
Total comments: 8
Patch Set 15 : nit changes #
Messages
Total messages: 58 (13 generated)
dschuyler@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, Please do an OWNERS review on this so I can land this change. Thanks!
groby@chromium.org changed reviewers: + groby@chromium.org
Drive-by comments :) https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:57: *image_ = gfx::ImageSkia::CreateFrom1xBitmap(answers_image); Please don't communicate via shared memory. Pass the image into a callback on the view. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; What exactly is the ownership situation? Is view_ guaranteed to outlive the request? (I'd assume it currently isn't - you probably want to cancel in-flight requests when the view is deleted https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:482: if (image_url.is_valid() && image_url == image_url_) We don't care about this condition. ImageFetcher IIRC will handle this for us. (And even cache the last <n> images) That follows the exact same path as if a download was completed, just instantly. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:490: Profile* profile = location_bar_view_->profile(); Can |profile| _ever_ be NULL here? https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:493: content::BrowserContext* browser_context = profile; No need for the temporary, just pass the profile directly into GetForBrowserContext. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:498: if (!image_service) Just DCHECK it's not NULL. It never should be, so we don't have to handle it. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:501: if (request_id_ != BitmapFetcherService::REQUEST_ID_INVALID) { Just call CancelRequest, without all the testing. It gracefully handles cancelling non-existent requests https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:511: if (observer) { Skip the test - new can't fail. (If it does, crashing is just fine) https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:517: return answer_image_; No need to return an image if you always request - it'll always travel through OnImageChanged. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.h:161: GURL image_url_; I don't think you need |image_url_|.
https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:50: {} Please read the Google style guide on how to format constructors Nit: I prefer to avoid inlining method definitions into class declarations, even for classes in .cc files, as it's a request to the compiler to inline things (which we don't really care about doing) and IMO it's a bit harder to read. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:61: // than it is truly taking. It seems like if you didn't do this we could potentially _never_ draw the icon on the view (assuming nothing else was happening). I think the comment is thus a bit misleading -- I'd just remove it entirely. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; On 2015/02/13 02:30:56, groby wrote: > What exactly is the ownership situation? Is view_ guaranteed to outlive the > request? (I'd assume it currently isn't - you probably want to cancel in-flight > requests when the view is deleted In that sense, why not just make the view a BitmapFetcherService::Observer directly? That avoids all the ownership and callback issues, I think. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:68: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:290: if (image.width()) This should probably be !image.size().IsEmpty() https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:304: x += default_icon_size_ + LocationBarView::kIconInternalPadding; How come we draw the |image| size, but then we step |x| by the |default_icon_size_|? What if the two don't match? https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:479: gfx::ImageSkia OmniboxResultView::GetAnswerIcon() { There's no real need to return |answer_image_| in every return statement. If we're always returning the same thing, we can return void and the caller can use |answer_image_| directly. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:490: Profile* profile = location_bar_view_->profile(); On 2015/02/13 02:30:56, groby wrote: > Can |profile| _ever_ be NULL here? I don't think so. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:511: if (observer) { On 2015/02/13 02:30:56, groby wrote: > Skip the test - new can't fail. (If it does, crashing is just fine) To be more precise: We have a memory allocator that force-crashes the browser on OOM, so you'll never see a failed new() call unless you do a bunch of work to special-case your allocation to allow failure. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:517: return answer_image_; On 2015/02/13 02:30:56, groby wrote: > No need to return an image if you always request - it'll always travel through > OnImageChanged. If OnImageChanged() is called synchronously in the case where we have the image available, that's fine. If it's called asynchronously, returning the image here can make it paint sooner. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.h:160: BitmapFetcherService::RequestId request_id_; I would consider making the members here mutable and making the other functions you've added/touched be const. From the perspective of a consumer of the class, this is invisible implementation detail and the apparent class state isn't changed, so const functions are more appropriate.
https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; On 2015/02/13 03:01:07, Peter Kasting wrote: > On 2015/02/13 02:30:56, groby wrote: > > What exactly is the ownership situation? Is view_ guaranteed to outlive the > > request? (I'd assume it currently isn't - you probably want to cancel > in-flight > > requests when the view is deleted > > In that sense, why not just make the view a BitmapFetcherService::Observer > directly? That avoids all the ownership and callback issues, I think. Because the ImageFetcher takes ownership of the observer :( https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:517: return answer_image_; On 2015/02/13 03:01:07, Peter Kasting wrote: > On 2015/02/13 02:30:56, groby wrote: > > No need to return an image if you always request - it'll always travel through > > OnImageChanged. > > If OnImageChanged() is called synchronously in the case where we have the image > available, that's fine. If it's called asynchronously, returning the image here > can make it paint sooner. It is called synchronously when data is retrieved from the cache, yes.
https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; On 2015/02/13 06:03:47, groby wrote: > On 2015/02/13 03:01:07, Peter Kasting wrote: > > On 2015/02/13 02:30:56, groby wrote: > > > What exactly is the ownership situation? Is view_ guaranteed to outlive the > > > request? (I'd assume it currently isn't - you probably want to cancel > > in-flight > > > requests when the view is deleted > > > > In that sense, why not just make the view a BitmapFetcherService::Observer > > directly? That avoids all the ownership and callback issues, I think. > > Because the ImageFetcher takes ownership of the observer :( Can we Just Fix That? That design is atypical of observers in the rest of our codebase, so it's liable to bite someone. The ImageFetcher should just have an ObserverList that consumers get registered on.
Aside from the question about how observers work/should work, the other issues should be addressed. Please let me know if I missed anything. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:50: {} On 2015/02/13 03:01:07, Peter Kasting wrote: > Please read the Google style guide on how to format constructors > > Nit: I prefer to avoid inlining method definitions into class declarations, even > for classes in .cc files, as it's a request to the compiler to inline things > (which we don't really care about doing) and IMO it's a bit harder to read. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:57: *image_ = gfx::ImageSkia::CreateFrom1xBitmap(answers_image); On 2015/02/13 02:30:56, groby wrote: > Please don't communicate via shared memory. Pass the image into a callback on > the view. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:61: // than it is truly taking. On 2015/02/13 03:01:07, Peter Kasting wrote: > It seems like if you didn't do this we could potentially _never_ draw the icon > on the view (assuming nothing else was happening). I think the comment is thus > a bit misleading -- I'd just remove it entirely. Empirically, something else eventually causes a paint and the icon shows up. The comment was accurate to the user experience of the issue, but technically you're correct of course. If nothing else triggered a paint, the icon would never show. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; On 2015/02/13 06:03:47, groby wrote: > On 2015/02/13 03:01:07, Peter Kasting wrote: > > On 2015/02/13 02:30:56, groby wrote: > > > What exactly is the ownership situation? Is view_ guaranteed to outlive the > > > request? (I'd assume it currently isn't - you probably want to cancel > > in-flight > > > requests when the view is deleted > > > > In that sense, why not just make the view a BitmapFetcherService::Observer > > directly? That avoids all the ownership and callback issues, I think. > > Because the ImageFetcher takes ownership of the observer :( Thanks for pointing out this over-site. I intended to cancel it and forgot. There is now a cancel in the view dtor. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; On 2015/02/13 03:01:07, Peter Kasting wrote: > On 2015/02/13 02:30:56, groby wrote: > > What exactly is the ownership situation? Is view_ guaranteed to outlive the > > request? (I'd assume it currently isn't - you probably want to cancel > in-flight > > requests when the view is deleted > > In that sense, why not just make the view a BitmapFetcherService::Observer > directly? That avoids all the ownership and callback issues, I think. Acknowledged. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; On 2015/02/13 02:30:56, groby wrote: > What exactly is the ownership situation? Is view_ guaranteed to outlive the > request? (I'd assume it currently isn't - you probably want to cancel in-flight > requests when the view is deleted Acknowledged. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:68: }; On 2015/02/13 03:01:07, Peter Kasting wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:290: if (image.width()) On 2015/02/13 03:01:07, Peter Kasting wrote: > This should probably be !image.size().IsEmpty() Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:304: x += default_icon_size_ + LocationBarView::kIconInternalPadding; On 2015/02/13 03:01:07, Peter Kasting wrote: > How come we draw the |image| size, but then we step |x| by the > |default_icon_size_|? What if the two don't match? Maybe I'm confused, but I think that the destination width/height are both default_icon_size_. Err, to be more clear, this is a scaling image draw. The size of the source image shouldn't matter if I understand correctly. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:479: gfx::ImageSkia OmniboxResultView::GetAnswerIcon() { On 2015/02/13 03:01:07, Peter Kasting wrote: > There's no real need to return |answer_image_| in every return statement. If > we're always returning the same thing, we can return void and the caller can use > |answer_image_| directly. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:482: if (image_url.is_valid() && image_url == image_url_) On 2015/02/13 02:30:56, groby wrote: > We don't care about this condition. ImageFetcher IIRC will handle this for us. > (And even cache the last <n> images) > > That follows the exact same path as if a download was completed, just instantly. I removed the "image_url.is_valid() && " part. I feel like I should still be checking the url to avoid spamming the allocation/free of AnswersImageObserverDesktop objects. (Am I wrong on that?) Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:490: Profile* profile = location_bar_view_->profile(); On 2015/02/13 02:30:56, groby wrote: > Can |profile| _ever_ be NULL here? Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:490: Profile* profile = location_bar_view_->profile(); On 2015/02/13 02:30:56, groby wrote: > Can |profile| _ever_ be NULL here? Acknowledged. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:493: content::BrowserContext* browser_context = profile; On 2015/02/13 02:30:56, groby wrote: > No need for the temporary, just pass the profile directly into > GetForBrowserContext. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:498: if (!image_service) On 2015/02/13 02:30:56, groby wrote: > Just DCHECK it's not NULL. It never should be, so we don't have to handle it. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:501: if (request_id_ != BitmapFetcherService::REQUEST_ID_INVALID) { On 2015/02/13 02:30:56, groby wrote: > Just call CancelRequest, without all the testing. It gracefully handles > cancelling non-existent requests Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:511: if (observer) { On 2015/02/13 02:30:56, groby wrote: > Skip the test - new can't fail. (If it does, crashing is just fine) Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:511: if (observer) { On 2015/02/13 02:30:56, groby wrote: > Skip the test - new can't fail. (If it does, crashing is just fine) Acknowledged. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:517: return answer_image_; On 2015/02/13 02:30:56, groby wrote: > No need to return an image if you always request - it'll always travel through > OnImageChanged. Done. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:517: return answer_image_; On 2015/02/13 02:30:56, groby wrote: > No need to return an image if you always request - it'll always travel through > OnImageChanged. Acknowledged. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:249: void OmniboxResultView::PaintMatch(const AutocompleteMatch& match, This reformatting was done by clang-format. Should I change it back or let clang-format have its way?
https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:304: x += default_icon_size_ + LocationBarView::kIconInternalPadding; On 2015/02/13 22:25:25, dschuyler wrote: > On 2015/02/13 03:01:07, Peter Kasting wrote: > > How come we draw the |image| size, but then we step |x| by the > > |default_icon_size_|? What if the two don't match? > > Maybe I'm confused, but I think that the destination width/height are both > default_icon_size_. Err, to be more clear, this is a scaling image draw. The > size of the source image shouldn't matter if I understand correctly. I think you're right. I read the code explicitly looking to see if that was the case and still missed it. Go me. https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:482: if (image_url.is_valid() && image_url == image_url_) On 2015/02/13 22:25:25, dschuyler wrote: > On 2015/02/13 02:30:56, groby wrote: > > We don't care about this condition. ImageFetcher IIRC will handle this for us. > > (And even cache the last <n> images) > > > > That follows the exact same path as if a download was completed, just > instantly. > > I removed the "image_url.is_valid() && " part. I feel like I should still be > checking the url to avoid spamming the allocation/free of > AnswersImageObserverDesktop objects. (Am I wrong on that?) Depends on how frequent this is. I probably would take the simpler code and lower memory cost (of not having a GURL member) over the allocation/free pair, but of course we could get both if we changed how the image fetcher handled observers as I'm hoping to do :) https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:249: void OmniboxResultView::PaintMatch(const AutocompleteMatch& match, On 2015/02/13 22:25:26, dschuyler wrote: > This reformatting was done by clang-format. Should I change it back or let > clang-format have its way? I would just break "int x" to a new line. I know clang-format packs args at callsites, but I'm surprised to see it pack args at a definition, especially in a case like this where almost everything is already one-per-line. You might consider filing a clang-format bug.
https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:67: views::View* view_; On 2015/02/13 21:59:32, Peter Kasting wrote: > On 2015/02/13 06:03:47, groby wrote: > > On 2015/02/13 03:01:07, Peter Kasting wrote: > > > On 2015/02/13 02:30:56, groby wrote: > > > > What exactly is the ownership situation? Is view_ guaranteed to outlive > the > > > > request? (I'd assume it currently isn't - you probably want to cancel > > > in-flight > > > > requests when the view is deleted > > > > > > In that sense, why not just make the view a BitmapFetcherService::Observer > > > directly? That avoids all the ownership and callback issues, I think. > > > > Because the ImageFetcher takes ownership of the observer :( > > Can we Just Fix That? That design is atypical of observers in the rest of our > codebase, so it's liable to bite someone. The ImageFetcher should just have an > ObserverList that consumers get registered on. Filed http://crbug.com/458681 to fix this. (base::Bind will be a better fit than ObserverList, I think - usually, callers want to be notified about one specific request only) https://codereview.chromium.org/917333004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:482: if (image_url.is_valid() && image_url == image_url_) On 2015/02/13 22:25:25, dschuyler wrote: > On 2015/02/13 02:30:56, groby wrote: > > We don't care about this condition. ImageFetcher IIRC will handle this for us. > > (And even cache the last <n> images) > > > > That follows the exact same path as if a download was completed, just > instantly. > > I removed the "image_url.is_valid() && " part. I feel like I should still be > checking the url to avoid spamming the allocation/free of > AnswersImageObserverDesktop objects. (Am I wrong on that?) > > Done. You shouldn't call GetAnswerIcon unless there's a new set of results that has an answer - that doesn't really happen that often. So I wouldn't worry about allocation/deallocation. (Long term, we should follow Peter's suggestion and have the image fetcher not take ownership, so we'll avoid any alloc/dealloc). In general, unless things happen in a hot loop, code clarity should win over small optimizations. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:249: void OmniboxResultView::PaintMatch(const AutocompleteMatch& match, On 2015/02/13 22:25:26, dschuyler wrote: > This reformatting was done by clang-format. Should I change it back or let > clang-format have its way? While I'm technically fine with either formatting, the entire rest of the file follows the "indent params 4 spaces" model, so yes, please undo clang-format. (git cl format is awesome, but usually, you want "git add -p" afterwards so you can skip the parts where it's clearly doing the wrong thing.)
https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:249: void OmniboxResultView::PaintMatch(const AutocompleteMatch& match, On 2015/02/13 22:47:22, groby wrote: > On 2015/02/13 22:25:26, dschuyler wrote: > > This reformatting was done by clang-format. Should I change it back or let > > clang-format have its way? > > While I'm technically fine with either formatting, the entire rest of the file > follows the "indent params 4 spaces" model, so yes, please undo clang-format. All omnibox code, at least, should only be indenting 4 (instead of even) if indenting even doesn't fit. I assume that's indeed the case. If not please fix -- but don't undo the change from indent-4 to indent-even here as that's a good change.
LGTM https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:281: // Source x, y, w, h. Nit: I'd normally omit in-the-middle-of-a-bunch-of-args comments like this, but it is true that DrawImageInt() has a lot of args, so up to you. (We really need to convert DrawImageInt() over to taking Rects, or at least Points and Sizes, instead of four ints.) https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:471: // We have already requested this url. Nit: A comment here would necessitate {} on the conditional (since the rule is based around "more than one physical line" rather than "more than one logical line"). Since the code seems pretty obvious without this, I'd just remove the comment. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:477: // Make a new image download request. Nit: This block doesn't actually make a new download request (you do that below), so I'd omit this comment. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:483: // Cancel the prior request. Nit: Omit this comment as it merely restates the code. (Overall I'd probably simply combine everything after the early return into a single block with a single comment above. Other than brief summaries of longer blocks, comments should mostly motivate "why" rather than explain "what" unless the "what" in the code is very unclear or there are hidden gotchas readers need to understand.) https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:607: if (match_.answer->second_line().image_url().is_valid()) { Nit: No {} (omnibox file style) https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.h:79: void SetAnswerImage(gfx::ImageSkia image); Nit: I wonder if this should be private and the observer a friend. The observer is sort of a technical implementation detail and we don't ant any other classes calling this API. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.h:127: void GetAnswerIcon(); Nit: FetchAnswerImage(), perhaps, since this no longer returns an image.
New patchsets have been uploaded after l-g-t-m from pkasting@chromium.org
The image fetch is called a couple times per keystroke when typing in the omnibox while the answer has an icon. It's not called otherwise (that I've found). Going without the answer image url member didn't impact my user experience, so I went ahead and removed it. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:249: void OmniboxResultView::PaintMatch(const AutocompleteMatch& match, On 2015/02/13 22:36:36, Peter Kasting wrote: > On 2015/02/13 22:25:26, dschuyler wrote: > > This reformatting was done by clang-format. Should I change it back or let > > clang-format have its way? > > I would just break "int x" to a new line. I know clang-format packs args at > callsites, but I'm surprised to see it pack args at a definition, especially in > a case like this where almost everything is already one-per-line. > > You might consider filing a clang-format bug. Acknowledged. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:249: void OmniboxResultView::PaintMatch(const AutocompleteMatch& match, On 2015/02/13 22:47:22, groby wrote: > On 2015/02/13 22:25:26, dschuyler wrote: > > This reformatting was done by clang-format. Should I change it back or let > > clang-format have its way? > > While I'm technically fine with either formatting, the entire rest of the file > follows the "indent params 4 spaces" model, so yes, please undo clang-format. > (git cl format is awesome, but usually, you want "git add -p" afterwards so you > can skip the parts where it's clearly doing the wrong thing.) Acknowledged. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:249: void OmniboxResultView::PaintMatch(const AutocompleteMatch& match, On 2015/02/13 22:49:39, Peter Kasting wrote: > On 2015/02/13 22:47:22, groby wrote: > > On 2015/02/13 22:25:26, dschuyler wrote: > > > This reformatting was done by clang-format. Should I change it back or let > > > clang-format have its way? > > > > While I'm technically fine with either formatting, the entire rest of the file > > follows the "indent params 4 spaces" model, so yes, please undo clang-format. > > All omnibox code, at least, should only be indenting 4 (instead of even) if > indenting even doesn't fit. I assume that's indeed the case. If not please fix > -- but don't undo the change from indent-4 to indent-even here as that's a good > change. If I'm following this correctly, the last thing said was to leave it. Acknowledged. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:471: // We have already requested this url. On 2015/02/14 00:10:14, Peter Kasting wrote: > Nit: A comment here would necessitate {} on the conditional (since the rule is > based around "more than one physical line" rather than "more than one logical > line"). Since the code seems pretty obvious without this, I'd just remove the > comment. I removed the image_url_ entirely. Done. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:477: // Make a new image download request. On 2015/02/14 00:10:14, Peter Kasting wrote: > Nit: This block doesn't actually make a new download request (you do that > below), so I'd omit this comment. Done. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:483: // Cancel the prior request. On 2015/02/14 00:10:15, Peter Kasting wrote: > Nit: Omit this comment as it merely restates the code. > > (Overall I'd probably simply combine everything after the early return into a > single block with a single comment above. Other than brief summaries of longer > blocks, comments should mostly motivate "why" rather than explain "what" unless > the "what" in the code is very unclear or there are hidden gotchas readers need > to understand.) Done. https://codereview.chromium.org/917333004/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:607: if (match_.answer->second_line().image_url().is_valid()) { On 2015/02/14 00:10:14, Peter Kasting wrote: > Nit: No {} (omnibox file style) Done.
lgtm https://codereview.chromium.org/917333004/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:473: // them. Personal nit: The comment is probably not helpful. (Feel free to leave in, though. I don't feel strongly)
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/917333004/80001
The CQ bit was unchecked by dschuyler@chromium.org
Hmm, it looks like this segfaulted in the linux_chromium_asan_rel test:
OmniboxResultView::~OmniboxResultView() {
BitmapFetcherServiceFactory::GetForBrowserContext(
location_bar_view_->profile())->CancelRequest(request_id_);
}
My first impression is GetForBrowserContext() may be returning nullptr.
I'll look into it further -- suggestions welcome :)
More info:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
New patchsets have been uploaded after l-g-t-m from groby@chromium.org
It looks like (through reading the code) that a null location_bar_view_ or profile() result would cause a crash sooner. I think having a null service pointer would still allow it to sail through the remaining calls on the stack from omnibox_result_view.cc:179 until the crash when it finally tries to deference it at vector:1476.
Yep. Looking at the callstack, that's because the LocationBarView owns the OmniboxResultView - you don't get to use it in the destructor. You might want to hold on to the BitmapFetcherService, so you don't have to look it up in the destructor. If LocationBarView::~LocationBarView can be trusted, it looks like services have not shut down at that point yet. (If you dig into chrome/browser/lifetime/* and follow the threads, you'll see that's indeed true. Browser windows are closed before we shut down profiles, and services associated with profiles)
Ping?
I'm testing with a BitmapFetcherDelegate and/or using a weak pointer for the view. I'm also looking at using BitmapFetcherService::Observer with a weak pointer for the view.
On 2015/02/28 00:59:48, dschuyler wrote: > I'm testing with a BitmapFetcherDelegate and/or using a weak pointer for the > view. I'm also looking at using BitmapFetcherService::Observer with a weak > pointer for the view. BTW, you may want to keep aware of https://codereview.chromium.org/931993002/ in case it impacts your design here.
I've switched to WeakPtr for the view in the image fetcher observer. This seems like a reasonable direction to go with until the image fetcher ownership is changed. Now that we're all green on the try bots, please re-give the LGTM (if it does, of course).
https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:63: view_.get()->SetAnswerImage(gfx::ImageSkia::CreateFrom1xBitmap(image)); Nit: No .get() https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:185: if (HasImageService()) This will create the image service if it doesn't exist, which you don't want. Seems like you should just test |iamge_service_| directly. https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:481: if (HasImageService()) { This now becomes the lone caller of this function. I would probably collapse not only the HasImageService() code but also the conditionals at the callsite of this function all into this and call it something like StartFetchingAnswerImageIfNecessary(). You might also consider initializing |image_service_| in the constructor if doing so is cheap, since in that case lazy-init mostly just makes the code more complex. https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:644: location_bar_view_->profile()); Can this call ever actually return null? Maybe when testing? If not, we should write the code unconditionally.
Asking more questions. https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:185: if (HasImageService()) On 2015/03/03 00:04:01, Peter Kasting wrote: > This will create the image service if it doesn't exist, which you don't want. > Seems like you should just test |iamge_service_| directly. Yep. I'll change that (but I want to ask questions about the comments below first). https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:481: if (HasImageService()) { On 2015/03/03 00:04:01, Peter Kasting wrote: > This now becomes the lone caller of this function. > > I would probably collapse not only the HasImageService() code but also the > conditionals at the callsite of this function all into this and call it > something like StartFetchingAnswerImageIfNecessary(). > > You might also consider initializing |image_service_| in the constructor if > doing so is cheap, since in that case lazy-init mostly just makes the code more > complex. It was initializing in the constructor and that seemed to cause some unit tests to fail with @@@STEP_LOG_LINE@AppListControllerBrowserTest.CreateNewWindow@[3248:420:0302/125006:FATAL:omnibox_result_view.cc(183)] Check failed: image_service_.@@@ So I was looking for a way to deal with that. Though, it's also possible that I may be misunderstanding the error in the unit test or maybe there's a better way to deal with it. Here's a link to the stdio from the failing tests: http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... I think I found 'a solution'. What would be a 'better solution'? https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:644: location_bar_view_->profile()); On 2015/03/03 00:04:01, Peter Kasting wrote: > Can this call ever actually return null? Maybe when testing? If not, we should > write the code unconditionally. GetForBrowserContext can return null in unit tests.
https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:481: if (HasImageService()) { On 2015/03/03 00:16:04, dschuyler wrote: > On 2015/03/03 00:04:01, Peter Kasting wrote: > > This now becomes the lone caller of this function. > > > > I would probably collapse not only the HasImageService() code but also the > > conditionals at the callsite of this function all into this and call it > > something like StartFetchingAnswerImageIfNecessary(). > > > > You might also consider initializing |image_service_| in the constructor if > > doing so is cheap, since in that case lazy-init mostly just makes the code > more > > complex. > > It was initializing in the constructor and that seemed to cause some unit tests > to fail with > @@@STEP_LOG_LINE@AppListControllerBrowserTest.CreateNewWindow@[3248:420:0302/125006:FATAL:omnibox_result_view.cc(183)] > Check failed: image_service_.@@@ > > So I was looking for a way to deal with that. Though, it's also possible that I > may be misunderstanding the error in the unit test or maybe there's a better way > to deal with it. > > Here's a link to the stdio from the failing tests: > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... > > I think I found 'a solution'. What would be a 'better solution'? That doesn't sound like the problem was in initializing this in the constructor, it sounds like the problem was that later code CHECKed that the initialization always succeeded, whereas, based on your comments below, this can intentionally return a null service in tests. I would assume, then, that the solution would be to init in the constructor, but null-check at the time of use.
On 2015/03/03 00:18:28, Peter Kasting wrote: > https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): > > https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:481: if > (HasImageService()) { > On 2015/03/03 00:16:04, dschuyler wrote: > > On 2015/03/03 00:04:01, Peter Kasting wrote: > > > This now becomes the lone caller of this function. > > > > > > I would probably collapse not only the HasImageService() code but also the > > > conditionals at the callsite of this function all into this and call it > > > something like StartFetchingAnswerImageIfNecessary(). > > > > > > You might also consider initializing |image_service_| in the constructor if > > > doing so is cheap, since in that case lazy-init mostly just makes the code > > more > > > complex. > > > > It was initializing in the constructor and that seemed to cause some unit > tests > > to fail with > > > @@@STEP_LOG_LINE@AppListControllerBrowserTest.CreateNewWindow@[3248:420:0302/125006:FATAL:omnibox_result_view.cc(183)] > > Check failed: image_service_.@@@ > > > > So I was looking for a way to deal with that. Though, it's also possible that > I > > may be misunderstanding the error in the unit test or maybe there's a better > way > > to deal with it. > > > > Here's a link to the stdio from the failing tests: > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... > > > > I think I found 'a solution'. What would be a 'better solution'? > > That doesn't sound like the problem was in initializing this in the constructor, > it sounds like the problem was that later code CHECKed that the initialization > always succeeded, whereas, based on your comments below, this can intentionally > return a null service in tests. > > I would assume, then, that the solution would be to init in the constructor, but > null-check at the time of use. In the version of that file that shown the error, line 183 is in the constructor. I hope I'm not missing something that is right in front of me (but I am human, so that happens). It really looks like it's failing in the constructor to me. Maybe you're suggesting that I check it in the constructor, and if it fails then put the checks in the other places to ignore the image fetching(?). Something I don't know is whether getting the image fetcher may eventually succeed after the constructor - or whether it's a thing that will always either work or not work (rather than coming online later in execution).
On 2015/03/03 00:44:37, dschuyler wrote: > On 2015/03/03 00:18:28, Peter Kasting wrote: > > > https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... > > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): > > > > > https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... > > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:481: if > > (HasImageService()) { > > On 2015/03/03 00:16:04, dschuyler wrote: > > > On 2015/03/03 00:04:01, Peter Kasting wrote: > > > > This now becomes the lone caller of this function. > > > > > > > > I would probably collapse not only the HasImageService() code but also the > > > > conditionals at the callsite of this function all into this and call it > > > > something like StartFetchingAnswerImageIfNecessary(). > > > > > > > > You might also consider initializing |image_service_| in the constructor > if > > > > doing so is cheap, since in that case lazy-init mostly just makes the code > > > more > > > > complex. > > > > > > It was initializing in the constructor and that seemed to cause some unit > > tests > > > to fail with > > > > > > @@@STEP_LOG_LINE@AppListControllerBrowserTest.CreateNewWindow@[3248:420:0302/125006:FATAL:omnibox_result_view.cc(183)] > > > Check failed: image_service_.@@@ > > > > > > So I was looking for a way to deal with that. Though, it's also possible > that > > I > > > may be misunderstanding the error in the unit test or maybe there's a better > > way > > > to deal with it. > > > > > > Here's a link to the stdio from the failing tests: > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... > > > > > > I think I found 'a solution'. What would be a 'better solution'? > > > > That doesn't sound like the problem was in initializing this in the > constructor, > > it sounds like the problem was that later code CHECKed that the initialization > > always succeeded, whereas, based on your comments below, this can > intentionally > > return a null service in tests. > > > > I would assume, then, that the solution would be to init in the constructor, > but > > null-check at the time of use. > > In the version of that file that shown the error, line 183 is in the > constructor. > I hope I'm not missing something that is right in front of me (but I am human, > so that happens). It really looks like it's failing in the constructor to me. I think you're misunderstanding my terminology. A "null-check" as I'm using the phrase means this: if (image_fetcher_) ... Whereas a "checkfailure" would be due to code like: [D]CHECK(image_fetcher_); Your comments claim that there may be no bitmap image fetcher service in testing mode. If so, [D]CHECKing that we can get one is wrong, and it's predictable that your tests would have failed when you wrote your patch to do so. The right answer in that case, as I was trying to say above, is to init the member in the constructor, not [D]CHECK that it's non-null, and conditional-check (again, not [D]CHECK) whether it's null where we want to use it. > Maybe you're suggesting that I check it in the constructor, and if it fails then > put the checks in the other places to ignore the image fetching(?). Something I > don't know is whether getting the image fetcher may eventually succeed after the > constructor - or whether it's a thing that will always either work or not work > (rather than coming online later in execution). Either getting the service will succeed in the constructor or it will never succeed.
Patchset #11 (id:200001) has been deleted
Keen, If I follow correctly this patch will have the changes desired. Thanks Peter.
Functionally this looks right, all the comments below are style. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:182: location_bar_view_->profile()); Nit: Init this in the initializer list https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:211: GetAnswerIcon(); It seems sort of weird to have caller code that conditionally calls a function, and have the function immediately do a further conditional check. Plus, both sides want to look at match_.answer->second_line().image_url(), so by extracting the code in GetAnswerIcon() out, we're not really improving any kind of encapsulation. I would respond to all this by collapsing together all this code; either call the function unconditionally here and move the conditionals there, or (probably better) just inline the contents of that function into here, since this is the lone callsite and the function body is short. Then the two separate conditionals can be one statement. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:480: // The answer images are not bundled with Chrome, so we need to download Is there a reason the reader would have expected these images to be bundled with Chrome? If not, I would probably omit this comment. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:79: void SetAnswerImage(gfx::ImageSkia image); Nit: Write a comment about what this does https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:81: base::WeakPtr<OmniboxResultView> GetWeakPtr() { You shouldn't inline functions that aren't simple cheap accessors, and you shouldn't name inlined functions in CamelCase. In this case this doesn't even need to be exposed as a function -- the lone caller could just call weak_ptr_factory_.GetWeakPtr() directly. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:164: // If the answer has an icon, these control the fetching and updating Nit: Since the |image_service_| is initted unconditionally, it probably belongs somewhere around where |location_bar_view_| is listed (since that's a similar "raw pointer to a useful external object"). The other two I might put near the bottom of the member list (maybe above the mutables?) since unlike most of the other members they're only going to be occasionally relevant.
I think I've got the changes in and the try bots are green again. Is this version good to go? https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:63: view_.get()->SetAnswerImage(gfx::ImageSkia::CreateFrom1xBitmap(image)); On 2015/03/03 00:04:01, Peter Kasting wrote: > Nit: No .get() Done. https://codereview.chromium.org/917333004/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:481: if (HasImageService()) { On 2015/03/03 00:18:28, Peter Kasting wrote: > On 2015/03/03 00:16:04, dschuyler wrote: > > On 2015/03/03 00:04:01, Peter Kasting wrote: > > > This now becomes the lone caller of this function. > > > > > > I would probably collapse not only the HasImageService() code but also the > > > conditionals at the callsite of this function all into this and call it > > > something like StartFetchingAnswerImageIfNecessary(). > > > > > > You might also consider initializing |image_service_| in the constructor if > > > doing so is cheap, since in that case lazy-init mostly just makes the code > > more > > > complex. > > > > It was initializing in the constructor and that seemed to cause some unit > tests > > to fail with > > > @@@STEP_LOG_LINE@AppListControllerBrowserTest.CreateNewWindow@[3248:420:0302/125006:FATAL:omnibox_result_view.cc(183)] > > Check failed: image_service_.@@@ > > > > So I was looking for a way to deal with that. Though, it's also possible that > I > > may be misunderstanding the error in the unit test or maybe there's a better > way > > to deal with it. > > > > Here's a link to the stdio from the failing tests: > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... > > > > I think I found 'a solution'. What would be a 'better solution'? > > That doesn't sound like the problem was in initializing this in the constructor, > it sounds like the problem was that later code CHECKed that the initialization > always succeeded, whereas, based on your comments below, this can intentionally > return a null service in tests. > > I would assume, then, that the solution would be to init in the constructor, but > null-check at the time of use. Done. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:182: location_bar_view_->profile()); On 2015/03/03 01:32:36, Peter Kasting wrote: > Nit: Init this in the initializer list Done. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:211: GetAnswerIcon(); On 2015/03/03 01:32:36, Peter Kasting wrote: > It seems sort of weird to have caller code that conditionally calls a function, > and have the function immediately do a further conditional check. Plus, both > sides want to look at match_.answer->second_line().image_url(), so by extracting > the code in GetAnswerIcon() out, we're not really improving any kind of > encapsulation. > > I would respond to all this by collapsing together all this code; either call > the function unconditionally here and move the conditionals there, or (probably > better) just inline the contents of that function into here, since this is the > lone callsite and the function body is short. > > Then the two separate conditionals can be one statement. Done. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:480: // The answer images are not bundled with Chrome, so we need to download On 2015/03/03 01:32:36, Peter Kasting wrote: > Is there a reason the reader would have expected these images to be bundled with > Chrome? If not, I would probably omit this comment. Done. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:79: void SetAnswerImage(gfx::ImageSkia image); On 2015/03/03 01:32:37, Peter Kasting wrote: > Nit: Write a comment about what this does Done. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:81: base::WeakPtr<OmniboxResultView> GetWeakPtr() { On 2015/03/03 01:32:37, Peter Kasting wrote: > You shouldn't inline functions that aren't simple cheap accessors, and you > shouldn't name inlined functions in CamelCase. > > In this case this doesn't even need to be exposed as a function -- the lone > caller could just call weak_ptr_factory_.GetWeakPtr() directly. Done. https://codereview.chromium.org/917333004/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:164: // If the answer has an icon, these control the fetching and updating On 2015/03/03 01:32:37, Peter Kasting wrote: > Nit: Since the |image_service_| is initted unconditionally, it probably belongs > somewhere around where |location_bar_view_| is listed (since that's a similar > "raw pointer to a useful external object"). The other two I might put near the > bottom of the member list (maybe above the mutables?) since unlike most of the > other members they're only going to be occasionally relevant. Done.
LGTM https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:43: // The observer watches for changes in the image being downloaded. Nit: How about something like: Calls back to the OmniboxResultView when the requested image is downloaded. This is a separate class instead of being implemented on OmniboxResultView because BitmapFetcherService currently takes ownership of this object. TODO(dschuyler): Make BitmapFetcherService use the more typical non-owning ObserverList pattern and have OmniboxResultView implement the Observer call directly. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:61: DCHECK(!image.empty()); Just to make sure, if the received image was corrupt or empty, the BitmapFetcherService would detect that and not call us? https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:79: // Store the image in a local data member and schedule a repaint. Nit: Stores/schedules https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:80: void SetAnswerImage(gfx::ImageSkia image); const &? https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:170: BitmapFetcherService* image_service_; Nit: May want to comment that this can be null while testing (assuming it can, which is what I took away from your previous comments).
Sorry, a few more things :( https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:54: const base::WeakPtr<OmniboxResultView> view_; I'm still not entirely convinced this needs to be a WeakPtr, but willing to decouple that discussion from this CL. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:61: DCHECK(!image.empty()); On 2015/03/05 00:33:43, Peter Kasting wrote: > Just to make sure, if the received image was corrupt or empty, the > BitmapFetcherService would detect that and not call us? Nice catch - it actually will always call. See the comment on OnImageChanged: "Called with an empty image if the fetch failed or the request ended for any reason." At this point, we won't get corrupt images. The helper process is there to prevent those from ever making it into Chrome proper. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:210: if (match_.answer && match_.answer->second_line().image_url().is_valid() && There's no need to check URL validity - the fetcher does that for you (and returns an empty image if it's invalid). I also wonder if there's a point in having a match.GetAnswerImageUrl() function. It'd clean this up significantly: image_service_->CancelRequest(request_id_); request_id = image_service_->RequestImage( match_.GetAnswerImageURL(), new AnswersImageObserverDesktop(weak_ptr_factory_.GetWeakPtr())); But even if there isn't, there's no need to check the URLs validity. BitmapFetcherService does that for you https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:212: image_service_->CancelRequest(request_id_); I'd assume you'd want to cancel a pending request even if the new match doesn't have an image - otherwise, a pending image request might come in late and do interesting things :) https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:296: if (!answer_image_.size().IsEmpty()) { Isn't that covered by answer_image_.IsNull()?
https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:296: if (!answer_image_.size().IsEmpty()) { On 2015/03/05 15:44:54, groby wrote: > Isn't that covered by answer_image_.IsNull()? I don't think so, though I'm not sure. It depends whether an empty image is always collapsed to null storage, or if we can have non-null storage that's for a zero-width/zero-height image. The Skia/gfx folks might know better? I suggested this route because it seemed safer than the IsNull() route.
https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:43: // The observer watches for changes in the image being downloaded. On 2015/03/05 00:33:43, Peter Kasting wrote: > Nit: How about something like: > > Calls back to the OmniboxResultView when the requested image is downloaded. > This is a separate class instead of being implemented on OmniboxResultView > because BitmapFetcherService currently takes ownership of this object. > TODO(dschuyler): Make BitmapFetcherService use the more typical non-owning > ObserverList pattern and have OmniboxResultView implement the Observer call > directly. Done. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:61: DCHECK(!image.empty()); On 2015/03/05 00:33:43, Peter Kasting wrote: > Just to make sure, if the received image was corrupt or empty, the > BitmapFetcherService would detect that and not call us? The caller currently does check the image. So the check should never fire unless something changes on that end. Done. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:61: DCHECK(!image.empty()); On 2015/03/05 15:44:54, groby wrote: > On 2015/03/05 00:33:43, Peter Kasting wrote: > > Just to make sure, if the received image was corrupt or empty, the > > BitmapFetcherService would detect that and not call us? > > Nice catch - it actually will always call. See the comment on OnImageChanged: > "Called with an empty image if the fetch failed or the request ended for any > reason." > > At this point, we won't get corrupt images. The helper process is there to > prevent those from ever making it into Chrome proper. I missed the comment, I looked at the code. which should the caller do - check or not check the image? void BitmapFetcherRequest::NotifyImageChanged(const SkBitmap& bitmap) { if (!bitmap.empty()) observer_->OnImageChanged(request_id_, bitmap); } https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:79: // Store the image in a local data member and schedule a repaint. On 2015/03/05 00:33:43, Peter Kasting wrote: > Nit: Stores/schedules Done. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:80: void SetAnswerImage(gfx::ImageSkia image); On 2015/03/05 00:33:43, Peter Kasting wrote: > const &? Both by-value and by-reference are used for ImageSkia. I was passing by value based on the comments below about ImageSkia. Either will work, of course. I'll change it to a const ref. // ImageSkia is cheap to copy and intentionally supports copy semantics. ... // A refptr so that ImageRepSkia can be copied cheaply. scoped_refptr<internal::ImageSkiaStorage> storage_; https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:170: BitmapFetcherService* image_service_; On 2015/03/05 00:33:43, Peter Kasting wrote: > Nit: May want to comment that this can be null while testing (assuming it can, > which is what I took away from your previous comments). Done.
https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:54: const base::WeakPtr<OmniboxResultView> view_; On 2015/03/05 15:44:54, groby wrote: > I'm still not entirely convinced this needs to be a WeakPtr, but willing to > decouple that discussion from this CL. Let's look at that with the change about who owns the image observer. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:210: if (match_.answer && match_.answer->second_line().image_url().is_valid() && On 2015/03/05 15:44:54, groby wrote: > There's no need to check URL validity - the fetcher does that for you (and > returns an empty image if it's invalid). > > I also wonder if there's a point in having a match.GetAnswerImageUrl() function. > It'd clean this up significantly: > > image_service_->CancelRequest(request_id_); > request_id = image_service_->RequestImage( > match_.GetAnswerImageURL(), > new AnswersImageObserverDesktop(weak_ptr_factory_.GetWeakPtr())); > > > But even if there isn't, there's no need to check the URLs validity. > BitmapFetcherService does that for you The accessor for the image url sounds cool, though I'd like to get this feature in :) May I followup with that in another CL? Done. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:212: image_service_->CancelRequest(request_id_); On 2015/03/05 15:44:54, groby wrote: > I'd assume you'd want to cancel a pending request even if the new match doesn't > have an image - otherwise, a pending image request might come in late and do > interesting things :) Ah, good one :) Done. https://codereview.chromium.org/917333004/diff/230003/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:296: if (!answer_image_.size().IsEmpty()) { On 2015/03/05 18:28:36, Peter Kasting wrote: > On 2015/03/05 15:44:54, groby wrote: > > Isn't that covered by answer_image_.IsNull()? > > I don't think so, though I'm not sure. It depends whether an empty image is > always collapsed to null storage, or if we can have non-null storage that's for > a zero-width/zero-height image. The Skia/gfx folks might know better? > > I suggested this route because it seemed safer than the IsNull() route. Is it ok to leave in the empty check for now?
LGTM https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:49: class AnswersImageObserverDesktop : public BitmapFetcherService::Observer { Nit: How come this has such a long name (and plural "Answers" when it only observes for one image)? AnswerImageObserver might be a better name. https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:66: DCHECK(!image.empty()); It sounds like this DCHECK is safe per the code in BitmapFetcherService, but the comment in bitmap_fetcher_service.h says otherwise. Can you update that comment to be correct? (I assume the current behavior of the code is OK, unless Rachel objects.) https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:217: if (match_.answer) Nit: Need {} since body takes >1 physical line https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:301: if (!answer_image_.size().IsEmpty()) { I wasn't thinking through this well enough. A simple !answer_image_.IsNull() check would be preferable. The DCHECK in AnswersImageObserverDesktop::OnImageChanged() guarantees that if this is non-null, it's non-empty as well.
https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:49: class AnswersImageObserverDesktop : public BitmapFetcherService::Observer { On 2015/03/05 23:11:16, Peter Kasting wrote: > Nit: How come this has such a long name (and plural "Answers" when it only > observes for one image)? AnswerImageObserver might be a better name. Done. https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:66: DCHECK(!image.empty()); On 2015/03/05 23:11:16, Peter Kasting wrote: > It sounds like this DCHECK is safe per the code in BitmapFetcherService, but the > comment in bitmap_fetcher_service.h says otherwise. Can you update that comment > to be correct? (I assume the current behavior of the code is OK, unless Rachel > objects.) I added a comment for me to follow up on whether to change the code or the comment. Done. https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:217: if (match_.answer) On 2015/03/05 23:11:16, Peter Kasting wrote: > Nit: Need {} since body takes >1 physical line Done. https://codereview.chromium.org/917333004/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:301: if (!answer_image_.size().IsEmpty()) { On 2015/03/05 23:11:16, Peter Kasting wrote: > I wasn't thinking through this well enough. A simple !answer_image_.IsNull() > check would be preferable. The DCHECK in > AnswersImageObserverDesktop::OnImageChanged() guarantees that if this is > non-null, it's non-empty as well. Done.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/917333004/#ps290001 (title: "nit changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/917333004/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dschuyler@chromium.org changed reviewers: + thestig@chromium.org - groby@chromium.org, pkasting@chromium.org
Please check chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h for a LGTM :)
groby@chromium.org changed reviewers: + groby@chromium.org
No LGTM required for that :) (Funny thing: just saying the word makes it one, anyways ;)
lgtm
On 2015/03/06 21:53:06, groby wrote:
> No LGTM required for that :) (Funny thing: just saying the word makes it one,
> anyways ;)
Ah, I may have been confused by this, I thought it meant I needed it:
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h
The CQ bit was checked by dschuyler@chromium.org
On 2015/03/06 21:57:42, dschuyler wrote: > On 2015/03/06 21:53:06, groby wrote: > > No LGTM required for that :) (Funny thing: just saying the word makes it one, > > anyways ;) > > Ah, I may have been confused by this, I thought it meant I needed it: > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for these files: > chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h You can TBR to an appropriate OWNER in the case where you're making a trivial change like this.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/917333004/290001
Message was sent while issue was closed.
Committed patchset #15 (id:290001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/e92708a2c20888698a7308f370dcc04c14c21e9f Cr-Commit-Position: refs/heads/master@{#319524} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
