Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929273002:
Add the Play Store app search to the launcher. (Closed)
|
Created:
3 years, 6 months ago by Jiaquan He Modified:
3 years, 5 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org, tablet-launcher_google.com, weidongg, Qiang(Joe) Xu Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the Play Store app search to the launcher.
Bug=736552
Review-Url: https://codereview.chromium.org/2929273002
Cr-Original-Commit-Position: refs/heads/master@{#482130}
Committed: https://chromium.googlesource.com/chromium/src/+/fa1bfdf6d5fb0952a524f4ca048ff104fdbdb2dd
Review-Url: https://codereview.chromium.org/2929273002
Cr-Commit-Position: refs/heads/master@{#482312}
Committed: https://chromium.googlesource.com/chromium/src/+/7f6b7ec8bcfcbb4cd8267e5af95feb8a91721492
Patch Set 1 #Patch Set 2 : Add the Play Store app search APIs. #
Total comments: 4
Patch Set 3 : Return a list of app search results at once. #
Total comments: 6
Patch Set 4 : Move AppDiscoveryResult into app.mojom. #Patch Set 5 : Merge API and implementation. #
Total comments: 62
Patch Set 6 : Finished comments and merged with the context menu CL. #
Total comments: 46
Patch Set 7 : Resolve Luis comments. #
Total comments: 12
Patch Set 8 : Resolve Luis comments again. #Patch Set 9 : Remove typemapping to make trybot happy. #Patch Set 10 : Implement methods in FakeAppInstance. #
Total comments: 8
Patch Set 11 : Resolve comments & embed AppDiscoveryResultPtr in ArcPlayStoreSearchResult. #Patch Set 12 : Fix the badge not showing issue. #
Total comments: 18
Patch Set 13 : Resolve Xiyuan comments. #
Total comments: 6
Patch Set 14 : Resolve Xiyuan comments & set result type to show separators. #
Total comments: 2
Patch Set 15 : Reuse context menu. #
Total comments: 11
Patch Set 16 : Resolve or reply to Daniel's comments. #Patch Set 17 : Removed unused methods in arc_playstore_search_result.h #Patch Set 18 : Fix unittest build failure. #Messages
Total messages: 92 (56 generated)
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
Can you also include the Chrome-side implementation? Security reviewers require this to evaluate if the APIs are used correctly. I'm also interested in looking at the end-to-end usage of this API to see whether there might be resource leaks. https://codereview.chromium.org/2929273002/diff/20001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/20001/components/arc/common/a... components/arc/common/app.mojom:167: // Notifies that a result app of a Playstore app query is ready. Mention that this is expected to be called multiple times. https://codereview.chromium.org/2929273002/diff/20001/components/arc/common/a... components/arc/common/app.mojom:254: [MinVersion=18] GetRecentAndSuggestedApps@16(string query, nit: [MinVersion=18] GetRecentAndSuggestedApps@16( string query, int32_t max_results, AppDiscoveryResultsReceiver receiver);
https://codereview.chromium.org/2929273002/diff/20001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/20001/components/arc/common/a... components/arc/common/app.mojom:167: // Notifies that a result app of a Playstore app query is ready. On 2017/06/12 21:03:59, Luis Héctor Chávez wrote: > Mention that this is expected to be called multiple times. Done. https://codereview.chromium.org/2929273002/diff/20001/components/arc/common/a... components/arc/common/app.mojom:254: [MinVersion=18] GetRecentAndSuggestedApps@16(string query, On 2017/06/12 21:03:59, Luis Héctor Chávez wrote: > nit: > > [MinVersion=18] GetRecentAndSuggestedApps@16( > string query, int32_t max_results, > AppDiscoveryResultsReceiver receiver); Changed to returning a list of results at once.
the mojom file itself l-g-t-m, but what about the usage? I'm not concerned about resource leakages anymore with this design, so I'll defer the final decision to the security reviewers. just be aware that in the past they have asked developers to provide something to review upfront end-to-end (within Chromium). https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... components/arc/common/app.mojom:238: // Starts a query for Playstore apps. nit: Play Store. https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... components/arc/common/app.mojom:239: [MinVersion=18] GetRecentAndSuggestedApps@16( I think it's better to explicitly mention that this comes from play store in the method name: GetRecentAndSuggestedAppsFromPlayStore. https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... File components/arc/common/app_discovery_result.mojom (right): https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... components/arc/common/app_discovery_result.mojom:8: struct AppDiscoveryResult { It's probably better to have this be in app.mojom. You are not sharing this between other .mojom files, so it doesn't make sense to have it be in a separate file.
https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... components/arc/common/app.mojom:238: // Starts a query for Playstore apps. On 2017/06/15 15:37:25, Luis Héctor Chávez wrote: > nit: Play Store. Done. https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... components/arc/common/app.mojom:239: [MinVersion=18] GetRecentAndSuggestedApps@16( On 2017/06/15 15:37:25, Luis Héctor Chávez wrote: > I think it's better to explicitly mention that this comes from play store in the > method name: > > GetRecentAndSuggestedAppsFromPlayStore. Done. https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... File components/arc/common/app_discovery_result.mojom (right): https://codereview.chromium.org/2929273002/diff/40001/components/arc/common/a... components/arc/common/app_discovery_result.mojom:8: struct AppDiscoveryResult { On 2017/06/15 15:37:25, Luis Héctor Chávez wrote: > It's probably better to have this be in app.mojom. You are not sharing this > between other .mojom files, so it doesn't make sense to have it be in a separate > file. Done.
First round. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:7: #if defined(OS_CHROMEOS) Can you only include this in the compilation if it's in Chrome OS? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:15: Stop(); This is a no-op. Why are you adding it? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:29: UTF16ToUTF8(query), max_results_, does it make sense to make the .mojom use a string16 instead? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:30: base::Bind(&PlaystoreSearchProvider::OnResults, base::Unretained(this))); base::Unretained seems to be unsafe here: there's nothing (obvious) that would prevent this object from being destroyed by the time the callback is invoked. Either use a WeakPtrFactory or call out why it's safe to use base::Unretained in a comment. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:37: for (unsigned long i = 0; i < results.size(); i++) { for (const auto result : results) { but it's probably better to add typemapping so you don't need to do the manual conversion here. See https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... https://cs.chromium.org/chromium/src/components/arc/common/intent_helper.typemap for an example of how to do this. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:8: #include "base/strings/utf_string_conversions.h" You don't need most of these #includes here, and it slows down compilation. Move as many as you can to the .cc file. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:16: #if defined(OS_CHROMEOS) Can't you move this to the place where it is #include'd? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:20: class PlaystoreSearchProvider : public SearchProvider { PlayStore https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:26: void Start(bool is_voice_query, const base::string16& query) override; // app_list::SearchProvider overrides: https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:28: void OnComplete(); unused https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:31: int max_results_; const int max_results_; https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:32: void OnResults(std::vector<arc::mojom::AppDiscoveryResultPtr> results); Methods go before members https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:33: }; DISABLE_COPY_AND_ASSIGN(PlaystoreSearchProvider) https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:30: ~IconSource() override; nit: ~IconSource() override = default; https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:60: CHECK(icon_to_scale); Do not use CHECK: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:87: set_title(base::ASCIIToUTF16(label)); I don't think there's a guarantee that this is ASCII. This should convert from UTF-8. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:93: if (gfx::PNGCodec::Decode( I don't think it's safe to call this from the browser process. Other code has used https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h in the past. See https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/wallpaper/ar... for an example. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:94: reinterpret_cast<const unsigned char*>(&(icon_png_data_[0])), nit: s/&(icon_png_data_[0])/icon_png_data_.data()/ https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:103: source_ = You cannot hold to the created IconSource since gfx::ImageSkia owns it: https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.h?type=cs&l=48 (arguably that parameter should be a std::unique_ptr, but that code predates it). https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:8: #include "base/memory/ptr_util.h" Same comment as the other file. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:14: // TODO: create helpers in app_src_util? Don't add macros in .h files. then again, you're only using it once, so I'd rather you not declare this macro at all. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:27: class PlaystoreSearchResult : public SearchResult { PlayStoreSearchResult https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:38: std::unique_ptr<SearchResult> Duplicate() const override; // app_list::SearchResult overrides: https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:39: no newline between overriden methods https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:44: const std::string kPlayAppPrefix = "play://"; move this to the .cc file into the anonymous namespace: constexpr char kPlayAppPrefix[] = "play://"; https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:58: }; DISALLOW_COPY_AND_ASSIGN(PlayStoreSearchResult) https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:38: constexpr size_t kMaxPlaystoreResults = 2; nit: kMaxPlayStoreResults https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:5: // Next MinVersion: 20 Next MinVersion: 21 https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:89: // Next method ID: 18 Next method ID: 19 https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:249: [MinVersion=18] GetRecentAndSuggestedAppsFromPlayStore@16( MinVersion=20
First round. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:7: #if defined(OS_CHROMEOS) Can you only include this in the compilation if it's in Chrome OS? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:15: Stop(); This is a no-op. Why are you adding it? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:29: UTF16ToUTF8(query), max_results_, does it make sense to make the .mojom use a string16 instead? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:30: base::Bind(&PlaystoreSearchProvider::OnResults, base::Unretained(this))); base::Unretained seems to be unsafe here: there's nothing (obvious) that would prevent this object from being destroyed by the time the callback is invoked. Either use a WeakPtrFactory or call out why it's safe to use base::Unretained in a comment. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:37: for (unsigned long i = 0; i < results.size(); i++) { for (const auto result : results) { but it's probably better to add typemapping so you don't need to do the manual conversion here. See https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... https://cs.chromium.org/chromium/src/components/arc/common/intent_helper.typemap for an example of how to do this. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:8: #include "base/strings/utf_string_conversions.h" You don't need most of these #includes here, and it slows down compilation. Move as many as you can to the .cc file. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:16: #if defined(OS_CHROMEOS) Can't you move this to the place where it is #include'd? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:20: class PlaystoreSearchProvider : public SearchProvider { PlayStore https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:26: void Start(bool is_voice_query, const base::string16& query) override; // app_list::SearchProvider overrides: https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:28: void OnComplete(); unused https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:31: int max_results_; const int max_results_; https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:32: void OnResults(std::vector<arc::mojom::AppDiscoveryResultPtr> results); Methods go before members https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:33: }; DISABLE_COPY_AND_ASSIGN(PlaystoreSearchProvider) https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:30: ~IconSource() override; nit: ~IconSource() override = default; https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:60: CHECK(icon_to_scale); Do not use CHECK: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:87: set_title(base::ASCIIToUTF16(label)); I don't think there's a guarantee that this is ASCII. This should convert from UTF-8. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:93: if (gfx::PNGCodec::Decode( I don't think it's safe to call this from the browser process. Other code has used https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h in the past. See https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/wallpaper/ar... for an example. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:94: reinterpret_cast<const unsigned char*>(&(icon_png_data_[0])), nit: s/&(icon_png_data_[0])/icon_png_data_.data()/ https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:103: source_ = You cannot hold to the created IconSource since gfx::ImageSkia owns it: https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.h?type=cs&l=48 (arguably that parameter should be a std::unique_ptr, but that code predates it). https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:8: #include "base/memory/ptr_util.h" Same comment as the other file. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:14: // TODO: create helpers in app_src_util? Don't add macros in .h files. then again, you're only using it once, so I'd rather you not declare this macro at all. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:27: class PlaystoreSearchResult : public SearchResult { PlayStoreSearchResult https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:38: std::unique_ptr<SearchResult> Duplicate() const override; // app_list::SearchResult overrides: https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:39: no newline between overriden methods https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:44: const std::string kPlayAppPrefix = "play://"; move this to the .cc file into the anonymous namespace: constexpr char kPlayAppPrefix[] = "play://"; https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:58: }; DISALLOW_COPY_AND_ASSIGN(PlayStoreSearchResult) https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:38: constexpr size_t kMaxPlaystoreResults = 2; nit: kMaxPlayStoreResults https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:5: // Next MinVersion: 20 Next MinVersion: 21 https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:89: // Next method ID: 18 Next method ID: 19 https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:249: [MinVersion=18] GetRecentAndSuggestedAppsFromPlayStore@16( MinVersion=20
Description was changed from ========== Add the Play Store app search APIs. Bug=719523 ========== to ========== Add the Play Store app search to the launcher. Bug=719523 ==========
https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:80: string launch_intent_uri; All the strings seem to be optional from the Android-side change. make them all "string?", please.
I think it's mostly done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:7: #if defined(OS_CHROMEOS) On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > Can you only include this in the compilation if it's in Chrome OS? Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:15: Stop(); On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > This is a no-op. Why are you adding it? Done. Not needed any more. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:29: UTF16ToUTF8(query), max_results_, On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > does it make sense to make the .mojom use a string16 instead? If we use mojo string16, do we have to convert it to java String on the Android side? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:30: base::Bind(&PlaystoreSearchProvider::OnResults, base::Unretained(this))); On 2017/06/16 22:24:31, Luis Héctor Chávez wrote: > base::Unretained seems to be unsafe here: there's nothing (obvious) that would > prevent this object from being destroyed by the time the callback is invoked. > Either use a WeakPtrFactory or call out why it's safe to use base::Unretained in > a comment. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:37: for (unsigned long i = 0; i < results.size(); i++) { On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > for (const auto result : results) { > > but it's probably better to add typemapping so you don't need to do the manual > conversion here. > > See > https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... > https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... > https://cs.chromium.org/chromium/src/components/arc/common/intent_helper.typemap > for an example of how to do this. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:8: #include "base/strings/utf_string_conversions.h" On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > You don't need most of these #includes here, and it slows down compilation. Move > as many as you can to the .cc file. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:16: #if defined(OS_CHROMEOS) On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > Can't you move this to the place where it is #include'd? Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:20: class PlaystoreSearchProvider : public SearchProvider { On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > PlayStore Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:26: void Start(bool is_voice_query, const base::string16& query) override; On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > // app_list::SearchProvider overrides: Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:28: void OnComplete(); On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > unused Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:31: int max_results_; On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > const int max_results_; Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:32: void OnResults(std::vector<arc::mojom::AppDiscoveryResultPtr> results); On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > Methods go before members Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:33: }; On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > DISABLE_COPY_AND_ASSIGN(PlaystoreSearchProvider) Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:30: ~IconSource() override; On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > nit: ~IconSource() override = default; Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:60: CHECK(icon_to_scale); On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > Do not use CHECK: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:87: set_title(base::ASCIIToUTF16(label)); On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > I don't think there's a guarantee that this is ASCII. This should convert from > UTF-8. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:93: if (gfx::PNGCodec::Decode( On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > I don't think it's safe to call this from the browser process. Other code has > used https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h in the > past. > > See > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/wallpaper/ar... > for an example. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:94: reinterpret_cast<const unsigned char*>(&(icon_png_data_[0])), On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > nit: s/&(icon_png_data_[0])/icon_png_data_.data()/ Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:103: source_ = On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > You cannot hold to the created IconSource since gfx::ImageSkia owns it: > https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.h?type=cs&l=48 > > (arguably that parameter should be a std::unique_ptr, but that code predates > it). Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:8: #include "base/memory/ptr_util.h" On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > Same comment as the other file. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:14: // TODO: create helpers in app_src_util? On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > Don't add macros in .h files. then again, you're only using it once, so I'd > rather you not declare this macro at all. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:27: class PlaystoreSearchResult : public SearchResult { On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > PlayStoreSearchResult Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:38: std::unique_ptr<SearchResult> Duplicate() const override; On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > // app_list::SearchResult overrides: Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:39: On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > no newline between overriden methods Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:44: const std::string kPlayAppPrefix = "play://"; On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > move this to the .cc file into the anonymous namespace: > > constexpr char kPlayAppPrefix[] = "play://"; Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:58: }; On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > DISALLOW_COPY_AND_ASSIGN(PlayStoreSearchResult) Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:38: constexpr size_t kMaxPlaystoreResults = 2; On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > nit: kMaxPlayStoreResults Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:5: // Next MinVersion: 20 On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > Next MinVersion: 21 Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:80: string launch_intent_uri; On 2017/06/19 17:51:53, Luis Héctor Chávez wrote: > All the strings seem to be optional from the Android-side change. make them all > "string?", please. Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:89: // Next method ID: 18 On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > Next method ID: 19 Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:249: [MinVersion=18] GetRecentAndSuggestedAppsFromPlayStore@16( On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > MinVersion=20 Done.
I think it's mostly done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:7: #if defined(OS_CHROMEOS) On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > Can you only include this in the compilation if it's in Chrome OS? Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:15: Stop(); On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > This is a no-op. Why are you adding it? Done. Not needed any more. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:29: UTF16ToUTF8(query), max_results_, On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > does it make sense to make the .mojom use a string16 instead? If we use mojo string16, do we have to convert it to java String on the Android side? https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:30: base::Bind(&PlaystoreSearchProvider::OnResults, base::Unretained(this))); On 2017/06/16 22:24:31, Luis Héctor Chávez wrote: > base::Unretained seems to be unsafe here: there's nothing (obvious) that would > prevent this object from being destroyed by the time the callback is invoked. > Either use a WeakPtrFactory or call out why it's safe to use base::Unretained in > a comment. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:37: for (unsigned long i = 0; i < results.size(); i++) { On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > for (const auto result : results) { > > but it's probably better to add typemapping so you don't need to do the manual > conversion here. > > See > https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... > https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_filt... > https://cs.chromium.org/chromium/src/components/arc/common/intent_helper.typemap > for an example of how to do this. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:8: #include "base/strings/utf_string_conversions.h" On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > You don't need most of these #includes here, and it slows down compilation. Move > as many as you can to the .cc file. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:16: #if defined(OS_CHROMEOS) On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > Can't you move this to the place where it is #include'd? Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:20: class PlaystoreSearchProvider : public SearchProvider { On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > PlayStore Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:26: void Start(bool is_voice_query, const base::string16& query) override; On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > // app_list::SearchProvider overrides: Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:28: void OnComplete(); On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > unused Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:31: int max_results_; On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > const int max_results_; Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:32: void OnResults(std::vector<arc::mojom::AppDiscoveryResultPtr> results); On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > Methods go before members Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:33: }; On 2017/06/16 22:24:32, Luis Héctor Chávez wrote: > DISABLE_COPY_AND_ASSIGN(PlaystoreSearchProvider) Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:30: ~IconSource() override; On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > nit: ~IconSource() override = default; Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:60: CHECK(icon_to_scale); On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > Do not use CHECK: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:87: set_title(base::ASCIIToUTF16(label)); On 2017/06/16 22:24:33, Luis Héctor Chávez wrote: > I don't think there's a guarantee that this is ASCII. This should convert from > UTF-8. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:93: if (gfx::PNGCodec::Decode( On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > I don't think it's safe to call this from the browser process. Other code has > used https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h in the > past. > > See > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/wallpaper/ar... > for an example. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:94: reinterpret_cast<const unsigned char*>(&(icon_png_data_[0])), On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > nit: s/&(icon_png_data_[0])/icon_png_data_.data()/ Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:103: source_ = On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > You cannot hold to the created IconSource since gfx::ImageSkia owns it: > https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.h?type=cs&l=48 > > (arguably that parameter should be a std::unique_ptr, but that code predates > it). Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:8: #include "base/memory/ptr_util.h" On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > Same comment as the other file. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:14: // TODO: create helpers in app_src_util? On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > Don't add macros in .h files. then again, you're only using it once, so I'd > rather you not declare this macro at all. Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:27: class PlaystoreSearchResult : public SearchResult { On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > PlayStoreSearchResult Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:38: std::unique_ptr<SearchResult> Duplicate() const override; On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > // app_list::SearchResult overrides: Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:39: On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > no newline between overriden methods Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:44: const std::string kPlayAppPrefix = "play://"; On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > move this to the .cc file into the anonymous namespace: > > constexpr char kPlayAppPrefix[] = "play://"; Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:58: }; On 2017/06/16 22:24:34, Luis Héctor Chávez wrote: > DISALLOW_COPY_AND_ASSIGN(PlayStoreSearchResult) Done. https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:38: constexpr size_t kMaxPlaystoreResults = 2; On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > nit: kMaxPlayStoreResults Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:5: // Next MinVersion: 20 On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > Next MinVersion: 21 Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:80: string launch_intent_uri; On 2017/06/19 17:51:53, Luis Héctor Chávez wrote: > All the strings seem to be optional from the Android-side change. make them all > "string?", please. Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:89: // Next method ID: 18 On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > Next method ID: 19 Done. https://codereview.chromium.org/2929273002/diff/80001/components/arc/common/a... components/arc/common/app.mojom:249: [MinVersion=18] GetRecentAndSuggestedAppsFromPlayStore@16( On 2017/06/16 22:24:35, Luis Héctor Chávez wrote: > MinVersion=20 Done.
Can you run trybots with `git cl try`? Once they are all green, you might want to consider starting to add the rest of the reviewers (use `git cl owners` for suggestions). Also, can you run `git cl format` and `git cl lint`? https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/playstore_app_context_menu.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/playstore_app_context_menu.cc:16: PlayStoreAppContextMenu::~PlayStoreAppContextMenu() {} nit: = default; https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/playstore_app_context_menu.cc:30: return false; unneeded https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/playstore_app_context_menu.h (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/playstore_app_context_menu.h:35: void IsAppOpen(); These two methods seem to be unused. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result.typemap (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result.typemap:11: type_mappings = [ "arc.mojom.AppDiscoveryResult=::app_list::PlayStoreSearchResultUniquePtr[move_only]" ] arc.mojom.AppDiscoveryResult=std::unique_ptr<app_list::PlayStoreSearchResult>[move_only] https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:24: PlayStoreSearchProvider::~PlayStoreSearchProvider() {} nit: = default https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:28: // TODO: should search for voice? please file a (Chromium) bug for this. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:50: for (unsigned long i = 0; i < results.size(); i++) { for (const auto& result : results) (you should also be using size_t instead of unsigned long to match std::vector::size()'s return type). https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:18: // app_list::SearchProvider overrides; Move this to after L24. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:29: void OnResults(std::vector<PlayStoreSearchResultUniquePtr> results); Newline below the methods. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:31: Profile* profile_; Profile* const profile_; Also, comment about its lifetime. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:34: AppListControllerDelegate* list_controller_; const https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:28: #if defined(OS_CHROMEOS) Remove. Only add this file to the build if Chrome OS. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:73: if (decoded_icon_ != NULL) { nullptr https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:174: PlayStoreSearchResult::~PlayStoreSearchResult() {} nit: = default; https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:12: #if defined(OS_CHROMEOS) Remove. Move this #if to where you are including it instead. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:23: // app_list::SearchResult overrides: Move this to after L34. The ctors/dtor are typically not mentioned in the override lists. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:24: PlayStoreSearchResult(); Why do you need this constructor? https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:37: void ExecuteLaunchCommand(int event_flags) override; Move this to after L39 and add an overrides: comment. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:77: class IconSource; You are not referring to IconSource anywhere in the .h, so remove this and move IconSource to the anonymous namespace in the .cc https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:92: // profile_ is ownd by ProfileInfo. nit: |profile_| is owned https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:94: // list_controller_ is owned by AppListServiceAsh and lives nit: |list_controller_| https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:104: using PlayStoreSearchResultUniquePtr = std::unique_ptr<PlayStoreSearchResult>; remove https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/search_controller_factory.cc:118: size_t playstore_api_group_id = here is where you need to add the #if defined(OS_CHROMEOS)
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done all comments and I'll upload a PS first. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/playstore_app_context_menu.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/playstore_app_context_menu.cc:16: PlayStoreAppContextMenu::~PlayStoreAppContextMenu() {} On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > nit: = default; Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/playstore_app_context_menu.cc:30: return false; On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > unneeded Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/playstore_app_context_menu.h (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/playstore_app_context_menu.h:35: void IsAppOpen(); On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > These two methods seem to be unused. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result.typemap (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result.typemap:11: type_mappings = [ "arc.mojom.AppDiscoveryResult=::app_list::PlayStoreSearchResultUniquePtr[move_only]" ] On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > arc.mojom.AppDiscoveryResult=std::unique_ptr<app_list::PlayStoreSearchResult>[move_only] Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:24: PlayStoreSearchProvider::~PlayStoreSearchProvider() {} On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > nit: = default Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:28: // TODO: should search for voice? On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > please file a (Chromium) bug for this. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:50: for (unsigned long i = 0; i < results.size(); i++) { On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > for (const auto& result : results) > > (you should also be using size_t instead of unsigned long to match > std::vector::size()'s return type). Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:18: // app_list::SearchProvider overrides; On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > Move this to after L24. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:29: void OnResults(std::vector<PlayStoreSearchResultUniquePtr> results); On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > Newline below the methods. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:31: Profile* profile_; On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > Profile* const profile_; > > Also, comment about its lifetime. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.h:34: AppListControllerDelegate* list_controller_; On 2017/06/22 15:42:09, Luis Héctor Chávez wrote: > const Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:28: #if defined(OS_CHROMEOS) On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > Remove. Only add this file to the build if Chrome OS. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:73: if (decoded_icon_ != NULL) { On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > nullptr Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:174: PlayStoreSearchResult::~PlayStoreSearchResult() {} On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > nit: = default; Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:12: #if defined(OS_CHROMEOS) On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > Remove. Move this #if to where you are including it instead. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:23: // app_list::SearchResult overrides: On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > Move this to after L34. The ctors/dtor are typically not mentioned in the > override lists. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:24: PlayStoreSearchResult(); On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > Why do you need this constructor? Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:37: void ExecuteLaunchCommand(int event_flags) override; On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > Move this to after L39 and add an overrides: comment. Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:77: class IconSource; On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > You are not referring to IconSource anywhere in the .h, so remove this and move > IconSource to the anonymous namespace in the .cc Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:92: // profile_ is ownd by ProfileInfo. On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > nit: |profile_| is owned Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:94: // list_controller_ is owned by AppListServiceAsh and lives On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > nit: |list_controller_| Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:104: using PlayStoreSearchResultUniquePtr = std::unique_ptr<PlayStoreSearchResult>; On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > remove Done. https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/search_controller_factory.cc:118: size_t playstore_api_group_id = On 2017/06/22 15:42:10, Luis Héctor Chávez wrote: > here is where you need to add the #if defined(OS_CHROMEOS) Done.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
The CQ bit was checked by hejq@chromium.org
The CQ bit was unchecked by hejq@chromium.org
https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result_traits.cc (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result_traits.cc:7: #include <memory> Leave only utility https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:7: #include <memory> Remove memory and vector. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:32: // TODO(crbug/736027): should search for voice? nit: crbug.com/736027 https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:54: // for (size_t i = 0; i < results.size(); i++) { Never leave commented-out code. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:7: #include <memory> You don't need to add these here: You can rely on anything being #include'd in the .h to be available. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:35: // app_list::SearchResult overrides: nit: add newlines around override blocks. So one before this line, and another one after L36.
Resolve Luis comments again. And I'm still trying to play with try scripts https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result_traits.cc (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/mojo/app_discovery_result_traits.cc:7: #include <memory> On 2017/06/22 19:49:17, Luis Héctor Chávez wrote: > Leave only utility Done. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:7: #include <memory> On 2017/06/22 19:49:17, Luis Héctor Chávez wrote: > Remove memory and vector. Done. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:32: // TODO(crbug/736027): should search for voice? On 2017/06/22 19:49:17, Luis Héctor Chávez wrote: > nit: crbug.com/736027 Done. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_provider.cc:54: // for (size_t i = 0; i < results.size(); i++) { On 2017/06/22 19:49:17, Luis Héctor Chávez wrote: > Never leave commented-out code. Oh my negligence. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.cc:7: #include <memory> On 2017/06/22 19:49:17, Luis Héctor Chávez wrote: > You don't need to add these here: You can rely on anything being #include'd in > the .h to be available. Done. https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:35: // app_list::SearchResult overrides: On 2017/06/22 19:49:17, Luis Héctor Chávez wrote: > nit: add newlines around override blocks. So one before this line, and another > one after L36. Done.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
For the new PS: - I'm removing typemapping, because it will take PlayStoreSearchResult to //components/arc, but the class depends on //chrome/browser/ui/app_list/app_context_menu_delegate.h. - I'm expanding the list of fields in PlayStoreSearchResult instead of putting arc::mojom::AppDiscoveryResultPtr into PlayStoreSearchResult, because in the later mixing and publishing stage contents of PlayStoreSearchResult will be copied.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cool, we're down to nits here too. Now's a good time to add the rest of the reviewers. https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:3353: "app_list/search/playstore/playstore_search_provider.cc", For consistency, can you rename these files to app_list/search/arc/arc_playstore_search_... ? https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:3421: "app_list/playstore_app_context_menu.h", same here: app_list/arc/arc_playstore_... https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:34: Profile* const profile, It's not customary to add '* const' to pointers in parameters, only in members when you know you don't expect to modify where the reference points to. https://codereview.chromium.org/2929273002/diff/180001/components/arc/test/fa... File components/arc/test/fake_app_instance.cc (right): https://codereview.chromium.org/2929273002/diff/180001/components/arc/test/fa... components/arc/test/fake_app_instance.cc:273: const GetRecentAndSuggestedAppsFromPlayStoreCallback& callback) {} other methods invoke their callbacks to avoid the caller from waiting forever: callback.Run({});
Done. https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:3353: "app_list/search/playstore/playstore_search_provider.cc", On 2017/06/23 15:17:02, Luis Héctor Chávez wrote: > For consistency, can you rename these files to > > app_list/search/arc/arc_playstore_search_... > > ? Done. https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:3421: "app_list/playstore_app_context_menu.h", On 2017/06/23 15:17:02, Luis Héctor Chávez wrote: > same here: > > app_list/arc/arc_playstore_... Done. https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/playstore/playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/playstore/playstore_search_result.h:34: Profile* const profile, On 2017/06/23 15:17:03, Luis Héctor Chávez wrote: > It's not customary to add '* const' to pointers in parameters, only in members > when you know you don't expect to modify where the reference points to. Done. https://codereview.chromium.org/2929273002/diff/180001/components/arc/test/fa... File components/arc/test/fake_app_instance.cc (right): https://codereview.chromium.org/2929273002/diff/180001/components/arc/test/fa... components/arc/test/fake_app_instance.cc:273: const GetRecentAndSuggestedAppsFromPlayStoreCallback& callback) {} On 2017/06/23 15:17:03, Luis Héctor Chávez wrote: > other methods invoke their callbacks to avoid the caller from waiting forever: > > callback.Run({}); Done.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hejq@chromium.org changed reviewers: + xiyuan@chromium.org
The CQ bit was checked by hejq@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:43: IconSource(const SkBitmap& decoded_bitmap, int resource_size_in_dip); Prefer to have only one constructor and add a SetDecodedImage method. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:51: std::unique_ptr<gfx::ImageSkia> decoded_icon_; std::unique_ptr<> is not necessary. We can test empty ImageSkia by checking .isNull(). https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:136: constexpr SkColor kBadgeColor = SkColorSetARGBMacro(0x8A, 0x00, 0x00, 0x00); This is used in one place. Move it close to where it is used. Or put it around kPlayAppPrefix to avoid having to anonymous namespace section. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:146: weak_ptr_factory_(this) { weak_ptr_factory_ seems not used. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:42: arc::mojom::AppDiscoveryResultPtr data_; Data member should be declared after all methods https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:43: const base::Optional<std::string> launch_intent_uri() { Make it a const method and return a const &, here and other similar accessors i.e. const base::Optional<std::string>& launch_intent_uri() const { ... } https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:59: const std::vector<uint8_t> icon_png_data() { return data_->icon_png_data; } nit: const std::vector<uint8_t>&, to avoid copy https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/search_controller_factory.cc:118: #if defined(OS_CHROMEOS) Is this necessary? If it is, we need to put header under it too. https://codereview.chromium.org/2929273002/diff/220001/components/arc/common/... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/220001/components/arc/common/... components/arc/common/app.mojom:91: // Next method ID: 19 Seems updated the wrong one... should update the one on line 177
Done. Thanks. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:43: IconSource(const SkBitmap& decoded_bitmap, int resource_size_in_dip); On 2017/06/23 19:57:38, xiyuan wrote: > Prefer to have only one constructor and add a SetDecodedImage method. Done. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:51: std::unique_ptr<gfx::ImageSkia> decoded_icon_; On 2017/06/23 19:57:38, xiyuan wrote: > std::unique_ptr<> is not necessary. We can test empty ImageSkia by checking > .isNull(). Done. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:136: constexpr SkColor kBadgeColor = SkColorSetARGBMacro(0x8A, 0x00, 0x00, 0x00); On 2017/06/23 19:57:38, xiyuan wrote: > This is used in one place. Move it close to where it is used. Or put it around > kPlayAppPrefix to avoid having to anonymous namespace section. Done. Moved under kPlayAppPrefix https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:146: weak_ptr_factory_(this) { On 2017/06/23 19:57:38, xiyuan wrote: > weak_ptr_factory_ seems not used. Done. Removed. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:42: arc::mojom::AppDiscoveryResultPtr data_; On 2017/06/23 19:57:38, xiyuan wrote: > Data member should be declared after all methods Done. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:43: const base::Optional<std::string> launch_intent_uri() { On 2017/06/23 19:57:38, xiyuan wrote: > Make it a const method and return a const &, here and other similar accessors > i.e. > const base::Optional<std::string>& launch_intent_uri() const { > ... > } Done. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:59: const std::vector<uint8_t> icon_png_data() { return data_->icon_png_data; } On 2017/06/23 19:57:38, xiyuan wrote: > nit: const std::vector<uint8_t>&, to avoid copy Done. https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/2929273002/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/search_controller_factory.cc:118: #if defined(OS_CHROMEOS) On 2017/06/23 19:57:39, xiyuan wrote: > Is this necessary? If it is, we need to put header under it too. Done. https://codereview.chromium.org/2929273002/diff/220001/components/arc/common/... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/220001/components/arc/common/... components/arc/common/app.mojom:91: // Next method ID: 19 On 2017/06/23 19:57:39, xiyuan wrote: > Seems updated the wrong one... should update the one on line 177 Done.
lgtm with one last request https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:106: gfx::Size resource_size(app_list::kGridIconDimension, nit: const gfx::Size https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:119: gfx::Size resource_size(app_list::kGridIconDimension, nit: const gfx::Size https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:131: IconSource* icon_source_; Can we get rid of this member and new it where it is needed?
Done, thanks!! https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:106: gfx::Size resource_size(app_list::kGridIconDimension, On 2017/06/23 22:06:33, xiyuan wrote: > nit: const gfx::Size Done. https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:119: gfx::Size resource_size(app_list::kGridIconDimension, On 2017/06/23 22:06:33, xiyuan wrote: > nit: const gfx::Size Done. https://codereview.chromium.org/2929273002/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:131: IconSource* icon_source_; On 2017/06/23 22:06:32, xiyuan wrote: > Can we get rid of this member and new it where it is needed? Done.
lgtm with one more nit https://codereview.chromium.org/2929273002/diff/260001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/260001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:186: context_menu_ = base::MakeUnique<ArcPlayStoreAppContextMenu>( does it make sense to check if |context_menu_| had already been created? if (!context_menu_) { // ... }
https://codereview.chromium.org/2929273002/diff/260001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/260001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:186: context_menu_ = base::MakeUnique<ArcPlayStoreAppContextMenu>( On 2017/06/23 22:47:17, Luis Héctor Chávez wrote: > does it make sense to check if |context_menu_| had already been created? > > if (!context_menu_) { > // ... > } Yes it makes sense.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hejq@chromium.org changed reviewers: + nasko@chromium.org
hejq@chromium.org changed reviewers: + dcheng@chromium.org - nasko@chromium.org
https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_provider.cc:53: Add(base::MakeUnique<ArcPlayStoreSearchResult>(std::move(result), profile_, Is it a problem if results.size() > max_results_? https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:182: base::Optional<gfx::Rect>()); Nit: base::nullopt https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:42: const base::Optional<std::string>& launch_intent_uri() const { Are these getters used? https://codereview.chromium.org/2929273002/diff/270001/components/arc/common/... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/270001/components/arc/common/... components/arc/common/app.mojom:81: string? install_intent_uri; Are these actual URLs? We're consuming them in Chrome right? What does an intent URL do? What are we doing with them? (Also, normally, we'd just use url.mojom.Url to represent URLs, but 1) maybe the intent URIs don't fit into that and 2) I guess ARC++ is trying to limit its dependencies on non-ARC++ structs)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_provider.cc:53: Add(base::MakeUnique<ArcPlayStoreSearchResult>(std::move(result), profile_, On 2017/06/24 00:16:47, dcheng wrote: > Is it a problem if results.size() > max_results_? The Play Store service guarantees that the amount of returned results won't exceed max_results_. If it somehow fails to do this, which should never happen, these results will be sent to a mixer to mixed with other types of results, and it will again make sure results of this type won't be more than max_results_. https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:182: base::Optional<gfx::Rect>()); On 2017/06/24 00:16:47, dcheng wrote: > Nit: base::nullopt Done. https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:42: const base::Optional<std::string>& launch_intent_uri() const { On 2017/06/24 00:16:47, dcheng wrote: > Are these getters used? Not all of them are used at this moment, but we'll use them in the future. Should I remove them now and add them back later? https://codereview.chromium.org/2929273002/diff/270001/components/arc/common/... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/270001/components/arc/common/... components/arc/common/app.mojom:81: string? install_intent_uri; On 2017/06/24 00:16:47, dcheng wrote: > Are these actual URLs? We're consuming them in Chrome right? What does an intent > URL do? What are we doing with them? > > (Also, normally, we'd just use url.mojom.Url to represent URLs, but 1) maybe the > intent URIs don't fit into that and 2) I guess ARC++ is trying to limit its > dependencies on non-ARC++ structs) A launch_intent_uri here is used to start an Android activity, which looks like "#Intent;component=com.android.browser/com.android.BrowserActivity;end" An install_intent_uri is used to guide the user to a Play Store installation page of an Android application, which is an URL and looks like "https://play.google.com/store/apps/details?id=shu.gear.data#Intent;package=com.android.vending;component=com.android.vending/com.google.android.finsky.appdiscoveryservice.AppDiscoveryLaunchActivity;l.com.google.android.finsky.analytics.LoggingContext.KEY_LAST_EVENT_ID=699526169;S.com.google.android.finsky.analytics.LoggingContext.KEY_USE_DEFAULT_LOGGER=true;i.sessionId=0;i.requestCode=8;i.callingVersionCode=25;S.callingPackageName=org.chromium.arc.gms;end". Other ARC++ interfaces are also using "string" for these intents.
Description was changed from ========== Add the Play Store app search to the launcher. Bug=719523 ========== to ========== Add the Play Store app search to the launcher. Bug=736552 ==========
ipc lgtm https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_provider.cc (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_provider.cc:53: Add(base::MakeUnique<ArcPlayStoreSearchResult>(std::move(result), profile_, On 2017/06/24 00:48:08, Jiaquan He wrote: > On 2017/06/24 00:16:47, dcheng wrote: > > Is it a problem if results.size() > max_results_? > > The Play Store service guarantees that the amount of returned results won't > exceed max_results_. > > If it somehow fails to do this, which should never happen, these results will be > sent to a mixer to mixed with other types of results, and it will again make > sure results of this type won't be more than max_results_. Right, but we don't trust the ARC++ process as much. So it could return more than max items. As long as it's handled correctly, I'm OK with that. https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h (right): https://codereview.chromium.org/2929273002/diff/270001/chrome/browser/ui/app_... chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h:42: const base::Optional<std::string>& launch_intent_uri() const { On 2017/06/24 00:48:08, Jiaquan He wrote: > On 2017/06/24 00:16:47, dcheng wrote: > > Are these getters used? > > Not all of them are used at this moment, but we'll use them in the future. > Should I remove them now and add them back later? It's generally nicer to add code when it's actually used. https://codereview.chromium.org/2929273002/diff/270001/components/arc/common/... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2929273002/diff/270001/components/arc/common/... components/arc/common/app.mojom:81: string? install_intent_uri; On 2017/06/24 00:48:08, Jiaquan He wrote: > On 2017/06/24 00:16:47, dcheng wrote: > > Are these actual URLs? We're consuming them in Chrome right? What does an > intent > > URL do? What are we doing with them? > > > > (Also, normally, we'd just use url.mojom.Url to represent URLs, but 1) maybe > the > > intent URIs don't fit into that and 2) I guess ARC++ is trying to limit its > > dependencies on non-ARC++ structs) > > A launch_intent_uri here is used to start an Android activity, which looks like > "#Intent;component=com.android.browser/com.android.BrowserActivity;end" > > An install_intent_uri is used to guide the user to a Play Store installation > page of an Android application, which is an URL and looks like > "https://play.google.com/store/apps/details?id=shu.gear.data#Intent;package=com.android.vending;component=com.android.vending/com.google.android.finsky.appdiscoveryservice.AppDiscoveryLaunchActivity;l.com.google.android.finsky.analytics.LoggingContext.KEY_LAST_EVENT_ID=699526169;S.com.google.android.finsky.analytics.LoggingContext.KEY_USE_DEFAULT_LOGGER=true;i.sessionId=0;i.requestCode=8;i.callingVersionCode=25;S.callingPackageName=org.chromium.arc.gms;end". > > Other ARC++ interfaces are also using "string" for these intents. OK, I think it's still worth having a discussion about these, but that's out of scope of this CL.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hejq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2929273002/#ps300001 (title: "Removed unused methods in arc_playstore_search_result.h")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1498273742104940,
"parent_rev": "afa29c90694249cef72239bccf9e96b6341c2a1b", "commit_rev":
"fa1bfdf6d5fb0952a524f4ca048ff104fdbdb2dd"}
Message was sent while issue was closed.
Description was changed from ========== Add the Play Store app search to the launcher. Bug=736552 ========== to ========== Add the Play Store app search to the launcher. Bug=736552 Review-Url: https://codereview.chromium.org/2929273002 Cr-Commit-Position: refs/heads/master@{#482130} Committed: https://chromium.googlesource.com/chromium/src/+/fa1bfdf6d5fb0952a524f4ca048f... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:300001) as https://chromium.googlesource.com/chromium/src/+/fa1bfdf6d5fb0952a524f4ca048f...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:300001) has been created in https://codereview.chromium.org/2954223002/ by dcheng@chromium.org. The reason for reverting is: Linux ChromiumOS Builder (dbg) is failing compile: ../../chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:0: error: undefined reference to 'app_list::kIcBadgeInstantIcon' ../../chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc:0: error: undefined reference to 'app_list::kIcBadgePlayIcon'.
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 482130 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
Description was changed from ========== Add the Play Store app search to the launcher. Bug=736552 Review-Url: https://codereview.chromium.org/2929273002 Cr-Commit-Position: refs/heads/master@{#482130} Committed: https://chromium.googlesource.com/chromium/src/+/fa1bfdf6d5fb0952a524f4ca048f... ========== to ========== Add the Play Store app search to the launcher. Bug=736552 Review-Url: https://codereview.chromium.org/2929273002 Cr-Commit-Position: refs/heads/master@{#482130} Committed: https://chromium.googlesource.com/chromium/src/+/fa1bfdf6d5fb0952a524f4ca048f... ==========
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi reviewers, This CL was reverted due to a build failure after it was landed. Could you help review the latest patch set? Thanks
Delta l-g-t-m. Did you try to reproduce the failure locally and verified that your fix does indeed solve the issue?
On 2017/06/26 17:21:31, Luis Héctor Chávez wrote: > Delta l-g-t-m. > > Did you try to reproduce the failure locally and verified that your fix does > indeed solve the issue? Yes I did. With this PS all those unit tests are built succesfully.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hejq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2929273002/#ps320001 (title: "Fix unittest build failure.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1498499095104130,
"parent_rev": "c7616c7db94822e05489004042923b0df02882ed", "commit_rev":
"7f6b7ec8bcfcbb4cd8267e5af95feb8a91721492"}
Message was sent while issue was closed.
Description was changed from ========== Add the Play Store app search to the launcher. Bug=736552 Review-Url: https://codereview.chromium.org/2929273002 Cr-Commit-Position: refs/heads/master@{#482130} Committed: https://chromium.googlesource.com/chromium/src/+/fa1bfdf6d5fb0952a524f4ca048f... ========== to ========== Add the Play Store app search to the launcher. Bug=736552 Review-Url: https://codereview.chromium.org/2929273002 Cr-Original-Commit-Position: refs/heads/master@{#482130} Committed: https://chromium.googlesource.com/chromium/src/+/fa1bfdf6d5fb0952a524f4ca048f... Review-Url: https://codereview.chromium.org/2929273002 Cr-Commit-Position: refs/heads/master@{#482312} Committed: https://chromium.googlesource.com/chromium/src/+/7f6b7ec8bcfcbb4cd8267e5af95f... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:320001) as https://chromium.googlesource.com/chromium/src/+/7f6b7ec8bcfcbb4cd8267e5af95f... |
