Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
790043002:
Hosted apps on OS X now act more like a native app. (Closed)
|
Created:
6 years ago by mitchellj Modified:
6 years ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@creating-app-shims-2 Project:
chromium Visibility:
Public. |
DescriptionHosted apps on OS X now act more like a native app.
- Quitting an app shim will close all associated windows.
- Closing all windows will quit the app shim.
- Can focus between Chrome and app shim windows.
- Can hide/show all windows associated with an app shim.
BUG=440651
Committed: https://crrev.com/e59a161377578b284095a89c410a91879b9b1bde
Cr-Commit-Position: refs/heads/master@{#308473}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Refactored code and addressed comments #
Total comments: 30
Patch Set 3 : Rebase #Patch Set 4 : Moved functions, cleaned up code and created CloseBrowsersForApp function #
Total comments: 10
Patch Set 5 : Linting and formatting #
Total comments: 4
Patch Set 6 : Added comments and refactored app_controller_mac #
Messages
Total messages: 17 (3 generated)
mitchelljones@chromium.org changed reviewers: + jackhou@chromium.org
Looks like the CL description lost its line breaks. https://codereview.chromium.org/790043002/diff/1/chrome/browser/app_controlle... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/1/chrome/browser/app_controlle... chrome/browser/app_controller_mac.mm:1198: if (extension->is_hosted_app()) { The goal here is to identify windows that are not logically associated with the Chrome dock icon. I think a better way would be to check ExtensionAppShimHandler::FindHost. Having a host implies the window has its own shim and so should not be included here. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc (right): https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:74: void SetHostedAppHidden(Profile* profile, Maybe make this a method of ExtensionAppShimHandler, so that it can just use the AppBrowserMap. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:80: for (BrowserList::const_iterator it = browser_list->begin(); You can use C++11's range-based for-loops now: for (const Browser* browser : *browser_list) { This probably applies to a few other places in this CL too. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:92: if (hidden) Do you need this loop? It doesn't look like this block does anything with the index or tab strip. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:116: bool FocusHostedAppWindows(std::vector<content::WebContents*>& windows) { Just pass in the BrowserSet here. You can get the native window via BrowserWindow::GetNativeWindow(). https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:472: // If it's a hosted app and a tabbed application, kill it immediately. s/kill it immediately/let the shim terminate immediately/ https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:479: host->OnAppLaunchComplete(APP_SHIM_LAUNCH_SUCCESS); Instead of sending APP_SHIM_LAUNCH_SUCCESS here, have OnBrowserAdded call OnAppActivated (see comment there). https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:602: TabStripModel* tab_strip = browser->tab_strip_model(); I think you can just call BrowserWindow::Close(). https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:617: // Otherwise, once the last window closes for a hosted app, OnBrowserRevmoed s/Revmoed/Removed/ https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:702: browsers.insert(browser); You should make this call OnAppActivated if it is the first browser being added for an app. OnAppActivated will ensure that the shim gets launched when you start the app from chrome:apps. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:706: if (!browser->is_app()) I think we should be consistent about checking both Browser:is_app and Extension::is_hosted_app, in case they are not equivalent. https://codereview.chromium.org/790043002/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:305: // Try and look if there is a relevant app window. Otherwise, look at the Maybe something like: // If there is no corresponding AppWindow, this could be a hosted app, so check for a Browser. https://codereview.chromium.org/790043002/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:422: if (browser && browser->is_app()) { This logic of "browser->is_app() && extension->is_hosted_app()" gets repeated a fair big, maybe we could factor it out. Maybe make it a static function in ExtensionAppShimHandler, since this logic is used there as well.
https://codereview.chromium.org/790043002/diff/1/chrome/browser/app_controlle... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/1/chrome/browser/app_controlle... chrome/browser/app_controller_mac.mm:1198: if (extension->is_hosted_app()) { On 2014/12/11 02:43:18, jackhou1 wrote: > The goal here is to identify windows that are not logically associated with the > Chrome dock icon. I think a better way would be to check > ExtensionAppShimHandler::FindHost. Having a host implies the window has its own > shim and so should not be included here. Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc (right): https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:74: void SetHostedAppHidden(Profile* profile, On 2014/12/11 02:43:18, jackhou1 wrote: > Maybe make this a method of ExtensionAppShimHandler, so that it can just use the > AppBrowserMap. Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:80: for (BrowserList::const_iterator it = browser_list->begin(); On 2014/12/11 02:43:18, jackhou1 wrote: > You can use C++11's range-based for-loops now: > > for (const Browser* browser : *browser_list) { > > This probably applies to a few other places in this CL too. Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:92: if (hidden) On 2014/12/11 02:43:18, jackhou1 wrote: > Do you need this loop? It doesn't look like this block does anything with the > index or tab strip. Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:116: bool FocusHostedAppWindows(std::vector<content::WebContents*>& windows) { On 2014/12/11 02:43:18, jackhou1 wrote: > Just pass in the BrowserSet here. You can get the native window via > BrowserWindow::GetNativeWindow(). Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:472: // If it's a hosted app and a tabbed application, kill it immediately. On 2014/12/11 02:43:18, jackhou1 wrote: > s/kill it immediately/let the shim terminate immediately/ Acknowledged. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:479: host->OnAppLaunchComplete(APP_SHIM_LAUNCH_SUCCESS); On 2014/12/11 02:43:18, jackhou1 wrote: > Instead of sending APP_SHIM_LAUNCH_SUCCESS here, have OnBrowserAdded call > OnAppActivated (see comment there). Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:602: TabStripModel* tab_strip = browser->tab_strip_model(); On 2014/12/11 02:43:18, jackhou1 wrote: > I think you can just call BrowserWindow::Close(). Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:617: // Otherwise, once the last window closes for a hosted app, OnBrowserRevmoed On 2014/12/11 02:43:18, jackhou1 wrote: > s/Revmoed/Removed/ Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:702: browsers.insert(browser); On 2014/12/11 02:43:18, jackhou1 wrote: > You should make this call OnAppActivated if it is the first browser being added > for an app. OnAppActivated will ensure that the shim gets launched when you > start the app from chrome:apps. Done. I ran into a problem where calling OnAppActivated immediately tries to focus the browser windows. However, browser sends the OnBrowserAdded message when BrowserWindow may still be NULL. So I had to move the code in OnBrowserAdded to the ExtensionAppShimHandler::Observer function to listen for when the BrowserWindow is created instead. https://codereview.chromium.org/790043002/diff/1/chrome/browser/apps/app_shim... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:706: if (!browser->is_app()) On 2014/12/11 02:43:18, jackhou1 wrote: > I think we should be consistent about checking both Browser:is_app and > Extension::is_hosted_app, in case they are not equivalent. Done. https://codereview.chromium.org/790043002/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:305: // Try and look if there is a relevant app window. Otherwise, look at the On 2014/12/11 02:43:18, jackhou1 wrote: > Maybe something like: > > // If there is no corresponding AppWindow, this could be a hosted app, so check > for a Browser. Acknowledged. https://codereview.chromium.org/790043002/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:422: if (browser && browser->is_app()) { On 2014/12/11 02:43:18, jackhou1 wrote: > This logic of "browser->is_app() && extension->is_hosted_app()" gets repeated a > fair big, maybe we could factor it out. Maybe make it a static function in > ExtensionAppShimHandler, since this logic is used there as well. Done.
https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc (right): https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:22: #include "chrome/browser/ui/tabs/tab_strip_model.h" Do you still need this #include? https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:95: for (const Browser* browser : browsers) { No braces on one-line for-loop. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:277: void ExtensionAppShimHandler::QuitHostedAppForWindow( I don't think you need to check is_hosted_app here, because if there's a host, then we'll want to call OnShimQuit. So you can have this function take a Profile* and app_id instead. Also, we need to handle the case when there is no host. Chrome should just close all the hosted app's windows. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:316: if (!browser->is_app() || Can this ever happen? All the Browser*s in the map should be associated with this app. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:478: // If it's a hosted app and a tabbed application, let the shim terminate s/and a tabbed application/that opens in a tab/ https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:541: std::vector<content::WebContents*> items; Is |items| ever used? https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:544: return; Blank line after return. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:575: handler->SetHostedAppHidden(profile, app_id, hidden); You can call SetHostedAppHidden directly here since OnShimSetHidden is a non-static method. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:592: for (const Browser* browser : it->second) { No braces here. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:646: if (!browser->is_app()) This should also check that the Extension is a hosted app. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:702: // chrome::NOTIFICATION_BROWSER_WINDOW_READY and then call OnAppActivited. OnAppActivated https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h (right): https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h:88: void SetHostedAppHidden(Profile* profile, Move this below FindHost. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h:94: const static extensions::Extension* GetAppExtension( s/const static/static const/ Also move this so that it's the first static (and move the definition as well). https://codereview.chromium.org/790043002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:310: apps::ExtensionAppShimHandler::GetAppExtension( You can assign this directly to |extension|. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:398: if (!browser || !browser->is_app()) What do you think of adding something like const Extension* ExtensionAppShimHandler::GetAppForBrowser(Browser*); It can check Browser::is_app() and then call GetAppExtension()?
https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc (right): https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:22: #include "chrome/browser/ui/tabs/tab_strip_model.h" On 2014/12/12 00:48:29, jackhou1 wrote: > Do you still need this #include? Acknowledged. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:95: for (const Browser* browser : browsers) { On 2014/12/12 00:48:28, jackhou1 wrote: > No braces on one-line for-loop. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:277: void ExtensionAppShimHandler::QuitHostedAppForWindow( On 2014/12/12 00:48:28, jackhou1 wrote: > I don't think you need to check is_hosted_app here, because if there's a host, > then we'll want to call OnShimQuit. So you can have this function take a > Profile* and app_id instead. > > Also, we need to handle the case when there is no host. Chrome should just close > all the hosted app's windows. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:316: if (!browser->is_app() || On 2014/12/12 00:48:29, jackhou1 wrote: > Can this ever happen? All the Browser*s in the map should be associated with > this app. This is correct. I've removed the first condition in the IF statement. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:478: // If it's a hosted app and a tabbed application, let the shim terminate On 2014/12/12 00:48:29, jackhou1 wrote: > s/and a tabbed application/that opens in a tab/ Acknowledged. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:541: std::vector<content::WebContents*> items; On 2014/12/12 00:48:28, jackhou1 wrote: > Is |items| ever used? Oops, forgot to remove it when I changed the signature of FocusHostedAppWindows and had to refactor the code. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:544: return; On 2014/12/12 00:48:28, jackhou1 wrote: > Blank line after return. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:575: handler->SetHostedAppHidden(profile, app_id, hidden); On 2014/12/12 00:48:28, jackhou1 wrote: > You can call SetHostedAppHidden directly here since OnShimSetHidden is a > non-static method. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:592: for (const Browser* browser : it->second) { On 2014/12/12 00:48:29, jackhou1 wrote: > No braces here. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:646: if (!browser->is_app()) On 2014/12/12 00:48:29, jackhou1 wrote: > This should also check that the Extension is a hosted app. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:702: // chrome::NOTIFICATION_BROWSER_WINDOW_READY and then call OnAppActivited. On 2014/12/12 00:48:28, jackhou1 wrote: > OnAppActivated Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h (right): https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h:88: void SetHostedAppHidden(Profile* profile, On 2014/12/12 00:48:29, jackhou1 wrote: > Move this below FindHost. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h:94: const static extensions::Extension* GetAppExtension( On 2014/12/12 00:48:29, jackhou1 wrote: > s/const static/static const/ > > Also move this so that it's the first static (and move the definition as well). Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:310: apps::ExtensionAppShimHandler::GetAppExtension( On 2014/12/12 00:48:29, jackhou1 wrote: > You can assign this directly to |extension|. Done. https://codereview.chromium.org/790043002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:398: if (!browser || !browser->is_app()) On 2014/12/12 00:48:29, jackhou1 wrote: > What do you think of adding something like > > const Extension* ExtensionAppShimHandler::GetAppForBrowser(Browser*); > > It can check Browser::is_app() and then call GetAppExtension()? Done.
lgtm with a few nits https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc (right): https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:297: if (!browser || !browser->is_app()) Blank line after return. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:653: std::string app_id = No need for this, you can use extension->id() below. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:717: std::string app_id = Same here. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h (right): https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h:83: const static extensions::Extension* GetAppExtension( static const and below as well. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:17: #include "chrome/browser/web_applications/web_app_mac.h" No need for web_app_mac.h any more.
https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc (right): https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:297: if (!browser || !browser->is_app()) On 2014/12/12 04:42:49, jackhou1 wrote: > Blank line after return. Done. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:653: std::string app_id = On 2014/12/12 04:42:49, jackhou1 wrote: > No need for this, you can use extension->id() below. Done. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:717: std::string app_id = On 2014/12/12 04:42:49, jackhou1 wrote: > Same here. Done. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... File chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h (right): https://codereview.chromium.org/790043002/diff/60001/chrome/browser/apps/app_... chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h:83: const static extensions::Extension* GetAppExtension( On 2014/12/12 04:42:49, jackhou1 wrote: > static const > > and below as well. Done. https://codereview.chromium.org/790043002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:17: #include "chrome/browser/web_applications/web_app_mac.h" On 2014/12/12 04:42:49, jackhou1 wrote: > No need for web_app_mac.h any more. Done.
mitchelljones@chromium.org changed reviewers: + rsesek@chromium.org
Hi Robert, Could you please review the files browser_window_cocoa.mm, app_shim_menu_controller_mac.mm and app_controller_mac.mm? Thanks.
https://codereview.chromium.org/790043002/diff/80001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/80001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:1194: if (browser && browser->is_app()) { Document what this is doing. https://codereview.chromium.org/790043002/diff/80001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:1195: ExtensionAppShimHandler* handler = g_browser_process->platform_part() You could pull this local out of the loop (and name it |appShimHandler|).
https://codereview.chromium.org/790043002/diff/80001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/790043002/diff/80001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:1194: if (browser && browser->is_app()) { On 2014/12/15 21:31:00, Robert Sesek wrote: > Document what this is doing. Done. https://codereview.chromium.org/790043002/diff/80001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:1195: ExtensionAppShimHandler* handler = g_browser_process->platform_part() On 2014/12/15 21:31:00, Robert Sesek wrote: > You could pull this local out of the loop (and name it |appShimHandler|). Done.
LGTM
The CQ bit was checked by mitchelljones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/790043002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e59a161377578b284095a89c410a91879b9b1bde Cr-Commit-Position: refs/heads/master@{#308473} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
