Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18089012:
Swap main menu with app-specific menu when app window focused. (Closed)
|
Created:
7 years, 5 months ago by jackhou1 Modified:
7 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, jeremya+watch_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSwap main menu with app-specific menu when app window focused.
When an app window is focused, a new menu item is created for that app
and appended to the main menu. All of Chrome's menu items are hidden.
When a Chrome window is focused, the app's menu item is removed and
Chrome's menu items are unhidden.
BUG=168080
TEST=Start an app.
Focus the app window, the only item in the main menu bar should be that app.
Focus a browser window, the main menu bar returns to normal.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213031
Patch Set 1 #
Total comments: 16
Patch Set 2 : Sync and rebase #Patch Set 3 : Address comments. Use NSWindowDidBecomeMainNotification. #
Total comments: 19
Patch Set 4 : Address comments #
Total comments: 10
Patch Set 5 : Sync and rebase #Patch Set 6 : Address comments. #Patch Set 7 : Add browsertest. #
Total comments: 7
Patch Set 8 : Address comments #
Total comments: 6
Patch Set 9 : Address comments #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.h:126: - (void)setMenuBarToApp:(NSString*)app_id argument should be appId https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1323: NSMenuItem* appMenuItem = [[NSMenuItem alloc] there's a memory leak here ;) - you need scoped_nsobject<NSMenuItem> appMenuItem( [[NSMenuItem alloc] initWithTitle: /* etc */]); https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1325: action:@selector(itemSelected:) I think the action can be `nil`, since we aren't setting a target. https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1327: NSMenu* appMenu = [[NSMenu alloc] initWithTitle:title]; scoped_nsobject<NSMenu> https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1331: [itemForApp setHidden:NO]; maybe in an else {? you're not re-assigning, so itemForApp will always be null if !else https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1335: NSMenu* mainMenu = [NSApp mainMenu]; nit: probably don't need this temporary https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1338: [item setHidden:extensions::Extension::IdIsValid(title)]; hm.. this feels a bit odd.. Swapping out [NSApp mainMenu] probably won't work -- background things in Chrome seem to depend on it. But maybe something more certain like simply storing a scoped_nsobject<NSMenuItem> appMenuItem_ data member. And I don't think we should keep them around -- just remove, and recreate/re-add each time the app is activated. Then this function is if (!appMenuItem_) return; [[NSApp mainMenu] removeItem:appMenuItem_]; appMenuItem_.reset(); for (..) setHidden:NO WDYT? You probably won't even need to pass in app_id then. https://codereview.chromium.org/18089012/diff/1/chrome/browser/ui/cocoa/exten... File chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm (right): https://codereview.chromium.org/18089012/diff/1/chrome/browser/ui/cocoa/exten... chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm:744: void NativeAppWindowCocoa::WindowDidBecomeKey() { does this happen for panel windows as well?
https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.h:126: - (void)setMenuBarToApp:(NSString*)app_id On 2013/06/28 04:09:43, tapted wrote: > argument should be appId Done. https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1323: NSMenuItem* appMenuItem = [[NSMenuItem alloc] On 2013/06/28 04:09:43, tapted wrote: > there's a memory leak here ;) - you need > > scoped_nsobject<NSMenuItem> appMenuItem( > [[NSMenuItem alloc] initWithTitle: > /* etc */]); Done. https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1325: action:@selector(itemSelected:) On 2013/06/28 04:09:43, tapted wrote: > I think the action can be `nil`, since we aren't setting a target. Done. https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1327: NSMenu* appMenu = [[NSMenu alloc] initWithTitle:title]; On 2013/06/28 04:09:43, tapted wrote: > scoped_nsobject<NSMenu> Done. https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1331: [itemForApp setHidden:NO]; On 2013/06/28 04:09:43, tapted wrote: > maybe in an else {? you're not re-assigning, so itemForApp will always be null > if !else Done. https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1335: NSMenu* mainMenu = [NSApp mainMenu]; On 2013/06/28 04:09:43, tapted wrote: > nit: probably don't need this temporary Done. https://codereview.chromium.org/18089012/diff/1/chrome/browser/app_controller... chrome/browser/app_controller_mac.mm:1338: [item setHidden:extensions::Extension::IdIsValid(title)]; On 2013/06/28 04:09:43, tapted wrote: > hm.. this feels a bit odd.. Swapping out [NSApp mainMenu] probably won't work -- > background things in Chrome seem to depend on it. But maybe something more > certain like simply storing a scoped_nsobject<NSMenuItem> appMenuItem_ data > member. > > And I don't think we should keep them around -- just remove, and recreate/re-add > each time the app is activated. > > Then this function is > > if (!appMenuItem_) > return; > > [[NSApp mainMenu] removeItem:appMenuItem_]; > appMenuItem_.reset(); > for (..) > setHidden:NO > > WDYT? You probably won't even need to pass in app_id then. Done. https://codereview.chromium.org/18089012/diff/1/chrome/browser/ui/cocoa/exten... File chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm (right): https://codereview.chromium.org/18089012/diff/1/chrome/browser/ui/cocoa/exten... chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm:744: void NativeAppWindowCocoa::WindowDidBecomeKey() { On 2013/06/28 04:09:43, tapted wrote: > does this happen for panel windows as well? Yeah it does. Changed to using notifications so we don't need to call this for every window implementation.
make sure you update the CL description too ;) https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.h:85: base::scoped_nsobject<NSMenuItem> appMenuItem_; Probably needs a comment here to tie this in. E.g. ~"The main menu item showing the name of a currently focussed packaged app." https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:290: name:NSWindowDidBecomeMainNotification hm... I think this observer might be getting clobbered? We might need a more generic function, which calls both updateMenuBar and windowLayeringDidChange https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:326: [[NSNotificationCenter defaultCenter] removeObserver:self]; (this will probably do the removeObserver bit for us) https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1127: addObserver:self Is there an appropriate place to removeObserver as well? https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1129: name:NSWindowDidBecomeMainNotification object:nil]; nit: object:nil on a new line https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1398: // Restore the Chrome menu bar. nit: menu -> main menu? And move down to before the for loop. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1412: for (NSMenuItem* item in [mainMenu itemArray]) This looks like it will hide appMenuItem_ when an app window is refocussed? Needs to come later - maybe right before [mainMenu addItem] https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1415: NSString* title = base::SysUTF8ToNSString(shell_window->extension()->name()); note: menu_controller.mm does some sanitization of things it puts into menus.. I don't think you need it, but I haven't thought about it too much https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1418: if ([[appMenuItem_ title] isEqualToString:title]) since we've got the whole extension, maybe it's better to store the extension_id at this level, and compare that. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1422: appMenuItem_.reset(); nit: this .reset is redundant
https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.h:85: base::scoped_nsobject<NSMenuItem> appMenuItem_; On 2013/07/04 02:43:33, tapted wrote: > Probably needs a comment here to tie this in. E.g. ~"The main menu item showing > the name of a currently focussed packaged app." Done. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:290: name:NSWindowDidBecomeMainNotification On 2013/07/04 02:43:33, tapted wrote: > hm... I think this observer might be getting clobbered? We might need a more > generic function, which calls both updateMenuBar and windowLayeringDidChange Nup, both get called. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1129: name:NSWindowDidBecomeMainNotification object:nil]; On 2013/07/04 02:43:33, tapted wrote: > nit: object:nil on a new line Done. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1398: // Restore the Chrome menu bar. On 2013/07/04 02:43:33, tapted wrote: > nit: menu -> main menu? And move down to before the for loop. Done. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1412: for (NSMenuItem* item in [mainMenu itemArray]) On 2013/07/04 02:43:33, tapted wrote: > This looks like it will hide appMenuItem_ when an app window is refocussed? > Needs to come later - maybe right before [mainMenu addItem] Done. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1415: NSString* title = base::SysUTF8ToNSString(shell_window->extension()->name()); On 2013/07/04 02:43:33, tapted wrote: > note: menu_controller.mm does some sanitization of things it puts into menus.. I > don't think you need it, but I haven't thought about it too much I think it's mostly for non-text stuff like check boxes etc. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1418: if ([[appMenuItem_ title] isEqualToString:title]) On 2013/07/04 02:43:33, tapted wrote: > since we've got the whole extension, maybe it's better to store the extension_id > at this level, and compare that. Done. https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1422: appMenuItem_.reset(); On 2013/07/04 02:43:33, tapted wrote: > nit: this .reset is redundant Done.
Actually... what do think about doing all of this in extension_app_shim_handler_mac.mm, with a new class, held as a scoped_nsobject in ExtensionAppShimHandler that can register itself with the notification centre? Maybe we should still check with owners, but I don't think we actually need to access anything from app_controller_mac any more https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/10001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:290: name:NSWindowDidBecomeMainNotification On 2013/07/05 03:52:37, jackhou1 wrote: > On 2013/07/04 02:43:33, tapted wrote: > > hm... I think this observer might be getting clobbered? We might need a more > > generic function, which calls both updateMenuBar and windowLayeringDidChange > > Nup, both get called. Ah, interesting. And from the documentation it does look like NSNotificaitonCentre supports this reliably, so I'm content.. might be other opinions though. https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:198: - (void)updateMenuBar:(NSNotification*)notification; maybe showOrHideMenuItemsForPackagedApp ? https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:313: Should we add the notification up here, to keep it with the other notifications. https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1394: apps::ShellWindow* shell_window = should be shellWindow https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1399: // Restore the Chrome main menu bar. move this comment down to before the for loop? https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1413: NSString* app_id = base::SysUTF8ToNSString(app->id()); should be appId
https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:198: - (void)updateMenuBar:(NSNotification*)notification; On 2013/07/05 04:50:54, tapted wrote: > maybe showOrHideMenuItemsForPackagedApp ? Done. https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:313: On 2013/07/05 04:50:54, tapted wrote: > Should we add the notification up here, to keep it with the other notifications. Done. https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1394: apps::ShellWindow* shell_window = On 2013/07/05 04:50:54, tapted wrote: > should be shellWindow Done. https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1399: // Restore the Chrome main menu bar. On 2013/07/05 04:50:54, tapted wrote: > move this comment down to before the for loop? Done. https://codereview.chromium.org/18089012/diff/17001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1413: NSString* app_id = base::SysUTF8ToNSString(app->id()); On 2013/07/05 04:50:54, tapted wrote: > should be appId Done.
I'm not sure about putting this in ExtensionAppShimHandler. I think AppController does most of the menu bar stuff, so it might be better to keep related code in the same place. Let's see what owners think.
lgtm
thakis, what do you think of putting this code here? It doesn't depend on anything in app_controller_mac, but app_controllor_mac handles other menu bar related things.
On 2013/07/05 12:49:15, jackhou1 wrote: > thakis, what do you think of putting this code here? It doesn't depend on > anything in app_controller_mac, but app_controllor_mac handles other menu bar > related things. Friendly ping.
On 2013/07/17 07:36:49, jackhou1 wrote: > On 2013/07/05 12:49:15, jackhou1 wrote: > > thakis, what do you think of putting this code here? It doesn't depend on > > anything in app_controller_mac, but app_controllor_mac handles other menu bar > > related things. > > Friendly ping. Given that this file is already 1.4k lines, I think it'd be good to come up with some way to put the app stuff in it somewhere else eventually. Doesn't block this CL though.
…and as always: Tests?
PTAL. I changed AppControllerPlatformAppBrowserTest to subclass PlatformAppBrowserTest instead of InProcessBrowserTest. Would it be better to have a new fixture?
I was worried you might need an interactive_ui_test, but I like this approach - I think it avoids the kind of flakiness an interactive_ui_test would prevent, and still gets good coverage. On 2013/07/18 06:13:10, jackhou1 wrote: > I changed AppControllerPlatformAppBrowserTest to subclass PlatformAppBrowserTest > instead of InProcessBrowserTest. Would it be better to have a new fixture? I think the way you've done it makes sense, with the way app_controller_mac_browsertest.mm is currently set up. https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... chrome/browser/app_controller_mac_browsertest.mm:61: base::scoped_nsobject<AppController> ac([[AppController alloc] init]); nit: .. although this is used in an existing test, it probably goes against the "no abbreviations" rule - probably app_controller is better. https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... chrome/browser/app_controller_mac_browsertest.mm:65: const extensions::Extension* app_1 = Do you need the ExtensionTestMessageListener ? InstallAndLaunchPlatformApp will wait for NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME, which should be enough to ensure the window can take focus. But maybe something balks if there's nothing listening when the test apps send out the test message. https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... chrome/browser/app_controller_mac_browsertest.mm:88: NSMenuItem* last_item = [item_array objectAtIndex:[item_array count] - 1]; Maybe [item_array lastObject]
https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... chrome/browser/app_controller_mac_browsertest.mm:61: base::scoped_nsobject<AppController> ac([[AppController alloc] init]); On 2013/07/19 03:33:23, tapted wrote: > nit: .. although this is used in an existing test, it probably goes against the > "no abbreviations" rule - probably app_controller is better. Done. https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... chrome/browser/app_controller_mac_browsertest.mm:65: const extensions::Extension* app_1 = On 2013/07/19 03:33:23, tapted wrote: > Do you need the ExtensionTestMessageListener ? InstallAndLaunchPlatformApp will > wait for NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME, which should be enough to > ensure the window can take focus. But maybe something balks if there's nothing > listening when the test apps send out the test message. I think there's a race somewhere. The first app and its window are fine, but for the second app, that notification fires before the shell window is added to ShellWindowRegistry. https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... chrome/browser/app_controller_mac_browsertest.mm:88: NSMenuItem* last_item = [item_array objectAtIndex:[item_array count] - 1]; On 2013/07/19 03:33:23, tapted wrote: > Maybe [item_array lastObject] Done.
https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/18089012/diff/35001/chrome/browser/app_contro... chrome/browser/app_controller_mac_browsertest.mm:65: const extensions::Extension* app_1 = On 2013/07/19 07:47:16, jackhou1 wrote: > On 2013/07/19 03:33:23, tapted wrote: > > Do you need the ExtensionTestMessageListener ? InstallAndLaunchPlatformApp > will > > wait for NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME, which should be enough to > > ensure the window can take focus. But maybe something balks if there's nothing > > listening when the test apps send out the test message. > > I think there's a race somewhere. The first app and its window are fine, but for > the second app, that notification fires before the shell window is added to > ShellWindowRegistry. Ah, yes. I think there might be two LOAD_COMPLETED_MAIN_FRAMEs for each, due to http://crbug.com/123007 . So, yeah - better to wait for the test message.
lgtm https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.h:85: // The main menu item showing the name of a currently focussed packaged app. nit: focused https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1425: [item setHidden:YES]; nit: This could be in an else to the previous if, right? https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1431: base::scoped_nsobject<NSMenu> appMenu([[NSMenu alloc] initWithTitle:title]); Is the plan that apps can have their own menu entries?
https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.h (right): https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.h:85: // The main menu item showing the name of a currently focussed packaged app. On 2013/07/19 19:43:05, Nico wrote: > nit: focused Done. https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1425: [item setHidden:YES]; On 2013/07/19 19:43:05, Nico wrote: > nit: This could be in an else to the previous if, right? Done. https://codereview.chromium.org/18089012/diff/43001/chrome/browser/app_contro... chrome/browser/app_controller_mac.mm:1431: base::scoped_nsobject<NSMenu> appMenu([[NSMenu alloc] initWithTitle:title]); On 2013/07/19 19:43:05, Nico wrote: > Is the plan that apps can have their own menu entries? I'm planning to add Quit, Hide, and Show to this menu. I don't think it's been decided if apps will be able to define custom entries (or additional menus), but if that goes ahead, this code will probably either move out of here, or talk to some interface that provides the menus.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/18089012/50001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/18089012/50001
Message was sent while issue was closed.
Change committed as 213031 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
