Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1071093002:
Implement setSearchResults. (Closed)
|
Created:
5 years, 8 months ago by yawano Modified:
5 years, 8 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, Matt Giuca, tapted, tfarina, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement setSearchResults.
Initial implementation of chrome.launcherSearchProvider.setSearchResults.
iconUrl is not supported with this CL. It will be implemented with another CL.
BUG=440649
Committed: https://crrev.com/6bbf53d7e8ee5fd25918c112896704d90b077a4b
Cr-Commit-Position: refs/heads/master@{#325428}
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Clear results after query is ended. #
Total comments: 30
Patch Set 4 : Rebase. #Patch Set 5 : Address comments. #Patch Set 6 : Use ScopedVector and STLValueDeleter. #Patch Set 7 : Rename to item_id. #
Total comments: 24
Patch Set 8 : Share icon_image between duplicated results. #Patch Set 9 : Add includes. #
Total comments: 12
Patch Set 10 : Move AddObserver to Initialize(). #Patch Set 11 : Fix memory leak. #
Total comments: 4
Patch Set 12 : Add comments. #Messages
Total messages: 26 (3 generated)
yawano@chromium.org changed reviewers: + mgiuca@chromium.org, satorux@chromium.org
PTAL. @satorux: files under chrome/browser/chromeos. @mgiuca: the rest of files. Thank you!
Sorry, I forgot to clear search results after query has ended. Added the logic with Patch Set 3. PTAL. Thank you!
I will review the rest shortly, but just an FYI comment first. https://codereview.chromium.org/1071093002/diff/40001/ui/app_list/search/mixe... File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/1071093002/diff/40001/ui/app_list/search/mixe... ui/app_list/search/mixer.cc:155: const Group& launcher_search_api_group = *groups_[LAUNCHER_SEARCH_API_GROUP]; Heads up: I am going to try and land https://codereview.chromium.org/882463004/ today. That will make these changes unnecessary, as Mixer will now automatically process all groups. (Could you wait for my CL to land, and rebase off that? Thanks.)
https://codereview.chromium.org/1071093002/diff/40001/ui/app_list/search/mixe... File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/1071093002/diff/40001/ui/app_list/search/mixe... ui/app_list/search/mixer.cc:155: const Group& launcher_search_api_group = *groups_[LAUNCHER_SEARCH_API_GROUP]; Okay, I'll wait until http://crrev.com/882463004 lands. After it lands, I'll rebase this CL on it.
Rebased CL. PTAL. Thank you!
https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/launcher_search_provider/service.cc:98: result->item_id, result->icon_url.Pass(), result->relevance, It looks like you are going to trust relevance values given by the extension. You should clip them to the range [0, 4] here to make sure extensions can't give themselves ridiculous or negative values. (You could also do this in the LauncherSearchResult constructor, but I think it makes sense to filter the input at this earlier point.) You should add a kMaxSearchResultScore = 4 because we'll be seeing this value used in a couple of places. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:65: std::vector<linked_ptr<LauncherSearchResult>> results = it.second; const std::vector<...>& Or else it will copy the vector. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:66: for (const auto& result : results) { nit: Don't need curly braces. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h:29: std::vector<linked_ptr<LauncherSearchResult>> extension_results); Hmm. I'm not really keen on having a vector of linked_ptrs here. linked_ptr means the ownership isn't clear, and here the ownership is quite clear: the results get created in Service::SetSearchResults, passed through to SetSearchResults, then stored in the map which continues to own them. Having said that, I've thought quite a bit about how you might do this with a scoped type (scoped_ptr, ScopedVector, etc) and any way I think of it causes trouble. Did you consider anything? The problem is that std::map can't store a ScopedVector. You could try having std::map onto a ScopedVector* and use an STLValueDeleter to clean up the ScopedVector*s. Would that be too much of a mess? To be clear: I'm not 100% against using linked_ptrs here, but I think it's good to explore other options first. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h:39: // Keep search results of each extension. s/Keep/The. (A field should be described as what it *is* not what it *does*.) https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:22: set_id(id); I think there will be a problem of ID conflicts between different extensions and search providers. A malicious extension could set an ID that matches, for example, one of the IDs in the app provider. We should namespace them by extension ID (e.g., "<YOUR-EXTENSION-ID>:<WHATEVER-THE-EXTENSION-WANTS>"). I *think* that should be done here. So the |id| parameter to LauncherSearchResult is "<WHATEVER-THE-EXTENSION-WANTS>", and we prepend the extension-id on to the front before calling set_id (so the ID seen by the Mixer is "<YOUR-EXTENSION-ID>:<WHATEVER-THE-EXTENSION-WANTS>"). (You could also do this in Service::SetSearchResults but I think it makes sense here.) https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:23: set_relevance(static_cast<double>(discrete_value_relevance) / 4.0); Above, I suggested creating kMaxSearchResultScore = 4. Please use that here (static_cast<double>(kMaxSearchResultScore)). Also please add DCHECKs testing discrete_value_relevance's range. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:26: // TODO(yawano) Decode passed icon url and show it baddged with extension badged https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:42: icon_url_ == NULL ? nullptr : new std::string(icon_url_->c_str()); s/NULL/nullptr No need for c_str; just new std::string(*icon_url_). This will be much cleaner if icon_url is not a scoped_ptr. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:45: new LauncherSearchResult(id(), make_scoped_ptr(icon_url), I think this is too expensive (given that you use Duplicate on every result every time new results come in). It looks like the LauncherSearchResult constructor creates a new IconImage from the extension (rather than copying the existing one). I'm not sure how much work there is involved in creating a new IconImage but I clicked through a bit and it looked like quite a lot of ImageSkias and Images are getting created. (I assume it isn't copying pixel data here, but it's still quite a bit of objects being created.) Could we make Duplicate copy the existing extension_icon_image_? Ideally we wouldn't have to call Duplicate at all (but I can't see a good way of avoiding that, given that Add expects a scoped_ptr and you need to hold onto the LauncherSearchResults in the map for future use). https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:58: if (!extension_icon_image_->image_skia().isNull()) Use AsImageSkia instead of image_skia(). Then if it is some other representation, it will be converted. (You should still check isNull.) https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:32: scoped_ptr<std::string> icon_url_; Can this just be a std::string? (Or better, a GURL?) In that case, the constructor should take a const std::string& (or const GURL&). https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:33: const int discrete_value_relevance_; // Must be between 0 and kMaxSearchResultScore.
https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:58: if (!extension_icon_image_->image_skia().isNull()) As discussed, ignore this comment. (I just checked that extension_icon_image_ always has an ImageSkia.)
@mgiuca: Thank you for the review! PTAL. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/launcher_search_provider/service.cc:98: result->item_id, result->icon_url.Pass(), result->relevance, On 2015/04/10 13:29:13, Matt Giuca wrote: > It looks like you are going to trust relevance values given by the extension. > You should clip them to the range [0, 4] here to make sure extensions can't give > themselves ridiculous or negative values. > > (You could also do this in the LauncherSearchResult constructor, but I think it > makes sense to filter the input at this earlier point.) > > You should add a kMaxSearchResultScore = 4 because we'll be seeing this value > used in a couple of places. Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:65: std::vector<linked_ptr<LauncherSearchResult>> results = it.second; Done. Changed to use pointer here. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:66: for (const auto& result : results) { On 2015/04/10 13:29:13, Matt Giuca wrote: > nit: Don't need curly braces. Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h:29: std::vector<linked_ptr<LauncherSearchResult>> extension_results); On 2015/04/10 13:29:14, Matt Giuca wrote: > Hmm. I'm not really keen on having a vector of linked_ptrs here. linked_ptr > means the ownership isn't clear, and here the ownership is quite clear: the > results get created in Service::SetSearchResults, passed through to > SetSearchResults, then stored in the map which continues to own them. > > Having said that, I've thought quite a bit about how you might do this with a > scoped type (scoped_ptr, ScopedVector, etc) and any way I think of it causes > trouble. Did you consider anything? > > The problem is that std::map can't store a ScopedVector. You could try having > std::map onto a ScopedVector* and use an STLValueDeleter to clean up the > ScopedVector*s. Would that be too much of a mess? > > To be clear: I'm not 100% against using linked_ptrs here, but I think it's good > to explore other options first. Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h:39: // Keep search results of each extension. On 2015/04/10 13:29:14, Matt Giuca wrote: > s/Keep/The. (A field should be described as what it *is* not what it *does*.) Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:22: set_id(id); On 2015/04/10 13:29:14, Matt Giuca wrote: > I think there will be a problem of ID conflicts between different extensions and > search providers. > > A malicious extension could set an ID that matches, for example, one of the IDs > in the app provider. We should namespace them by extension ID (e.g., > "<YOUR-EXTENSION-ID>:<WHATEVER-THE-EXTENSION-WANTS>"). > > I *think* that should be done here. So the |id| parameter to > LauncherSearchResult is "<WHATEVER-THE-EXTENSION-WANTS>", and we prepend the > extension-id on to the front before calling set_id (so the ID seen by the Mixer > is "<YOUR-EXTENSION-ID>:<WHATEVER-THE-EXTENSION-WANTS>"). > > (You could also do this in Service::SetSearchResults but I think it makes sense > here.) Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:23: set_relevance(static_cast<double>(discrete_value_relevance) / 4.0); On 2015/04/10 13:29:14, Matt Giuca wrote: > Above, I suggested creating kMaxSearchResultScore = 4. > > Please use that here (static_cast<double>(kMaxSearchResultScore)). > > Also please add DCHECKs testing discrete_value_relevance's range. Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:26: // TODO(yawano) Decode passed icon url and show it baddged with extension On 2015/04/10 13:29:14, Matt Giuca wrote: > badged Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:42: icon_url_ == NULL ? nullptr : new std::string(icon_url_->c_str()); Done. Changed not to use scoped_ptr here. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:45: new LauncherSearchResult(id(), make_scoped_ptr(icon_url), Since icon image loading is done as async, I think it's difficult to simply copy the result. How about to share icon_image with duplicated results? Custom icon loading will also be done as async. i.e. Shares icon_image (loader) between duplicated results, pass the image (pixel data) if image loading is completed. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:32: scoped_ptr<std::string> icon_url_; On 2015/04/10 13:29:14, Matt Giuca wrote: > Can this just be a std::string? (Or better, a GURL?) > > In that case, the constructor should take a const std::string& (or const GURL&). Done. https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:33: const int discrete_value_relevance_; On 2015/04/10 13:29:14, Matt Giuca wrote: > // Must be between 0 and kMaxSearchResultScore. Done.
https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:45: new LauncherSearchResult(id(), make_scoped_ptr(icon_url), You mean actually share the extension::IconImage using a linked_ptr across duplicates of the same LauncherSearchResult? Yeah I would be happy with that. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/launcher_search_provider/service.cc:101: result->icon_url == NULL ? GURL() : GURL(*result->icon_url.get()); result->icon_url ? GURL(*result->icon_url.get()) : GURL() https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:54: ScopedVector<LauncherSearchResult> extension_results) { It is really confusing having |extension_results| and |extension_results_| being two different things. Can you just rename this parameter to |results|? https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:59: extension_results_.erase(extension_id); Unfortunately, I think this will need an explicit delete: delete extension_results_[extension_id]; (The ScopedValueDeleter only deletes during the destruction, not in the erase case.) This is why I was a bit hesitant to recommend switching from linked_ptr, because it makes it a bit harder to avoid leaks. But I think this is still OK if you're careful. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:61: // Adds this extension's results. Adds -> Add (use imperative voice for inline comments; you are not describing a function). https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:66: // Updates results with other extension results. Updates -> Update https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:68: for (const auto& it : extension_results_) { nit: This is not an iterator, so should not be called "it". It is a reference to a map item pair, so call it "item". https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:70: for (const auto& result : *results) nit: Should use "const auto*" to make it clear that this is a pointer type. (I can't find it now, but I'm sure I've read this guideline somewhere.) https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h:40: // The search results of each extension. Because of the danger of raw pointers, add a bit more detail here: // The STLValueDeleter will automatically free the vectors in this map, but elements that are individually erased or replaced must be manually deleted. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:14: const char delimiter = ':'; kResultIdDelimiter. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:35: set_id(extension->id() + delimiter + item_id); // The search result ID is comprised of the extension ID and the extension-supplied item ID. This is to avoid naming collisions for results of different extensions. Optional: It may be a good idea to move this concatenation and comment into a stand-alone function (e.g., MakeSearchResultId). Not really for reuse, but just so it's clearly visible that this is going on, instead of being hidden away in the constructor. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:7: #include <string> https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:12: #include "ui/app_list/search_result.h" #include "url/gurl.h"
@mgiuca: PTAL. Thank you! https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:45: new LauncherSearchResult(id(), make_scoped_ptr(icon_url), Yes. I did it with the new patch set. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/launcher_search_provider/service.cc:101: result->icon_url == NULL ? GURL() : GURL(*result->icon_url.get()); On 2015/04/14 03:15:43, Matt Giuca wrote: > result->icon_url ? GURL(*result->icon_url.get()) : GURL() Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:54: ScopedVector<LauncherSearchResult> extension_results) { On 2015/04/14 03:15:44, Matt Giuca wrote: > It is really confusing having |extension_results| and |extension_results_| being > two different things. > > Can you just rename this parameter to |results|? Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:59: extension_results_.erase(extension_id); On 2015/04/14 03:15:43, Matt Giuca wrote: > Unfortunately, I think this will need an explicit delete: > > delete extension_results_[extension_id]; > > (The ScopedValueDeleter only deletes during the destruction, not in the erase > case.) This is why I was a bit hesitant to recommend switching from linked_ptr, > because it makes it a bit harder to avoid leaks. But I think this is still OK if > you're careful. Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:61: // Adds this extension's results. On 2015/04/14 03:15:43, Matt Giuca wrote: > Adds -> Add > > (use imperative voice for inline comments; you are not describing a function). Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:66: // Updates results with other extension results. On 2015/04/14 03:15:44, Matt Giuca wrote: > Updates -> Update Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:68: for (const auto& it : extension_results_) { On 2015/04/14 03:15:44, Matt Giuca wrote: > nit: This is not an iterator, so should not be called "it". It is a reference to > a map item pair, so call it "item". Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:70: for (const auto& result : *results) On 2015/04/14 03:15:44, Matt Giuca wrote: > nit: Should use "const auto*" to make it clear that this is a pointer type. (I > can't find it now, but I'm sure I've read this guideline somewhere.) Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.h:40: // The search results of each extension. On 2015/04/14 03:15:44, Matt Giuca wrote: > Because of the danger of raw pointers, add a bit more detail here: > // The STLValueDeleter will automatically free the vectors in this map, but > elements that are individually erased or replaced must be manually deleted. Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:14: const char delimiter = ':'; On 2015/04/14 03:15:44, Matt Giuca wrote: > kResultIdDelimiter. Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:35: set_id(extension->id() + delimiter + item_id); On 2015/04/14 03:15:44, Matt Giuca wrote: > // The search result ID is comprised of the extension ID and the > extension-supplied item ID. This is to avoid naming collisions for results of > different extensions. > > Optional: It may be a good idea to move this concatenation and comment into a > stand-alone function (e.g., MakeSearchResultId). Not really for reuse, but just > so it's clearly visible that this is going on, instead of being hidden away in > the constructor. Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h (right): https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:7: On 2015/04/14 03:15:44, Matt Giuca wrote: > #include <string> Done. https://codereview.chromium.org/1071093002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:12: #include "ui/app_list/search_result.h" On 2015/04/14 03:15:44, Matt Giuca wrote: > #include "url/gurl.h" Done.
https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:74: DCHECK(extension_icon_image_ != nullptr); nit: DCHECK(extension_icon_image_); https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:75: extension_icon_image_->AddObserver(this); Ah, I just noticed it looks like you aren't calling AddObserver on the original extension_icon_image (in the main constructor), only in this Duplicate. Could you move this line to Initialize() so it gets called on both?
PTAL. Thank you! https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:74: DCHECK(extension_icon_image_ != nullptr); On 2015/04/15 02:58:48, Matt Giuca wrote: > nit: DCHECK(extension_icon_image_); Done. https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:75: extension_icon_image_->AddObserver(this); In the public constructor, "this" instance is added as observer by passing it in constructor of IconImage. For making it more consistent, I moved AddObserver to Initialize().
Just had a final look over it, with a few more nits and an important memory leak https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:42: extension_results_.clear(); Ah, another case where manual deletion is necessary: STLDeleteElements(extension_results_); https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:45: if (extension_icon_image_ != nullptr) nit: if (extension_icon_image_) https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:40: void Initialize(); nit: Blank lines between these methods.
Thank you for the review! PTAL. https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_provider.cc:42: extension_results_.clear(); Done. For map, we needed to use STLDeleteValues. https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:45: if (extension_icon_image_ != nullptr) Since extension_icon_image is linked_ptr, I think we cannot write in that way. We get an compile error for this. https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:74: DCHECK(extension_icon_image_ != nullptr); DCHECK(extension_icon_image_) hasn't passed the compile since extension_icon_image_ is linked_ptr. https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h:40: void Initialize(); On 2015/04/15 04:17:46, Matt Giuca wrote: > nit: Blank lines between these methods. Done.
Thanks for putting up with my constantly adding new comments to this. I kept noticing new things. LGTM! https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc (right): https://codereview.chromium.org/1071093002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc:45: if (extension_icon_image_ != nullptr) On 2015/04/15 04:43:48, yawano wrote: > Since extension_icon_image is linked_ptr, I think we cannot write in that way. > We get an compile error for this. Acknowledged.
@mgiuca: Thank you for the review!
https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/launcher_search_provider/service.cc:78: std::to_string(query_id_))))); query id is internally uint32. why is it exposed as a string to JavaScript? https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/launcher_search_provider/service.h (right): https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/launcher_search_provider/service.h:22: const int kMaxSearchResultScore = 4; add some comment about why it's 4?
@satorux: Added comments. PTAL. Thank you! @mgiuca: WDYT about changing the queryId(javascript side) to integer? I cannot find any problem of using integer as queryId and it can make the code simpler. https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/launcher_search_provider/service.cc (right): https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/launcher_search_provider/service.cc:78: std::to_string(query_id_))))); Added comment about the conversion. We designed the id as string at first, but there is no reason that we should use string as id. We'll consider if we can change it to integer at javascript side. We'll do it in another CL. https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/launcher_search_provider/service.h (right): https://codereview.chromium.org/1071093002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/launcher_search_provider/service.h:22: const int kMaxSearchResultScore = 4; On 2015/04/16 08:12:25, satorux1 wrote: > add some comment about why it's 4? Done.
LGTM, but the description looks too short. Please describe more about the patch. Re query IDs, fileSystemProvider API uses integers for request Ids. Just FYI. https://developer.chrome.com/apps/fileSystemProvider
On 2015/04/16 11:03:34, satorux1 wrote: > LGTM, but the description looks too short. Please describe more about the patch. > > > Re query IDs, fileSystemProvider API uses integers for request Ids. Just FYI. > > https://developer.chrome.com/apps/fileSystemProvider Thank you! I added the description.
The CQ bit was checked by yawano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/1071093002/#ps220001 (title: "Add comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1071093002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/6bbf53d7e8ee5fd25918c112896704d90b077a4b Cr-Commit-Position: refs/heads/master@{#325428} |
