Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
412043003:
Make --install-chrome-app use extensions::WebstoreInstallWithPrompt. (Closed)
|
Created:
6 years, 5 months ago by jackhou1 Modified:
6 years, 4 months ago Reviewers:
tapted CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMake --install-chrome-app use extensions::WebstoreInstallWithPrompt.
This removes the code that resolves the webstore URL which doesn't seem
too work any more. Instead it just navigates to the webstore page and
uses WebstoreInstallWithPrompt to install the app.
BUG=341353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285876
Patch Set 1 #
Total comments: 2
Patch Set 2 : Keep install_chrome_app #
Total comments: 8
Patch Set 3 : Redo this with only changes to install_chrome_app. #
Total comments: 8
Patch Set 4 : Address comments #
Total comments: 8
Patch Set 5 : Address comments #Messages
Total messages: 13 (0 generated)
tapted, could you take a look at startup_browser_creator?
Can you highlight how it's different from the kInstallFromWebstore flag? (can we just add an extra argument along that codepath?) https://codereview.chromium.org/412043003/diff/1/chrome/browser/ui/startup/st... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/412043003/diff/1/chrome/browser/ui/startup/st... chrome/browser/ui/startup/startup_browser_creator.cc:106: const char kWebstoreUrlFormat[] = I don't think this constant belongs here..
> Can you highlight how it's different from the kInstallFromWebstore flag? (can we > just add an extra argument along that codepath?) At the moment the only difference is that --install-chrome-app is asynchronous, opens an extra tab, and continues with normal startup. However, this behavior is likely to change as we finalize the UX, so I'd like to keep it separate from the --install-from-webstore path. I guess this is a good reason to keep install_chrome_app.cc since we might need to put more stuff in there later. https://codereview.chromium.org/412043003/diff/1/chrome/browser/ui/startup/st... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/412043003/diff/1/chrome/browser/ui/startup/st... chrome/browser/ui/startup/startup_browser_creator.cc:106: const char kWebstoreUrlFormat[] = On 2014/07/24 07:52:50, tapted wrote: > I don't think this constant belongs here.. Kept this part in install_chrome_app.
https://codereview.chromium.org/412043003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/startup_helper.cc (right): https://codereview.chromium.org/412043003/diff/20001/chrome/browser/extension... chrome/browser/extensions/startup_helper.cc:283: base::RunLoop run_loop; InstallFromWebstore does this, which is blocking - the header comment for InstallChromeApp suggests that they are more consistent than this. https://codereview.chromium.org/412043003/diff/20001/chrome/browser/extension... chrome/browser/extensions/startup_helper.cc:313: if (!Extension::IdIsValid(id)) { Would be nice to consolidate some of the repeated logic here... I guess they're all just different enough though. https://codereview.chromium.org/412043003/diff/20001/chrome/browser/extension... chrome/browser/extensions/startup_helper.cc:320: helper->BeginInstall(profile, id, true /*show_prompt*/, Should the prompt that's shown be modal to a browser window? I don't think the browser exists yet - how do you prevent the browser window covering up the install prompt? (on Mac we hack the window layer, but I don't think that happens with Views) https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:511: std::vector<GURL> extra_urls_to_launch; URLs are already added in StartupBrowserCreator::LaunchBrowser() - I don't think they should be added here too. (StartupBrowserCreator::ProcessCmdLineImpl() is already crazy huge - we should try not to add more logic, but something like if (extensions::StartupHelper::HandleCommandLine(..)) return false; that does all of kInstallFromWebstore, kValidateCrx, kLimitedInstallFromWebstore, and kInstallChromeApp might be a nice cleanup. But that doesn't help with the URLs thing - see next comment) https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left): https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_impl.cc:374: ProcessLaunchURLs(process_startup, urls_to_open, desktop_type); Is there a downside to making a copy of `urls_to_open` here and passing it as a modifiable argument to install_chrome_app::InstallChromeApp(..) ? (i.e. do all the processing here, and move the logic being added to StartupBrowserCreator::ProcessCmdLineImpl() to install_chrome_app::InstallChromeApp() instead? Then move the call to ProcessLaunchURLs down after InstallChromeApp.
https://codereview.chromium.org/412043003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/startup_helper.cc (right): https://codereview.chromium.org/412043003/diff/20001/chrome/browser/extension... chrome/browser/extensions/startup_helper.cc:283: base::RunLoop run_loop; On 2014/07/25 03:49:12, tapted wrote: > InstallFromWebstore does this, which is blocking - the header comment for > InstallChromeApp suggests that they are more consistent than this. oops I didn't see the "but" in the function comment (i.e. read "Like InstallFromWebstore, this is asynchronous."). So you can ignore this.
After thinking about this again, I realised the main goal is to get it working again so we can make progress with UX. So I've reworked this to only touch install_chrome_app. It now uses WebstoreStartupInstaller directly instead of via StartupHelper. I couldn't use WebstoreInlineInstaller because it requires a requester URL, and for that URL to be whitelisted by the webstore. https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:511: std::vector<GURL> extra_urls_to_launch; On 2014/07/25 03:49:12, tapted wrote: > URLs are already added in StartupBrowserCreator::LaunchBrowser() - I don't think > they should be added here too. (StartupBrowserCreator::ProcessCmdLineImpl() is > already crazy huge - we should try not to add more logic, but something like > > if (extensions::StartupHelper::HandleCommandLine(..)) > return false; > > that does all of kInstallFromWebstore, kValidateCrx, > kLimitedInstallFromWebstore, and kInstallChromeApp might be a nice cleanup. But > that doesn't help with the URLs thing - see next comment) I open up the webstore page because WebstoreStartupInstaller will cancel installation if it's a paid app. In that case, we want to present the webstore page which asks the user to sign in. We might change this, e.g. to only show the webstore if the user did not have Chrome installed (this is a first run). https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left): https://codereview.chromium.org/412043003/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_impl.cc:374: ProcessLaunchURLs(process_startup, urls_to_open, desktop_type); On 2014/07/25 03:49:12, tapted wrote: > Is there a downside to making a copy of `urls_to_open` here and passing it as a > modifiable argument to install_chrome_app::InstallChromeApp(..) ? (i.e. do all > the processing here, and move the logic being added to > StartupBrowserCreator::ProcessCmdLineImpl() to > install_chrome_app::InstallChromeApp() instead? Then move the call to > ProcessLaunchURLs down after InstallChromeApp. If there are multiple last_opened_profiles, this will happen for each one.
https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... File chrome/browser/apps/install_chrome_app.cc (right): https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:44: BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_NATIVE)->get(0); I don't think BrowserList::get() can ever return NULL, but you might need to check BrowserList->empty() before calling get()? What happens if chrome is running, but has no browser windows open (e.g. background mode / hangouts), and you execute this via the process singleton? (should we allocate a new browser for that case?) https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:55: extensions::ExtensionRegistry* registry = nit: maybe `using extensions::ExtensionRegistry`? https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:60: extensions::ExtensionRegistry::BLACKLISTED); This probably needs a comment to explain this flag choice. And, e.g. to note why the UX works if the extension is disabled or terminated. If the UX works that is :) - I'm not sure. https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:62: return; Maybe a TODO(..) determine UX for an installed extension?
https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... File chrome/browser/apps/install_chrome_app.cc (right): https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:44: BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_NATIVE)->get(0); On 2014/07/28 03:41:04, tapted wrote: > I don't think BrowserList::get() can ever return NULL, but you might need to > check BrowserList->empty() before calling get()? > > What happens if chrome is running, but has no browser windows open (e.g. > background mode / hangouts), and you execute this via the process singleton? > (should we allocate a new browser for that case?) At the moment it's not an issue, but that might change. Added a comment. https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:55: extensions::ExtensionRegistry* registry = On 2014/07/28 03:41:04, tapted wrote: > nit: maybe `using extensions::ExtensionRegistry`? Done. https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:60: extensions::ExtensionRegistry::BLACKLISTED); On 2014/07/28 03:41:04, tapted wrote: > This probably needs a comment to explain this flag choice. And, e.g. to note why > the UX works if the extension is disabled or terminated. If the UX works that is > :) - I'm not sure. Done. https://codereview.chromium.org/412043003/diff/40001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:62: return; On 2014/07/28 03:41:04, tapted wrote: > Maybe a TODO(..) determine UX for an installed extension? Done.
lgtm % nits and after updating the CL description https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... File chrome/browser/apps/install_chrome_app.cc (right): https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:15: #include "chrome/common/extensions/webstore_install_result.h" meta-nit: with my nit below, you probably won't need this https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:50: if (!browser) nit: DCHECK(browser) - (if there's a null pointer in that array, something is wrong) https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:72: new extensions::WebstoreStartupInstaller( Not sure if you want this, but I think you could make the dialog properly modal on the browser window by using WebstoreInstallWithPrompt, or adding a new constructor to WebstoreStartupInstaller i.e. .. installer = new ..WebstoreInstallWithPrompt(app_id, browser->profile(), browser->window()->GetNativeWindow(), ..); installer->set_install_source(WebstoreInstaller::INSTALL_SOURCE_INLINE); installer->BeginInstall(); Maybe something to ask UX. https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:73: app_id, browser->profile(), true, base::Bind(&DoNothingCallback)); nit: probably nicer to do `base::Bind(&DoNothingCallback)` -> `WebstoreStandaloneInstaller::Callback()` i.e. just pass a "null" callback -- CompleteInstall(..) checks is_null()
The CQ bit was checked by jackhou@chromium.org
https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... File chrome/browser/apps/install_chrome_app.cc (right): https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:15: #include "chrome/common/extensions/webstore_install_result.h" On 2014/07/28 04:35:43, tapted wrote: > meta-nit: with my nit below, you probably won't need this Done. https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:50: if (!browser) On 2014/07/28 04:35:43, tapted wrote: > nit: DCHECK(browser) - (if there's a null pointer in that array, something is > wrong) Done. https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:72: new extensions::WebstoreStartupInstaller( On 2014/07/28 04:35:43, tapted wrote: > Not sure if you want this, but I think you could make the dialog properly modal > on the browser window by using WebstoreInstallWithPrompt, or adding a new > constructor to WebstoreStartupInstaller > > i.e. > > .. installer = new ..WebstoreInstallWithPrompt(app_id, browser->profile(), > browser->window()->GetNativeWindow(), ..); > installer->set_install_source(WebstoreInstaller::INSTALL_SOURCE_INLINE); > installer->BeginInstall(); > > > Maybe something to ask UX. Done. https://codereview.chromium.org/412043003/diff/60001/chrome/browser/apps/inst... chrome/browser/apps/install_chrome_app.cc:73: app_id, browser->profile(), true, base::Bind(&DoNothingCallback)); On 2014/07/28 04:35:43, tapted wrote: > nit: probably nicer to do > > `base::Bind(&DoNothingCallback)` -> `WebstoreStandaloneInstaller::Callback()` > > i.e. just pass a "null" callback -- CompleteInstall(..) checks is_null() Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/412043003/80001
Message was sent while issue was closed.
Change committed as 285876 |
