Mac: Show a context menu for poup window.

Mac: Show a context menu for poup window.
- Added PopupStripeView for handling click events on the title bar.
- Showed a context menu when the view is right-clicked.

BUG=43953
TEST=manual

[gmorrita, 2010-08-20 11:11:57]

Hi Nico, pink,
Tried to add a context menu for popup window,
say, one that opened with window.open("...", "toolbar=0").
Could you take a look?

I know I'm stepping in some tricky area.
Any advices are welcome!

[viettrungluu, 2010-08-20 17:02:00]

Also, in your CL description, please describe the changes to the .xib file.

(How is the context menu hooked up to anything which will make it show?)

http://codereview.chromium.org/3181029/diff/1/6
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/3181029/diff/1/6#newcode147
chrome/browser/cocoa/browser_window_controller.mm:147: //
BrowserWindowPopupMenuController subclasses MenuController to
Why is this in browser_window_controller.mm?

http://codereview.chromium.org/3181029/diff/1/6#newcode150
chrome/browser/cocoa/browser_window_controller.mm:150:
scoped_ptr<menus::SimpleMenuModel> popupModel_;
@private

http://codereview.chromium.org/3181029/diff/1/6#newcode157
chrome/browser/cocoa/browser_window_controller.mm:157: -
(menus::SimpleMenuModel*)newModelFor:(menus::SimpleMenuModel::Delegate*)delegate
{
80 chars

http://codereview.chromium.org/3181029/diff/1/6#newcode173
chrome/browser/cocoa/browser_window_controller.mm:173: if ((self = [super init])
!= nil) {
'!= nil' not needed

http://codereview.chromium.org/3181029/diff/1/6#newcode175
chrome/browser/cocoa/browser_window_controller.mm:175: [self setModel:
popupModel_.get()];
no space after ':'

http://codereview.chromium.org/3181029/diff/1/6#newcode1679
chrome/browser/cocoa/browser_window_controller.mm:1679: -
(NSMenu*)popupContextMenu {
Should you check that this is a popup window?

http://codereview.chromium.org/3181029/diff/1/6#newcode1682
chrome/browser/cocoa/browser_window_controller.mm:1682:
[[BrowserWindowPopupMenuController alloc] initWithDelegate:windowShim_.get()]);
80 chars

http://codereview.chromium.org/3181029/diff/1/7
File chrome/browser/cocoa/tab_window_controller.h (right):

http://codereview.chromium.org/3181029/diff/1/7#newcode37
chrome/browser/cocoa/tab_window_controller.h:37: @class PopupStripView;
alphabetical order

http://codereview.chromium.org/3181029/diff/1/7#newcode176
chrome/browser/cocoa/tab_window_controller.h:176: // Returns a context menu for
popup (non-tab) window.
It's just a tad strange that you're talking about popup windows in a *tab*
window controller.

http://codereview.chromium.org/3181029/diff/1/8
File chrome/browser/cocoa/tab_window_controller.mm (right):

http://codereview.chromium.org/3181029/diff/1/8#newcode17
chrome/browser/cocoa/tab_window_controller.mm:17: @protected
why protected and not private?

http://codereview.chromium.org/3181029/diff/1/8#newcode18
chrome/browser/cocoa/tab_window_controller.mm:18: TabWindowController*
controller_; // weak: own us.
s/own/owns/

http://codereview.chromium.org/3181029/diff/1/8#newcode21
chrome/browser/cocoa/tab_window_controller.mm:21: -
(void)setController:(TabWindowController*)controller;
Just declare it as a @property; in general, things with setters should have
getters.

http://codereview.chromium.org/3181029/diff/1/8#newcode110
chrome/browser/cocoa/tab_window_controller.mm:110: NSRect contentFrame = [[[self
window] contentView] frame];
Please do |NSView* contentView = [[self window] contentView];| and use that.

http://codereview.chromium.org/3181029/diff/1/8#newcode120
chrome/browser/cocoa/tab_window_controller.mm:120: NSView* contentParent =
[[[self window] contentView] superview];
You can probably now do |[[contentView superview] addSubview:popupStripView_];|
below. Or, if you want to do this, you may as well do it at the top.

http://codereview.chromium.org/3181029/diff/1/8#newcode390
chrome/browser/cocoa/tab_window_controller.mm:390: return nil; // Doesn't show a
context menu at the default.
s/at the/by/

[pink, 2010-08-20 19:15:06]

I haven't looked at the CL, but are we sure we really want this? Title
bars having context menus isn't something that's normal on Mac, and I
don't think we really should be going this route.

The whole concept seems kinda dubious...

On Fri, Aug 20, 2010 at 7:11 AM,  <morrita@google.com> wrote:
> Reviewers: Nico, pink,
>
> Message:
> Hi Nico, pink,
> Tried to add a context menu for popup window,
> say, one that opened with window.open("...", "toolbar=0").
> Could you take a look?
>
> I know I'm stepping in some tricky area.
> Any advices are welcome!
>
>
> Description:
> Mac: Show a context menu for poup window.
> - Added PopupStripeView for handling click events on the title bar.
> - Showed a context menu when the view is right-clicked.
>
> BUG=43953
> TEST=manual
>
> Please review this at http://codereview.chromium.org/3181029/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M chrome/app/nibs/BrowserWindow.xib
>  M chrome/browser/cocoa/browser_window_cocoa.h
>  M chrome/browser/cocoa/browser_window_cocoa.mm
>  M chrome/browser/cocoa/browser_window_controller.h
>  M chrome/browser/cocoa/browser_window_controller.mm
>  M chrome/browser/cocoa/tab_window_controller.h
>  M chrome/browser/cocoa/tab_window_controller.mm
>
>
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[gmorrita, 2010-08-21 09:15:27]

viettrungluu, pink, thank you for reviewing!
As pink mentioned, context-menu-on-titlebar might not be usual for Mac.
I'll search for other place to put "Show As Tab" menu, which is essentially
requested
in the bug report.

[nancy.xu, 2014-03-15 20:37:51]

On 2010/08/21 09:15:27, morrita wrote:
> viettrungluu, pink, thank you for reviewing!
> As pink mentioned, context-menu-on-titlebar might not be usual for Mac.
> I'll search for other place to put "Show As Tab" menu, which is essentially
> requested
> in the bug report.

I'd like to re-open this request.  It doesn't matter if there isn't a context
menu on title bar, but there needs to be some way to take a pop-up and put it
back in a tab.  In my scenario I'm trying to chromecast the pop-up and there
doesn't seem to be an intuitive way to do so because the pop-up is hiding the
chromecast extension.

[pink, 2014-03-17 12:22:03]

What about something added to the Windows menu to consolidate popups into
the window that opened them? I still believe that a context menu on the
title bar is unintuitive and undiscoverable.


On Sat, Mar 15, 2014 at 1:37 PM, <nancy.xu@gmail.com> wrote:

> On 2010/08/21 09:15:27, morrita wrote:
>
>> viettrungluu, pink, thank you for reviewing!
>> As pink mentioned, context-menu-on-titlebar might not be usual for Mac.
>> I'll search for other place to put "Show As Tab" menu, which is
>> essentially
>> requested
>> in the bug report.
>>
>
> I'd like to re-open this request.  It doesn't matter if there isn't a
> context
> menu on title bar, but there needs to be some way to take a pop-up and put
> it
> back in a tab.  In my scenario I'm trying to chromecast the pop-up and
> there
> doesn't seem to be an intuitive way to do so because the pop-up is hiding
> the
> chromecast extension.
>
> https://codereview.chromium.org/3181029/
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.