Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1936953002:
Don't return installed apps in webstore searches in Launcher (Closed)
|
Created:
4 years, 7 months ago by Greg Levin Modified:
4 years, 7 months ago CC:
chromium-reviews, tfarina, Matt Giuca, tapted, xiyuan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't return installed apps in webstore searches in Launcher
BUG=592168
TEST=Open Launcher, search for and install Sudoku app, launch it.
Reopen Launcher, observer that app icon on Start Page looks right.
In the Launcher search, webstore app results used the same internal id
as installed apps. Sometimes the wrong time of result object would get
cached in the Mixer, and a webstore result (smaller icon, no context
menu) would end up on the Start Page.
This CL changes internal id scheme for webstore app results, and
explicitly removes installed apps from results returned by webstore
provider.
Committed: https://crrev.com/bcf7574721f7393dd33469fe476999ab50a35075
Cr-Commit-Position: refs/heads/master@{#391791}
Patch Set 1 #
Total comments: 13
Patch Set 2 : First reviewer revisions #
Total comments: 6
Patch Set 3 : Address final reviews #
Messages
Total messages: 24 (10 generated)
glevin@chromium.org changed reviewers: + calamity@chromium.org
Please have a look! Choosing calamity@ as reviewer (somewhat arbitrarily), but +mgiuca@ and xiyuan@ for reference. Please jump in as official reviewers if you have input! https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:41: const char kWebstoreUrlPlaceholder[] = "[webstore_url]"; Placeholder for URL, since webstore search result URLs aren't constant across test runs. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:388: RunQueryAndVerify("fun", kOneResult, kParsedOneStoreSearchResult, 1); If there are no matching apps, the provider returns a webstore search result as a placeholder: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... That's why we expect one result, with a non-constant URL, as noted above. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_result.cc:27: #include "extensions/common/extension.h" TODO: Remove this include, no longer used. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_result.cc:85: extension_id + "/"; The internal id for WebstoreResults now looks like webstore-result://lneaknkopdijkpnocmklfnjbeapigfbh/ instead of chrome-extension://lneaknkopdijkpnocmklfnjbeapigfbh/
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_provider.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_provider.cc:174: if (controller_->IsExtensionInstalled(profile_, app_id)) Comment here please (explaining that we do not show a webstore result if the corresponding app is already installed). https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:41: const char kWebstoreUrlPlaceholder[] = "[webstore_url]"; On 2016/05/02 00:09:13, Greg Levin wrote: > Placeholder for URL, since webstore search result URLs aren't constant across > test runs. They aren't? Why not... they should just be a function of the app_id which is constant. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_result.cc:27: #include "extensions/common/extension.h" On 2016/05/02 00:09:13, Greg Levin wrote: > TODO: Remove this include, no longer used. Are you going to do that this CL? https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_result.cc:85: extension_id + "/"; On 2016/05/02 00:09:13, Greg Levin wrote: > The internal id for WebstoreResults now looks like > webstore-result://lneaknkopdijkpnocmklfnjbeapigfbh/ > instead of > chrome-extension://lneaknkopdijkpnocmklfnjbeapigfbh/ Any reason this can't be "https://chrome.google.com/webstore/detail/xxx/extension_id"? The only issue I can think of is that you also need the app title in xxx, but you technically don't (the webstore doesn't care what you put in that segment). So you could just replace "xxx" with "app" (a literal string). That would be better than inventing a fake URL scheme.
https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_provider.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_provider.cc:174: if (controller_->IsExtensionInstalled(profile_, app_id)) On 2016/05/02 00:34:52, Matt Giuca wrote: > Comment here please (explaining that we do not show a webstore result if the > corresponding app is already installed). Done. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:41: const char kWebstoreUrlPlaceholder[] = "[webstore_url]"; On 2016/05/02 00:34:52, Matt Giuca wrote: > On 2016/05/02 00:09:13, Greg Levin wrote: > > Placeholder for URL, since webstore search result URLs aren't constant across > > test runs. > > They aren't? Why not... they should just be a function of the app_id which is > constant. This isn't the URL for an app result; it's the URL for a search of the webstore (i.e. a SearchWebstoreResult, not a WebstoreResult). In the test, the URL is http://127.0.0.1:36329/search/fun The "36329" varies from run to run, so this URL can't be hardcoded. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_result.cc:27: #include "extensions/common/extension.h" On 2016/05/02 00:34:52, Matt Giuca wrote: > On 2016/05/02 00:09:13, Greg Levin wrote: > > TODO: Remove this include, no longer used. > > Are you going to do that this CL? Yeah, it's in (out of) the next patch. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_result.cc:85: extension_id + "/"; On 2016/05/02 00:34:52, Matt Giuca wrote: > On 2016/05/02 00:09:13, Greg Levin wrote: > > The internal id for WebstoreResults now looks like > > webstore-result://lneaknkopdijkpnocmklfnjbeapigfbh/ > > instead of > > chrome-extension://lneaknkopdijkpnocmklfnjbeapigfbh/ > > Any reason this can't be > "https://chrome.google.com/webstore/detail/xxx/extension_id"? > > The only issue I can think of is that you also need the app title in xxx, but > you technically don't (the webstore doesn't care what you put in that segment). > So you could just replace "xxx" with "app" (a literal string). That would be > better than inventing a fake URL scheme. Done... seems to work fine. I just went with https://chrome.google.com/webstore/detail/extension_id since the webstore doesn't actually require that segment at all.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1936953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1936953002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xiyuan@chromium.org changed reviewers: + xiyuan@chromium.org
lgtm https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc (right): https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:155: std::set<std::string> installed_ids_; nit: #include <set>
lgtm https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/webstore/webstore_provider.cc (right): https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/webstore/webstore_provider.cc:174: // If an app is already installed, dont' show it in results. nit: don't https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc (right): https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:386: IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoInstalledApps) { nit: IgnoreInstalledApps
lgtm with xiyuan and calamity's nits addressed. https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right): https://codereview.chromium.org/1936953002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/search/webstore/webstore_result.cc:85: extension_id + "/"; On 2016/05/02 01:38:49, Greg Levin wrote: > On 2016/05/02 00:34:52, Matt Giuca wrote: > > On 2016/05/02 00:09:13, Greg Levin wrote: > > > The internal id for WebstoreResults now looks like > > > webstore-result://lneaknkopdijkpnocmklfnjbeapigfbh/ > > > instead of > > > chrome-extension://lneaknkopdijkpnocmklfnjbeapigfbh/ > > > > Any reason this can't be > > "https://chrome.google.com/webstore/detail/xxx/extension_id"? > > > > The only issue I can think of is that you also need the app title in xxx, but > > you technically don't (the webstore doesn't care what you put in that > segment). > > So you could just replace "xxx" with "app" (a literal string). That would be > > better than inventing a fake URL scheme. > > Done... seems to work fine. I just went with > https://chrome.google.com/webstore/detail/extension_id > since the webstore doesn't actually require that segment at all. Nice.
The CQ bit was checked by glevin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, calamity@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/1936953002/#ps40001 (title: "Address final reviews")
https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/webstore/webstore_provider.cc (right): https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/webstore/webstore_provider.cc:174: // If an app is already installed, dont' show it in results. On 2016/05/02 18:16:54, calamity wrote: > nit: don't Done. https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc (right): https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:155: std::set<std::string> installed_ids_; On 2016/05/02 16:16:58, xiyuan wrote: > nit: #include <set> Done. https://codereview.chromium.org/1936953002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc:386: IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoInstalledApps) { On 2016/05/02 18:16:54, calamity wrote: > nit: IgnoreInstalledApps Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1936953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1936953002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by glevin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1936953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1936953002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't return installed apps in webstore searches in Launcher BUG=592168 TEST=Open Launcher, search for and install Sudoku app, launch it. Reopen Launcher, observer that app icon on Start Page looks right. In the Launcher search, webstore app results used the same internal id as installed apps. Sometimes the wrong time of result object would get cached in the Mixer, and a webstore result (smaller icon, no context menu) would end up on the Start Page. This CL changes internal id scheme for webstore app results, and explicitly removes installed apps from results returned by webstore provider. ========== to ========== Don't return installed apps in webstore searches in Launcher BUG=592168 TEST=Open Launcher, search for and install Sudoku app, launch it. Reopen Launcher, observer that app icon on Start Page looks right. In the Launcher search, webstore app results used the same internal id as installed apps. Sometimes the wrong time of result object would get cached in the Mixer, and a webstore result (smaller icon, no context menu) would end up on the Start Page. This CL changes internal id scheme for webstore app results, and explicitly removes installed apps from results returned by webstore provider. Committed: https://crrev.com/bcf7574721f7393dd33469fe476999ab50a35075 Cr-Commit-Position: refs/heads/master@{#391791} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bcf7574721f7393dd33469fe476999ab50a35075 Cr-Commit-Position: refs/heads/master@{#391791} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
