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 (ping after 24hrs), 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 (ping after 24hrs), 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.

[blagus, 2016-01-14 21:19:42]

+1 for this request. Very usefull resource on Windows, I miss a lot on MacOS.
Please reconsider it.
In respect to the Apple Mac OS User Interface Guidelines, the best solution to
this problem is add the "Show as tab" at the "Window" menu. Please refer to the
mockup bellow:
http://s8.postimg.org/jmg461wb7/Screen_Shot_2016_01_14_at_19_02_58.png
(this is just a mockup, use the most suitable position in this menu)
thanks in advance.


On 2014/03/17 12:22:03, pink wrote:
> 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, <mailto: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
> mailto:pinkerton@google.com
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.