Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
864153003:
Added 'Find'/'Paste and Match Style' menu items for hosted apps on Mac. (Closed)
DescriptionAdded 'Find'/'Paste and Match Style' menu items for hosted apps on Mac.
This feature should exist for hosted apps, currently there is no option
to search a page. Instead of removing these two menu items, they are now
hidden if the app that is currently focussed is not a hosted app.
BUG=450834
Committed: https://crrev.com/a80419dcbde7c1a1bd6a6eb8e2074f1a2c643f5a
Cr-Commit-Position: refs/heads/master@{#314272}
Patch Set 1 #Patch Set 2 : Now hide menu items instead of removing them #Patch Set 3 : Updated comments #
Total comments: 1
Patch Set 4 : Refactored hiding menu items #
Total comments: 17
Patch Set 5 : Added test #
Total comments: 22
Patch Set 6 : Refactoring #
Total comments: 10
Patch Set 7 : More refactoring and comments #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : Updated logic in SetItemWithTagVisible #
Total comments: 2
Patch Set 10 : Refactoring logic in SetItemWithTagVisible #Patch Set 11 : ExtensionUninstallUpdatesMenuBar now passes #
Total comments: 2
Patch Set 12 : Added flag to SetUpApps #
Total comments: 4
Patch Set 13 : Nits #Patch Set 14 : Moved enum declaration #
Total comments: 4
Patch Set 15 : Commenting #
Messages
Total messages: 43 (10 generated)
mitchelljones@chromium.org changed reviewers: + tapted@chromium.org
tapted@ could you please review? Thanks.
+chrome-apps-syd-reviews What happens if you open a packaged app, then switch to a hosted app? Do the removed items reappear?
On 2015/01/22 03:50:22, tapted wrote: > +chrome-apps-syd-reviews > > What happens if you open a packaged app, then switch to a hosted app? Do the > removed items reappear? Good point, I've changed it so menu items are now hidden instead. Could you ptal?
https://codereview.chromium.org/864153003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:92: if ([nextMenuItem isAlternate]) { Maybe http://stackoverflow.com/questions/11494509/nsmenuitem-sethidden-doesnt-work has the answer Possible other alternatives to the one in stackoverflow: - perhaps disabling the item rather than setting it hidden. - or: packagedAppEditMenuItem_.reset([[[NSApp mainMenu] itemWithTag:IDC_EDIT_MENU] copy]); RemoveFoo... hostedAppEditMenuItem_.reset([[[NSApp mainMenu] itemWithTag:IDC_EDIT_MENU] copy]); Try them out. It might also depend how "deep" the -[NSMenuItem copy] is. I.e. if they point to the same subitem, disabling/hiding it might have implications for the menu shown when a browser is active.
I ended up sticking with the current method. Could you ptal?
The WithTag comment takes precednce, which might make some of the other comments obsolete :) Also make sure you flesh out the CL description. It needs to have: - Summary - Why this change is being made. - How the change is made. Also it needs a test - there's already a AppShimMenuControllerBrowserTest.PlatformAppFocusUpdatesMenuBar -- we need something for hosted apps too and they should check this stuff. Importantly, the test needs to fail if someone ever decides to fix that glitch you noticed with the paste-and-match-style alternate. E.g. if that alternate item were to be removed from the default model, very strange stuff would be happening here. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:78: void HideMenuItemWithIndex(NSMenuItem* top_level_item, Since it both hides and shows, a better name would be something like `SetItemAtIndexVisible` (and invert the meaning of hidden -> visible so there isn't a double negative https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:78: void HideMenuItemWithIndex(NSMenuItem* top_level_item, Comment for this function - e.g. it's a bit subtle what's going on with the alternate stuff, and why it's necessary to pass it in rather than get it from [NSMenuItem isAlternate] https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:79: NSInteger index, could you use WithTag instead? combined with has_alternate, this should be sufficient and help encapsulate the oddness around those https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:81: bool isAlternateMenuItem) { since this isn't an Objective C method, use hacker_style for arguments. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:83: if (index >= [submenu numberOfItems]) if this ever hit? DCHECK instead? https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:395: NSInteger pasteMatchStyleindex = [[editMenuItem_ submenu] index -> Index https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:398: // We need to hide the alternate menu item to "Patch and Match Style" I'd say this more like // AppKit requires that alternate menu items are hidden first. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:400: HideMenuItemWithIndex(editMenuItem_, pasteMatchStyleindex + 1, YES, YES); since they are being passed to bool (not BOOL) use true/false instead of YES/NO https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:404: [[editMenuItem_ submenu] indexOfItemWithTag:IDC_FIND_MENU], YES, NO); nit: make a temporary for this, same as pasteMatchStyle, since it's spread over 3 lines here anyway.
Added a test as well. Could you please take a look at that? https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:78: void HideMenuItemWithIndex(NSMenuItem* top_level_item, On 2015/01/29 05:13:37, tapted wrote: > Comment for this function - e.g. it's a bit subtle what's going on with the > alternate stuff, and why it's necessary to pass it in rather than get it from > [NSMenuItem isAlternate] Done. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:79: NSInteger index, On 2015/01/29 05:13:37, tapted wrote: > could you use WithTag instead? combined with has_alternate, this should be > sufficient and help encapsulate the oddness around those Done. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:81: bool isAlternateMenuItem) { On 2015/01/29 05:13:37, tapted wrote: > since this isn't an Objective C method, use hacker_style for arguments. Done. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:83: if (index >= [submenu numberOfItems]) On 2015/01/29 05:13:37, tapted wrote: > if this ever hit? DCHECK instead? This is now checked, as we are grabbing the next menu item. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:395: NSInteger pasteMatchStyleindex = [[editMenuItem_ submenu] On 2015/01/29 05:13:37, tapted wrote: > index -> Index Done. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:398: // We need to hide the alternate menu item to "Patch and Match Style" On 2015/01/29 05:13:37, tapted wrote: > I'd say this more like > > // AppKit requires that alternate menu items are hidden first. Done. Moved this comment to the new function SetItemWithTagVisible. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:400: HideMenuItemWithIndex(editMenuItem_, pasteMatchStyleindex + 1, YES, YES); On 2015/01/29 05:13:37, tapted wrote: > since they are being passed to bool (not BOOL) use true/false instead of YES/NO Done. https://codereview.chromium.org/864153003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:404: [[editMenuItem_ submenu] indexOfItemWithTag:IDC_FIND_MENU], YES, NO); On 2015/01/29 05:13:37, tapted wrote: > nit: make a temporary for this, same as pasteMatchStyle, since it's spread over > 3 lines here anyway. Done.
https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:79: // Assumes that the menu item only has exactly one alternate, since AppKit I think it needs to say If |has_alternate| is true, the item immediately following |item_tag| is assumed to be its (only) alternate. Since AppKit is unable to hide items with alternates, |has_alternate| will cause -[NSMenuItem alternate] to be removed when hiding and restored when showing. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:86: NSMenuItem* menuItem = [submenu itemWithTag:item_tag]; Since it's not an objective-C method, hacker_style for variables - menu_item, next_index. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:86: NSMenuItem* menuItem = [submenu itemWithTag:item_tag]; itemWithTag returns `nil` if the tag isn't found. `[submenu indexOfItem:nil]` might then return -1 ... which we add 1 to, thus making `nextIndex` a valid, `0`. So: DCHECK(menu_item) after this. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:90: if (!has_alternate || nextIndex >= [submenu numberOfItems]) { I think `nextIndex >= [submenu numberOfItems]` is not a valid thing to check here. If has_alternate is true, it is a programming error to specify something that doesn't have an item after it. Can you move the |next_index| declaration further down. and turn the out-of-bounds check into a DCHECK? https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:404: editMenuItem_, IDC_CONTENT_CONTEXT_PASTE_AND_MATCH_STYLE, false, true); how about SetItemWithTagVisible( editMenuItem_, IDC_CONTENT_CONTEXT_PASTE_AND_MATCH_STYLE, app->is_hosted_app(), true); SetItemWithTagVisible( editMenuItem_, IDC_FIND_MENU, app->is_hosted_app(), false); https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:35: hosted_app_(NULL), nit: NULL -> nullptr. You can change those nearby too. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:89: const int kEditMenuIndex = initial_menu_item_count_ + 2; nit: for function-local consts, we're meant to use hacker_style (i.e. as if it were not const) https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:91: NSMenuItem* editMenu = editMenu -> edit_menu, more below (camelCase only between @interface/@implementation Foo -> @end) https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:163: Browser* browser; = nullptr; And perhaps a better name, like hosted_app_browser (it's weird to call a hosted app `browser` even though it's a Browser*) https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:166: for (Browser* b : *browsers) { `b` isn't really a common abbreviation. Maybe for (Browser* browser: *browser_list) { https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:186: ->GetAppWindowsForApp(app_1_->id()).front(); Calling .front() is a bit spooky... and this is repeated in the other tests. Can you make helper function in AppShimMenuControllerBrowserTest like NSWindow* FirstWindowForApp(Extension* app) { AppWindowRegistry::AppWindowList window_list = extensions::AppWindowRegistry::Get(profile()) ->GetAppWindowsForApp(app->id()) EXPECT_FALSE(window_list.empty()); return window_list.front(); } then refactor PlatformAppFocusUpdatesMenuBar to use it as well
https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:79: // Assumes that the menu item only has exactly one alternate, since AppKit On 2015/01/30 00:52:48, tapted wrote: > I think it needs to say > > If |has_alternate| is true, the item immediately following |item_tag| is assumed > to be its (only) alternate. Since AppKit is unable to hide items with > alternates, |has_alternate| will cause -[NSMenuItem alternate] to be removed > when hiding and restored when showing. > Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:86: NSMenuItem* menuItem = [submenu itemWithTag:item_tag]; On 2015/01/30 00:52:48, tapted wrote: > Since it's not an objective-C method, hacker_style for variables - menu_item, > next_index. Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:86: NSMenuItem* menuItem = [submenu itemWithTag:item_tag]; On 2015/01/30 00:52:48, tapted wrote: > itemWithTag returns `nil` if the tag isn't found. `[submenu indexOfItem:nil]` > might then return -1 ... which we add 1 to, thus making `nextIndex` a valid, > `0`. So: DCHECK(menu_item) after this. Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:90: if (!has_alternate || nextIndex >= [submenu numberOfItems]) { On 2015/01/30 00:52:48, tapted wrote: > I think `nextIndex >= [submenu numberOfItems]` is not a valid thing to check > here. If has_alternate is true, it is a programming error to specify something > that doesn't have an item after it. > > Can you move the |next_index| declaration further down. and turn the > out-of-bounds check into a DCHECK? Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:404: editMenuItem_, IDC_CONTENT_CONTEXT_PASTE_AND_MATCH_STYLE, false, true); On 2015/01/30 00:52:48, tapted wrote: > how about > > SetItemWithTagVisible( > editMenuItem_, IDC_CONTENT_CONTEXT_PASTE_AND_MATCH_STYLE, > app->is_hosted_app(), true); > SetItemWithTagVisible( > editMenuItem_, IDC_FIND_MENU, app->is_hosted_app(), false); Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:35: hosted_app_(NULL), On 2015/01/30 00:52:48, tapted wrote: > nit: NULL -> nullptr. You can change those nearby too. Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:89: const int kEditMenuIndex = initial_menu_item_count_ + 2; On 2015/01/30 00:52:49, tapted wrote: > nit: for function-local consts, we're meant to use hacker_style (i.e. as if it > were not const) Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:91: NSMenuItem* editMenu = On 2015/01/30 00:52:48, tapted wrote: > editMenu -> edit_menu, more below > > (camelCase only between @interface/@implementation Foo -> @end) Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:163: Browser* browser; On 2015/01/30 00:52:48, tapted wrote: > = nullptr; And perhaps a better name, like hosted_app_browser (it's weird to > call a hosted app `browser` even though it's a Browser*) Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:166: for (Browser* b : *browsers) { On 2015/01/30 00:52:48, tapted wrote: > `b` isn't really a common abbreviation. Maybe > > for (Browser* browser: *browser_list) { Done. https://codereview.chromium.org/864153003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:186: ->GetAppWindowsForApp(app_1_->id()).front(); On 2015/01/30 00:52:48, tapted wrote: > Calling .front() is a bit spooky... and this is repeated in the other tests. Can > you make helper function in AppShimMenuControllerBrowserTest like > > NSWindow* FirstWindowForApp(Extension* app) { > AppWindowRegistry::AppWindowList window_list = > extensions::AppWindowRegistry::Get(profile()) > ->GetAppWindowsForApp(app->id()) > EXPECT_FALSE(window_list.empty()); > return window_list.front(); > } > > then refactor PlatformAppFocusUpdatesMenuBar to use it as well Done.
lgtm % nits and the `if` removed https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:78: // If |has_alternate| is true, the item immediately following |item_tag| is keep the `// Sets the menu item with |item_tag| in |top_level_item| visible.` bit above this. https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:98: NSMenuItem* next_menu_item = [submenu itemAtIndex:next_index]; nit: next_menu_item -> alternate_item is probably clearer https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:411: } else { this `else` is now identical to the non-else -> no if needed. https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:188: extensions::AppWindow* app_1_app_window = FirstWindowForApp(app_1_); nit: this temporary probably isn't needed. https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:203: extensions::AppWindow* app_2_app_window = FirstWindowForApp(app_2_); nit: can do without the temporaries here too
https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:78: // If |has_alternate| is true, the item immediately following |item_tag| is On 2015/01/30 03:06:22, tapted wrote: > keep the `// Sets the menu item with |item_tag| in |top_level_item| visible.` > bit above this. Done. https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:98: NSMenuItem* next_menu_item = [submenu itemAtIndex:next_index]; On 2015/01/30 03:06:22, tapted wrote: > nit: next_menu_item -> alternate_item is probably clearer Done. https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:411: } else { On 2015/01/30 03:06:22, tapted wrote: > this `else` is now identical to the non-else -> no if needed. Oops, you're right. https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:188: extensions::AppWindow* app_1_app_window = FirstWindowForApp(app_1_); On 2015/01/30 03:06:22, tapted wrote: > nit: this temporary probably isn't needed. Done. https://codereview.chromium.org/864153003/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:203: extensions::AppWindow* app_2_app_window = FirstWindowForApp(app_2_); On 2015/01/30 03:06:22, tapted wrote: > nit: can do without the temporaries here too Done.
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/864153003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/864153003/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:100: [alternate_item setAlternate:visible]; hm. since there's something to fix anyway.. I thought of a thing that would make me less scared about that alternate item getting dropped from the menu model. I think, as it is, all the tests would pass and something weird would happen. But, to be safe, here I think you could change this line to: if (visible && [alternate_item isHidden]) { // Only restore alternate if we've previously hidden the item. [alternate_item setAlternate:YES]; } else if (!visible) { DCHECK([alternate_item isAlternate]); [alternate_item setAlternate:NO]; }
Oops, forgot to rebase. The compile error has been fixed now. https://codereview.chromium.org/864153003/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:100: [alternate_item setAlternate:visible]; On 2015/01/30 05:02:31, tapted wrote: > hm. since there's something to fix anyway.. I thought of a thing that would make > me less scared about that alternate item getting dropped from the menu model. I > think, as it is, all the tests would pass and something weird would happen. > > But, to be safe, here I think you could change this line to: > > if (visible && [alternate_item isHidden]) { > // Only restore alternate if we've previously hidden the item. > [alternate_item setAlternate:YES]; > } else if (!visible) { > DCHECK([alternate_item isAlternate]); > [alternate_item setAlternate:NO]; > } > Done.
https://codereview.chromium.org/864153003/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:109: [alternate_item setAlternate:NO]; we still want DCHECK([alternate_item isAlternate]); before this check my logic... but remember the goal: we want to detect if the menu model (which is separate from this code) is changed, and then breaks any of the assumptions that this code is making. One of those assumptions is that passing `has_alternate` is only done for menu items that have alternates.
https://codereview.chromium.org/864153003/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right): https://codereview.chromium.org/864153003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:109: [alternate_item setAlternate:NO]; On 2015/02/02 00:05:52, tapted wrote: > we still want DCHECK([alternate_item isAlternate]); before this > > check my logic... but remember the goal: we want to detect if the menu model > (which is separate from this code) is changed, and then breaks any of the > assumptions that this code is making. > > One of those assumptions is that passing `has_alternate` is only done for menu > items that have alternates. Right. I think adding the check if (visible != [menu_item isHidden]) ... Is enough. If we reach like 103, then we know that: The menu_item (and therefore the alternate_item) are hidden (not hidden) and we want to make them visible (not visible). If we want to make the items visible, then we DCHECK that the alternate_item is hidden (which must be true, since these operations are performed on lines 111-112). Otherwise if we want to hide the items, then alternate_item must be an alternate (since has_alternate was true), and therefore it must not be hidden. Of course, if these DCHECKs fail, then the menu model must have been modified elsewhere.
sweet! patchset 10 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/864153003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
tapted@ could you ptal just to double check it all looks good? The last test was failing because in the SetUpApps function, I'm now launching a hosted app in addition to the other two packaged apps. The test ExtensionUninstallUpdatesMenuBar closes the windows associated with these two packaged apps and assumes that there will be no other windows open. However with this patch, this meant that the hosted app window was still open, resulting in a failure. My latest patch fixes this by simply launching the hosted app window inside the test HostedAppHasAdditionalEditMenuItems instead of in the SetUpApps function.
https://codereview.chromium.org/864153003/diff/200001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:50: hosted_app_ = InstallHostedApp(); hm - this isn't really used for other tests, and app_2_ isn't used for the new test. Later tests might want a hosted app too. How about an argument for SetUpApps? e.g. enum { PACKAGED_1 = 0x1, PACKAGED_2 = 0x2, HOSTED = 0x4 };
https://codereview.chromium.org/864153003/diff/200001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:50: hosted_app_ = InstallHostedApp(); On 2015/02/02 23:35:56, tapted wrote: > hm - this isn't really used for other tests, and app_2_ isn't used for the new > test. Later tests might want a hosted app too. > > How about an argument for SetUpApps? > > e.g. > > enum { PACKAGED_1 = 0x1, PACKAGED_2 = 0x2, HOSTED = 0x4 }; Done.
lgtm % nits https://codereview.chromium.org/864153003/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:42: // Start two apps and wait for them to be launched. nit: update comment. Ensure you say |flags| is a bitmask of AvailableApps. https://codereview.chromium.org/864153003/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:122: enum AvailableApps { PACKAGED_1 = 0x1, PACKAGED_2 = 0x2, HOSTED = 0x4 }; nit: types come first in any public/private/protected section.
https://codereview.chromium.org/864153003/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:42: // Start two apps and wait for them to be launched. On 2015/02/03 00:20:45, tapted wrote: > nit: update comment. Ensure you say |flags| is a bitmask of AvailableApps. Done. https://codereview.chromium.org/864153003/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:122: enum AvailableApps { PACKAGED_1 = 0x1, PACKAGED_2 = 0x2, HOSTED = 0x4 }; On 2015/02/03 00:20:45, tapted wrote: > nit: types come first in any public/private/protected section. Done.
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/864153003/260001
#pedant https://codereview.chromium.org/864153003/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:44: // Start and app and wait for it to be launched. |flags| is a bitmask of nit: and -> an? Or `Start apps and wait for them to launch.` https://codereview.chromium.org/864153003/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:45: // AvailableApps, which are the apps that can be installed and launched. nit: The stuff after the comma probably belongs on `AvailableApps` (minus "which are". e.g. up on line 31, // The apps that can be installed and launched by SetUpApps().
The CQ bit was unchecked by tapted@chromium.org
https://codereview.chromium.org/864153003/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm (right): https://codereview.chromium.org/864153003/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:44: // Start and app and wait for it to be launched. |flags| is a bitmask of On 2015/02/03 00:42:46, tapted wrote: > nit: and -> an? Or `Start apps and wait for them to launch.` Done. https://codereview.chromium.org/864153003/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:45: // AvailableApps, which are the apps that can be installed and launched. On 2015/02/03 00:42:46, tapted wrote: > nit: The stuff after the comma probably belongs on `AvailableApps` (minus "which > are". > e.g. up on line 31, > > // The apps that can be installed and launched by SetUpApps(). Done.
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/864153003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (None)
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/864153003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/a80419dcbde7c1a1bd6a6eb8e2074f1a2c643f5a Cr-Commit-Position: refs/heads/master@{#314272} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
