Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10833056:
Add a menu to the Action Box Button. (Closed)
|
Created:
8 years, 4 months ago by beaudoin Modified:
8 years, 4 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a menu (with placeholder items) to the Action Box Button.
Supersedes CL 10825003 (owner change).
BUG=138118
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151065
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Applied review comments. #
Total comments: 16
Patch Set 3 : Made model and controller method-scoped. #
Total comments: 2
Patch Set 4 : Final changes! #
Messages
Total messages: 13 (0 generated)
Sorry for starting a new patch, but since Cait is going on vacation it will be easier this way.
I'll be OOT next week. I tried IM'ing you, but perhaps you were already off ... anyhow, I think overall this should work, just needs some whacks. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (left): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:160: Egregious whitespace change is egregious. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.h (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:29: Browser* browser); Can drop explicit then. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:49: scoped_nsobject<NSPopUpButtonCell> popUpCell_; Probably want this one after menuController_ so that it destructs first. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:29: :owner_(owner), Either this line is indented wrong, or the others are. I think you also need a space after :. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:31: browser_(browser) { You aren't actually using browser_ at this time, AFAICT. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:41: popUpCell_.reset(nil); On 2012/07/27 19:01:54, beaudoin wrote: > On 2012/07/27 00:22:41, shess wrote: > > My guess is that the problem here is that the menu you're putting into the > > popUpCell_ has distinct ownership. It might be better to just let the cell > own > > the menu directly. > > MenuController keeps ownership of the menu (via a scoped_ptr). Should I add a > mechanism to make it possible for PlusDecoration to grab ownership of this and > pass it along to the PopupCell? Sorry, I may have made an assumption, here. Usually, scoped_nsobject<> can just be destructed implicitly. Sometimes an explicit reset is necessary because the object has weak references to things which might be destructed before the scoped_nsobject<> is destructed, so you get use-after-free. It's possible that this is just here because you saw it somewhere else, in which case you could just delete this line. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:62: useWithPopUpButtonCell:YES]); I think you should pull the menu_model_, menuController_, and popUpCell_ building out to the constructor, rather than doing it ad-hoc. This function should be targetted at popping up the menu. I think the code itself looks fine, I just would rather it wasn't a mixture of setting up the context and setting up the activity resulting from the mouse press. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:67: frame.size.height += 10.0; This magic constant could use some explaining. Actually, the other one, too :-). It looks like approximately half the decoration frame height, so it might be appropriate to use NSMidY() versus NSMinY() or something like that. Actually - OSX coordinate space can take a little getting used to, maybe it would be easier to ask where you want it relative to the frame of the decoration? I _think_ you're trying to pull it up relative to the overall omnibox, rather than the decoration. In that case, you should just use owner_->GetAutocompleteTextField() to get the omnibox, and call -bounds and adjust the width and x-pos of that appropriately. Probably something like: NSRect popupFrame = [field bounds]; popupFrame.origin.x = NSMaxX(popupFrame) - menu.size.width; popupFrame.size.width = menu.size.width; [-frame returns the rectangle a view is in within its parent view, whereas -bounds returns the rectangle the view thinks of itself in. To this function |frame| is within the containing view, which would be in the same coordinate space as what -bounds returns from that view.] http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:71: pullsDown:YES]); Auto-magic alignment is sometimes sketchy with Objective-C, but I think you need to align with : if possible. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:73: // Lazily-instantiated wrench menu controller. Drop this change if you aren't making other changes in here. Well, even if you are, I can figure out it refers to the wrench menu :-). http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:21: #include "chrome/browser/extensions/extension_system.h" Looks like this file can be reverted out of the CL.
Thanks for the very useful comments. It's my first real cocoa patch and I confess I've been struggling a bit with frames. I'll try to read on this, good references welcome! :) http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (left): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:160: On 2012/07/27 20:31:06, shess wrote: > Egregious whitespace change is egregious. Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.h (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:29: Browser* browser); On 2012/07/27 20:31:06, shess wrote: > Can drop explicit then. Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:49: scoped_nsobject<NSPopUpButtonCell> popUpCell_; On 2012/07/27 20:31:06, shess wrote: > Probably want this one after menuController_ so that it destructs first. Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:29: :owner_(owner), On 2012/07/27 20:31:06, shess wrote: > Either this line is indented wrong, or the others are. I think you also need a > space after :. Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:31: browser_(browser) { On 2012/07/27 20:31:06, shess wrote: > You aren't actually using browser_ at this time, AFAICT. If you don't mind I'll keep browser_ here as I have a CL already ready that needs it. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:41: popUpCell_.reset(nil); On 2012/07/27 20:31:06, shess wrote: > On 2012/07/27 19:01:54, beaudoin wrote: > > On 2012/07/27 00:22:41, shess wrote: > > > My guess is that the problem here is that the menu you're putting into the > > > popUpCell_ has distinct ownership. It might be better to just let the cell > > own > > > the menu directly. > > > > MenuController keeps ownership of the menu (via a scoped_ptr). Should I add a > > mechanism to make it possible for PlusDecoration to grab ownership of this and > > pass it along to the PopupCell? > > Sorry, I may have made an assumption, here. Usually, scoped_nsobject<> can just > be destructed implicitly. Sometimes an explicit reset is necessary because the > object has weak references to things which might be destructed before the > scoped_nsobject<> is destructed, so you get use-after-free. It's possible that > this is just here because you saw it somewhere else, in which case you could > just delete this line. Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:62: useWithPopUpButtonCell:YES]); On 2012/07/27 20:31:06, shess wrote: > I think you should pull the menu_model_, menuController_, and popUpCell_ > building out to the constructor, rather than doing it ad-hoc. This function > should be targetted at popping up the menu. > > I think the code itself looks fine, I just would rather it wasn't a mixture of > setting up the context and setting up the activity resulting from the mouse > press. At some point the menu model will be dynamic. Since we've decided to go with a local MenuModel rather than a custom controller, the menu could be out-of-sync if we create it in the constructor. Creating it at mouse pressed time ensures it is always synced when displayed. I've extracted the instantiation to a private method, let me know if it works for you. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:67: frame.size.height += 10.0; On 2012/07/27 20:31:06, shess wrote: > This magic constant could use some explaining. Actually, the other one, too > :-). It looks like approximately half the decoration frame height, so it might > be appropriate to use NSMidY() versus NSMinY() or something like that. > > Actually - OSX coordinate space can take a little getting used to, maybe it > would be easier to ask where you want it relative to the frame of the > decoration? I _think_ you're trying to pull it up relative to the overall > omnibox, rather than the decoration. In that case, you should just use > owner_->GetAutocompleteTextField() to get the omnibox, and call -bounds and > adjust the width and x-pos of that appropriately. > > Probably something like: > > NSRect popupFrame = [field bounds]; > popupFrame.origin.x = NSMaxX(popupFrame) - menu.size.width; > popupFrame.size.width = menu.size.width; > > [-frame returns the rectangle a view is in within its parent view, whereas > -bounds returns the rectangle the view thinks of itself in. To this function > |frame| is within the containing view, which would be in the same coordinate > space as what -bounds returns from that view.] Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:71: pullsDown:YES]); On 2012/07/27 20:31:06, shess wrote: > Auto-magic alignment is sometimes sketchy with Objective-C, but I think you need > to align with : if possible. Copied the height adjustment from somewhere where it was not documented. Dropped it and it still seems to work. :) Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:73: // Lazily-instantiated wrench menu controller. On 2012/07/27 20:31:06, shess wrote: > Drop this change if you aren't making other changes in here. Well, even if you > are, I can figure out it refers to the wrench menu :-). Done. http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:21: #include "chrome/browser/extensions/extension_system.h" On 2012/07/27 20:31:06, shess wrote: > Looks like this file can be reverted out of the CL. Done.
+isherman@chromium.org Trying to move this one forward. :)
http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:62: useWithPopUpButtonCell:YES]); On 2012/08/01 02:23:26, beaudoin wrote: > On 2012/07/27 20:31:06, shess wrote: > > I think you should pull the menu_model_, menuController_, and popUpCell_ > > building out to the constructor, rather than doing it ad-hoc. This function > > should be targetted at popping up the menu. > > > > I think the code itself looks fine, I just would rather it wasn't a mixture of > > setting up the context and setting up the activity resulting from the mouse > > press. > > At some point the menu model will be dynamic. Since we've decided to go with a > local MenuModel rather than a custom controller, the menu could be out-of-sync > if we create it in the constructor. Creating it at mouse pressed time ensures it > is always synced when displayed. > > I've extracted the instantiation to a private method, let me know if it works > for you. To be clear - there's two parts, here, there's setting up the objects used to construct the menu, and there's getting the menu. I expect the menu model to end up being something which you get from some other code, either at setup or by asking the browser or something, so it could exist before this method. The menu controller I'm less certain about. I _think_ that you could probably have it scoped to this function, as the -performClickWithFrame:inView: will run a nested message loop. If that is not the case, then you need to reset the controller every time and re-create it, because it caches the menu built. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... File chrome/browser/ui/cocoa/location_bar/plus_decoration.h (right): http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:29: Browser* browser); Double-check the style guide as to whether continuation lines should line up indented further (like L, C, and B in a vertical line). http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:55: scoped_nsobject<MenuController> menuController_; Studly caps is for Objective-C files. Admittedly ambiguous at this point, but I'd lean towards c++ style. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:57: // Popup cell. Redundant comments document things multiple times! http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:14: #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.h" Duplicate import. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:56: popupFrame.origin.y += kOmniboxYOffset; Is there any possibility that this ends up at NSMinY(frame)? http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:60: popUpCell_ .reset([[NSPopUpButtonCell alloc] initTextCell:@"" odd space before .reset(). Also, I'd prefer you just initialize it in the constructor. Cells are generally pretty cheap. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:83: OmniboxViewMac::ImageForResource(IDR_MOBILE))); Group the InsertItem() and SetIcon() calls for a given offset together, to make future editting more robust. Also, maybe poke around and see if there's a direct resource_id->skbitmap helper somewhere. I think the OmniboxViewMac::ImageForResource() helper used to do more than it does now, so here it might be worthless. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:88: [[MenuController alloc] initWithModel:menu_model_.get() Did you try it without the .get()? If you don't need it, you can drop it.
resources lgtm
http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:62: useWithPopUpButtonCell:YES]); On 2012/08/08 15:00:14, shess wrote: > On 2012/08/01 02:23:26, beaudoin wrote: > > On 2012/07/27 20:31:06, shess wrote: > > > I think you should pull the menu_model_, menuController_, and popUpCell_ > > > building out to the constructor, rather than doing it ad-hoc. This function > > > should be targetted at popping up the menu. > > > > > > I think the code itself looks fine, I just would rather it wasn't a mixture > of > > > setting up the context and setting up the activity resulting from the mouse > > > press. > > > > At some point the menu model will be dynamic. Since we've decided to go with a > > local MenuModel rather than a custom controller, the menu could be out-of-sync > > if we create it in the constructor. Creating it at mouse pressed time ensures > it > > is always synced when displayed. > > > > I've extracted the instantiation to a private method, let me know if it works > > for you. > > To be clear - there's two parts, here, there's setting up the objects used to > construct the menu, and there's getting the menu. I expect the menu model to > end up being something which you get from some other code, either at setup or by > asking the browser or something, so it could exist before this method. > > The menu controller I'm less certain about. I _think_ that you could probably > have it scoped to this function, as the -performClickWithFrame:inView: will run > a nested message loop. If that is not the case, then you need to reset the > controller every time and re-create it, because it caches the menu built. On the windows side of things the menu model is created on-the-fly, via new ActionBoxMenuModel(), whenever a menu is needed. I too, would have expected it to be a longer lived object, but it's not. I guess it's simpler this way as it does not have to observe the content that can change dynamically. Not sure I understand your comment for the controller. If you want me to change anything in here please let me know. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... File chrome/browser/ui/cocoa/location_bar/plus_decoration.h (right): http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:29: Browser* browser); On 2012/08/08 15:00:14, shess wrote: > Double-check the style guide as to whether continuation lines should line up > indented further (like L, C, and B in a vertical line). Done. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:55: scoped_nsobject<MenuController> menuController_; On 2012/08/08 15:00:14, shess wrote: > Studly caps is for Objective-C files. Admittedly ambiguous at this point, but > I'd lean towards c++ style. Done. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.h:57: // Popup cell. On 2012/08/08 15:00:14, shess wrote: > Redundant comments document things multiple times! Done. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:14: #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.h" On 2012/08/08 15:00:14, shess wrote: > Duplicate import. Done. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:60: popUpCell_ .reset([[NSPopUpButtonCell alloc] initTextCell:@"" On 2012/08/08 15:00:14, shess wrote: > odd space before .reset(). > > Also, I'd prefer you just initialize it in the constructor. Cells are generally > pretty cheap. Done. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:83: OmniboxViewMac::ImageForResource(IDR_MOBILE))); On 2012/08/08 15:00:14, shess wrote: > Group the InsertItem() and SetIcon() calls for a given offset together, to make > future editting more robust. > > Also, maybe poke around and see if there's a direct resource_id->skbitmap helper > somewhere. I think the OmniboxViewMac::ImageForResource() helper used to do > more than it does now, so here it might be worthless. Done. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:88: [[MenuController alloc] initWithModel:menu_model_.get() On 2012/08/08 15:00:14, shess wrote: > Did you try it without the .get()? If you don't need it, you can drop it. Seems needed.
http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:62: useWithPopUpButtonCell:YES]); On 2012/08/08 21:38:08, beaudoin wrote: > On the windows side of things the menu model is created on-the-fly, via new > ActionBoxMenuModel(), whenever a menu is needed. I too, would have expected it > to be a longer lived object, but it's not. I guess it's simpler this way as it > does not have to observe the content that can change dynamically. OK, follow Windows, then. > Not sure I understand your comment for the controller. Basically, I think that -performClickWithFrame: will not return until the menu is dismissed (it runs a nested message loop). So you can scope the menu controller and model to this function, rather than to the object. You should check that I'm speaking sense, of course!
http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/2001/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:62: useWithPopUpButtonCell:YES]); On 2012/08/08 22:04:00, shess wrote: > On 2012/08/08 21:38:08, beaudoin wrote: > > On the windows side of things the menu model is created on-the-fly, via new > > ActionBoxMenuModel(), whenever a menu is needed. I too, would have expected it > > to be a longer lived object, but it's not. I guess it's simpler this way as it > > does not have to observe the content that can change dynamically. > > OK, follow Windows, then. > > > Not sure I understand your comment for the controller. > > Basically, I think that -performClickWithFrame: will not return until the menu > is dismissed (it runs a nested message loop). So you can scope the menu > controller and model to this function, rather than to the object. You should > check that I'm speaking sense, of course! Made model and controller method-scoped. It works and I confirmed that performClickWithFrame is blocking (via logging). Done. http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/11001/chrome/browser/ui/cocoa/lo... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:56: popupFrame.origin.y += kOmniboxYOffset; On 2012/08/08 15:00:14, shess wrote: > Is there any possibility that this ends up at NSMinY(frame)? Dug a bit deeper here. Using the field height leads to a popup that crosses the bottom border of the text box, and I did not find a nice way to clear it withouth a constant (tried superview and a couple of other things). The code doing something similar for the omnibox popup is even messier and full of constants. :) I still cleaned this up a little, as the popupFrame expected here is the frame to which the menu will be attached (NOT the menu's frame). Took me a while to understand this. :)
lgtm with the popup cell scoped as indicated. http://codereview.chromium.org/10833056/diff/4002/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/4002/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:43: DCHECK(pop_up_cell_.get()); Guess what! Now that the model and controller are all scoped to OnMousePressed(), you can make this a scoped_nsobject<> in that method! Sorry I didn't see that earlier, it should have been obvious, sigh.
http://codereview.chromium.org/10833056/diff/4002/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right): http://codereview.chromium.org/10833056/diff/4002/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:43: DCHECK(pop_up_cell_.get()); On 2012/08/09 22:59:10, shess wrote: > Guess what! Now that the model and controller are all scoped to > OnMousePressed(), you can make this a scoped_nsobject<> in that method! Sorry I > didn't see that earlier, it should have been obvious, sigh. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10833056/6006
Change committed as 151065 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
