Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11498008:
RenderWidget popup should be a NSWindow so it can go outside the main window. (Closed)
|
Created:
8 years ago by keishi Modified:
7 years, 11 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, jam, penghuang+watch_chromium.org, sail+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, James Su, Elliot Glaysher Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRenderWidget popup should be a NSWindow so it can go outside the main window.
BUG=165338
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175488
Patch Set 1 #
Total comments: 19
Patch Set 2 : Fixed other comments #Patch Set 3 : Created EventHookApplication in content #
Total comments: 2
Patch Set 4 : Moved to ui/base/cocoa. Added Cr prefix. #Patch Set 5 : Shouldn't be calling setParentWindow #
Total comments: 17
Patch Set 6 : Removed cancelChildPopups #
Total comments: 6
Patch Set 7 : Removed closing child popups #
Total comments: 1
Patch Set 8 : Removed include #
Total comments: 4
Patch Set 9 : Used local monitor #
Total comments: 3
Patch Set 10 : Fixed nits #
Messages
Total messages: 45 (0 generated)
Autofill and <input type=date> popups should be able to be shown outside the web contents area. This patch creates a NSWindow for render widget popups.
Avi, you did popups originally, right? Probably best if you look at this. keishi: This needs a BUG= number to explain what you want to do. erg@ said last week that this is an issue in aura too and it's not clear what the plan is there, so that should probably be coordinated with some aura person too?
Yes, this was originally me, and I did it this way because I was trying to avoid child windows. But there's no reason why we shouldn't be able to go this route. Keishi: yes, we definitely need a bug # here. Am reviewing now.
https://codereview.chromium.org/11498008/diff/1/chrome/browser/ui/cocoa/tab_c... File chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm (right): https://codereview.chromium.org/11498008/diff/1/chrome/browser/ui/cocoa/tab_c... chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:59: - (void)startObservingClick { This is a change in behavior; how did we used to close popups with a click outside of them? Plus the menu click close is definitely new. https://codereview.chromium.org/11498008/diff/1/chrome/browser/ui/cocoa/tab_c... chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:106: defer:NO]; Is there any possible way to move this into content? This looks like boilerplate that every embedder will need. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_guest.h (right): https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_guest.h:62: WebContentsViewDelegate* delegate = NULL) OVERRIDE; I don't believe we allow default arguments like this. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:61: #include "content/public/browser/web_contents_view_delegate.h" alphabetical https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:151: stray return https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:351: pos.width(), pos.height()) display:YES]; Wrap "display:YES" to the next line; it gets lost here. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:355: [cocoa_view_ setCanBeKeyView:YES]; It seems silly to me to call -setCanBeKeyView above on line 340 and then re-set it here. Pick one. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:358: [[popup_window_ contentView] addSubview:cocoa_view_]; You may want to set the bindings on the cocoa_view_ to bind its size to the superview. This will help later... https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:479: display:YES]; This is wrong because rect.size() is in view coordinates, yet you use it here for both view and screen coordinate systems. My advice is to bind the cocoa_view_ to its superview (as I advised above), and here convert the size to global coordinates and resize the popup window. The view, having been bound to its superview, will follow suit and resize too.
In addition, there's already lots of machinery in RWHV to close it when it has a click outside, look in resignFirstResponder. How does this interact? Can we reuse that code? If not, can we remove it in favor of the new code rather than leaving it in?
https://codereview.chromium.org/11498008/diff/1/chrome/browser/ui/cocoa/tab_c... File chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm (right): https://codereview.chromium.org/11498008/diff/1/chrome/browser/ui/cocoa/tab_c... chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:59: - (void)startObservingClick { On 2012/12/10 20:52:06, Avi wrote: > This is a change in behavior; how did we used to close popups with a click > outside of them? Plus the menu click close is definitely new. Right now we close the popup when the web view blurs or when the web view is clicked. We don't close when we click on the window title bar or menu bar. We don't close the popup even when we drag the window around. This becomes an apparent issue when we use NSWindow because the popup window remains in place when we drag and move the main window around. There seems to be no way to get events from outside the window. i found this trick in bookmark_bar_controller.mm. The bookmark bar menu had the same problem. https://codereview.chromium.org/11498008/diff/1/chrome/browser/ui/cocoa/tab_c... chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:106: defer:NO]; On 2012/12/10 20:52:06, Avi wrote: > Is there any possible way to move this into content? This looks like boilerplate > that every embedder will need. This was the difficult part. RenderWidgetPopupWindow needed access to BrowserCrApplication to hook the events so I figured it has to be passed from the chrome layer to content layer. Is there a better way?
On 2012/12/10 21:56:47, Avi wrote: > In addition, there's already lots of machinery in RWHV to close it when it has a > click outside, look in resignFirstResponder. How does this interact? Can we > reuse that code? If not, can we remove it in favor of the new code rather than > leaving it in? Right now resignFirstResponder is only called when the focus changes to something else like the omnibar. I will reuse this code in my next patch.
I'll double-check with Mark, but I'd much rather move more event hooking to content.
https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_guest.h (right): https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_guest.h:62: WebContentsViewDelegate* delegate = NULL) OVERRIDE; On 2012/12/10 20:52:06, Avi wrote: > I don't believe we allow default arguments like this. Done. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:61: #include "content/public/browser/web_contents_view_delegate.h" On 2012/12/10 20:52:06, Avi wrote: > alphabetical Done. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:151: On 2012/12/10 20:52:06, Avi wrote: > stray return Done. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:351: pos.width(), pos.height()) display:YES]; On 2012/12/10 20:52:06, Avi wrote: > Wrap "display:YES" to the next line; it gets lost here. Done. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:355: [cocoa_view_ setCanBeKeyView:YES]; On 2012/12/10 20:52:06, Avi wrote: > It seems silly to me to call -setCanBeKeyView above on line 340 and then re-set > it here. Pick one. Done. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:358: [[popup_window_ contentView] addSubview:cocoa_view_]; On 2012/12/10 20:52:06, Avi wrote: > You may want to set the bindings on the cocoa_view_ to bind its size to the > superview. This will help later... Done. https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:479: display:YES]; On 2012/12/10 20:52:06, Avi wrote: > This is wrong because rect.size() is in view coordinates, yet you use it here > for both view and screen coordinate systems. > > My advice is to bind the cocoa_view_ to its superview (as I advised above), and > here convert the size to global coordinates and resize the popup window. The > view, having been bound to its superview, will follow suit and resize too. When will a view size and a screen size be different? Is it for hi-dpi screens? Could you tell me which method to use?
https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_view_mac.mm:479: display:YES]; On 2012/12/11 05:09:14, keishi1 wrote: > When will a view size and a screen size be different? Is it for hi-dpi screens? > Could you tell me which method to use? Yes, the primary time they differ is on hi-dpi screens. In general you use -[NSView convert(type): toView:] to convert from one view's coordinate system to another view's. Please study up on how each view has its own coordinate system. For here, because you have a view in a window, and you always want to have the view be the same size as the window, please bind them. That way, you can set the window size, and the view will automatically resize with it.
On 2012/12/11 05:14:20, Avi wrote: > https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/11498008/diff/1/content/browser/renderer_host... > content/browser/renderer_host/render_widget_host_view_mac.mm:479: display:YES]; > On 2012/12/11 05:09:14, keishi1 wrote: > > When will a view size and a screen size be different? Is it for hi-dpi > screens? > > Could you tell me which method to use? > > Yes, the primary time they differ is on hi-dpi screens. > > In general you use -[NSView convert(type): toView:] to convert from one view's > coordinate system to another view's. Please study up on how each view has its > own coordinate system. I did [cocoa_view_ convertSize:size toView:nil]. > For here, because you have a view in a window, and you always want to have the > view be the same size as the window, please bind them. That way, you can set the > window size, and the view will automatically resize with it. I did [cocoa_view_ setAutoresizingMask:NSViewWidthSizable | NSViewHeightSizable]; and just resize the popup window. I created EventHookApplication in content/public and made BrowserCrApplication inherit it.
I like the changes you made in RWHV. The concerns that I have are the moving of the event hook stuff to content. Yes, we need to, but I'm not the expert in that area. Mark, can you take a look at that? We're trying to find a good place where we can put it so that any embedder gets this for free. https://codereview.chromium.org/11498008/diff/18001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/18001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:552: display:YES]; Yes, I like this. https://codereview.chromium.org/11498008/diff/18001/content/port/browser/rend... File content/port/browser/render_widget_host_view_port.h (right): https://codereview.chromium.org/11498008/diff/18001/content/port/browser/rend... content/port/browser/render_widget_host_view_port.h:46: class WebContentsViewDelegate; Is this needed?
I think it’s fine to consolidate the event hook stuff in the way that was done here. I’d consider naming it with a Cr prefix because this is interfacey stuff that embedders will need to touch, and that’s how people namespace their things in Objective-C.
keishi, have you verified that this works in hidpi mode?
I talked to John, and content/public isn't for concrete implementations. Mark, would it be OK to put the base NSApp class in ui/base? (Probably ui/base/cocoa.)
I don’t personally have any problem with that, but you should talk to some ui OWNERs. Nico, maybe?
On 2012/12/11 22:06:29, Avi wrote: > keishi, have you verified that this works in hidpi mode? Yes, I have verified that it works in hidpi mode. Avi, thanks for talking with everyone. I have moved the files to ui/base/cocoa.
https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:60: #include "ui/gfx/native_widget_types.h" in alphabetical order https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:206: } You don't need {} for a one-liner. I'm still curious about this. For a popup, the view has closeOnDeactivate_ == YES, which in its -resignFirstResponder: should cause it to kill itself on deactivation. Why is this event hooking necessary? In addition, what seems to me to be necessary (and what isn't here) is handling scroll events outside the window. Currently, scroll events in the parent RWHV kill all child RWHVs. Since we're using child windows, this no longer happens. See my comment below. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:211: - (void)begunTracking:(NSNotification *)notification { No space before *. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:216: - (void)startObservingClick { -startObservingClicks is more grammatical. (And -stopObservingClicks too.) https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:221: NSNotificationCenter *nc = [NSNotificationCenter defaultCenter]; Space after, not before *. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:408: [parent_host_view->GetNativeView() addSubview:cocoa_view_]; This line doesn't make sense any more. If we're installing cocoa_view_ in its own window, why do we make it a subview of the parent only to immediately remove it below? https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:432: object:popup_window_]; What is the case that you are trying to catch here? The window is borderless, so you should be the only one closing it. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:2188: [self cancelChildPopups]; How does this code work? As I noted above, it used to be the job of the root RWHV, when receiving a scroll event, to kill any children. This code won't function now, because child RWHVs are now in their own windows, so in -cancelChildPopups nothing will be found. If this code is useless, remove it.
https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:60: #include "ui/gfx/native_widget_types.h" On 2012/12/12 15:59:49, Avi wrote: > in alphabetical order Didn't need this. Removed. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:206: } On 2012/12/12 15:59:49, Avi wrote: > You don't need {} for a one-liner. > > I'm still curious about this. For a popup, the view has closeOnDeactivate_ == > YES, which in its -resignFirstResponder: should cause it to kill itself on > deactivation. Why is this event hooking necessary? resignFirstResponder isn't enough. For example it isn't called when dragging the window title bar(because it doesn't become the first responder). It is also not called when clicking on windows for other applications. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:211: - (void)begunTracking:(NSNotification *)notification { On 2012/12/12 15:59:49, Avi wrote: > No space before *. Done. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:216: - (void)startObservingClick { On 2012/12/12 15:59:49, Avi wrote: > -startObservingClicks is more grammatical. (And -stopObservingClicks too.) Done. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:221: NSNotificationCenter *nc = [NSNotificationCenter defaultCenter]; On 2012/12/12 15:59:49, Avi wrote: > Space after, not before *. Done. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:408: [parent_host_view->GetNativeView() addSubview:cocoa_view_]; On 2012/12/12 15:59:49, Avi wrote: > This line doesn't make sense any more. If we're installing cocoa_view_ in its > own window, why do we make it a subview of the parent only to immediately remove > it below? Removed. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:432: object:popup_window_]; On 2012/12/12 15:59:49, Avi wrote: > What is the case that you are trying to catch here? The window is borderless, so > you should be the only one closing it. When a click is observed RenderWidgetPopupWindow will close itself. cocoa_view_ will get notified of this and kill the RenderWidgetHostViewMac. https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:2188: [self cancelChildPopups]; On 2012/12/12 15:59:49, Avi wrote: > How does this code work? > > As I noted above, it used to be the job of the root RWHV, when receiving a > scroll event, to kill any children. This code won't function now, because child > RWHVs are now in their own windows, so in -cancelChildPopups nothing will be > found. > > If this code is useless, remove it. You're right. We can't do this if we use NSWindow. It turns out we want to close popups on mouse when event for all platforms so I will change WebKit::WebViewImpl to do it. +bool WebViewImpl::handleMouseWheel(Frame& mainFrame, const WebMouseWheelEvent& event) +{ + hidePopups(); + return PageWidgetEventHandler::handleMouseWheel(mainFrame, event); +} + https://codereview.chromium.org/11498008/diff/30001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:3438: [self resignFirstResponder]; Changing this line because cocoa_view_ for a popup doesn't become first responder and we don't have text fields in popups.
One last set of comments. https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:204: } You could fix WebKit to close popups on scroll events, or you could put it here... https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:805: [cocoa_view_ autorelease]; Is it necessary to remove the view from the window if we're closing the window?
https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:204: } On 2012/12/13 16:04:01, Avi wrote: > You could fix WebKit to close popups on scroll events, or you could put it > here... Hiding popups on scroll wheel event is a webview specific behavior (for example scroll wheel doesn't close Cocoa popup menu) so I think its better to have it in WebKit. https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:805: [cocoa_view_ autorelease]; On 2012/12/13 16:04:01, Avi wrote: > Is it necessary to remove the view from the window if we're closing the window? I think cocoa_view_ is used for non popup widgets like the renderview and so we need to keep this.
https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:790: renderWidgetHostViewMac]->ShutdownHost(); All the RenderWidgetHostViewCocoa objects used as popups are in their own windows now, so this if() is now useless, yes? https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:805: [cocoa_view_ autorelease]; Ah, yes, this is the general case Destroy.
On 2012/12/14 04:08:35, Avi wrote: > https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/11498008/diff/32001/content/browser/renderer_... > content/browser/renderer_host/render_widget_host_view_mac.mm:790: > renderWidgetHostViewMac]->ShutdownHost(); > All the RenderWidgetHostViewCocoa objects used as popups are in their own > windows now, so this if() is now useless, yes? Yes looks like it. Removed.
lgtm with the last change https://codereview.chromium.org/11498008/diff/38001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/38001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:43: #include "content/public/browser/web_contents_view_delegate.h" I don't think this is needed any more
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/11498008/40001
Presubmit check for 11498008-40001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
ui
Presubmit checks took 3.7s to calculate.
On 2012/12/14 04:42:15, Avi wrote: > lgtm with the last change Thanks Avi. Nico, could you review the ui/base/cocoa changes?
If it looks good to Avi, lgtm I suppose. Feel free to land. For my education: Why do we need this hooks business, why can't we use a nested event loop for the popup? https://codereview.chromium.org/11498008/diff/40001/ui/base/cocoa/event_hook_... File ui/base/cocoa/event_hook_application.h (right): https://codereview.chromium.org/11498008/diff/40001/ui/base/cocoa/event_hook_... ui/base/cocoa/event_hook_application.h:8: #ifdef __OBJC__ nit: we prefer #if defined https://codereview.chromium.org/11498008/diff/40001/ui/base/cocoa/event_hook_... ui/base/cocoa/event_hook_application.h:12: #include <vector> unused https://codereview.chromium.org/11498008/diff/40001/ui/base/cocoa/event_hook_... ui/base/cocoa/event_hook_application.h:21: @interface CrEventHookApplication : NSApplication { Add a class comment. https://codereview.chromium.org/11498008/diff/40001/ui/base/cocoa/event_hook_... ui/base/cocoa/event_hook_application.h:40: #endif // defined(__OBJC__)
On 2012/12/14 19:00:46, Nico wrote: > For my education: Why do we need this hooks business, why can't we use a nested > event loop for the popup? I don't know how a nested event loop is done (I would like to know), but the comments say something about it. // This is not a good alternative to a nested event loop. It should // be used only when normal event logic and notification breaks down // (e.g. when clicking outside a canBecomeKey:NO window to "switch // context" out of it). I think this example case applies to render widget popups. The popup window can't become key window because a blur event will be raised in the main window and the popup will be hidden immediately after being shown.
On Sun, Dec 16, 2012 at 8:45 PM, <keishi@chromium.org> wrote: > On 2012/12/14 19:00:46, Nico wrote: > >> For my education: Why do we need this hooks business, why can't we use a >> > nested > >> event loop for the popup? >> > I don't know how a nested event loop is done (I would like to know), See for example dragShouldBeginFromMouseDown in chrome/browser/ui/cocoa/ draggable_button_mixin.mm, but there are various ways to implement this. > but the > comments say something about it. > // This is not a good alternative to a nested event loop. It should > // be used only when normal event logic and notification breaks down > // (e.g. when clicking outside a canBecomeKey:NO window to "switch > // context" out of it). > I think this example case applies to render widget popups. The popup window > can't become key window because a blur event will be raised in the main > window > and the popup will be hidden immediately after being shown. > How does arrow up/down in that popup work then? (Also, the rwhv could suppress forwarding blurs if it knows that the blur is for opening a popup.) Avi, can you say anything about this? > > > https://codereview.chromium.**org/11498008/<https://codereview.chromium.org/1... >
I know just about nothing about nested event loops. I'm just for whatever works...
On 2012/12/17 06:04:26, Nico wrote: > On Sun, Dec 16, 2012 at 8:45 PM, <mailto:keishi@chromium.org> wrote: > > How does arrow up/down in that popup work then? Right now key events are received by the main web view and forwarded in WebKit::WebViewImpl::handleKeyEvent()
> (Also, the rwhv could suppress forwarding blurs if it knows that the blur
> is for opening a popup.)
I've been trying to implement this for a day but it is quite tricky. We need to
handle blurring while a popup is open, and BrowserWindowController is observing
the didResignKeyWindow notification but it is in chrome so it knows nothing
about render widget popups.
Render widget popups are used for autofill popups too and the user needs to be
able to type into the textfield while the popups is open. I feel the textfield
should have priority over the popup in handling key events so I think we
shouldn't be making the popup window a key window.
I ran the code below in F-script and it seems like NSComboBoxWindow isn't
becoming a key window too.
(((NSComboBoxWindow alloc) initWithContentRect:NSZeroRect
styleMask:NSBorderlessWindowMask backing:NSBackingStoreBuffered defer:YES)
canBecomeKeyWindow)
On 2012/12/18 08:51:07, keishi1 wrote: > > (Also, the rwhv could suppress forwarding blurs if it knows that the blur > > is for opening a popup.) > > I've been trying to implement this for a day but it is quite tricky. We need to > handle blurring while a popup is open, and BrowserWindowController is observing > the didResignKeyWindow notification but it is in chrome so it knows nothing > about render widget popups. > > Render widget popups are used for autofill popups too and the user needs to be > able to type into the textfield while the popups is open. I feel the textfield > should have priority over the popup in handling key events so I think we > shouldn't be making the popup window a key window. > > I ran the code below in F-script and it seems like NSComboBoxWindow isn't > becoming a key window too. > > (((NSComboBoxWindow alloc) initWithContentRect:NSZeroRect > styleMask:NSBorderlessWindowMask backing:NSBackingStoreBuffered defer:YES) > canBecomeKeyWindow) Ok. Thanks for checking! Can you add a short blurb to the CL description about why this uses event hooking instead of a nested event loop? With that, go ahead and land.
In crbug.com/166680, Nico wonders if you can do this kind of work with +[NSEvent addLocalMonitorForEventsMatchingMask:handler:]. If you can, this would simplify this CL enormously.
On 2012/12/19 17:48:32, Avi wrote: > In crbug.com/166680, Nico wonders if you can do this kind of work with +[NSEvent > addLocalMonitorForEventsMatchingMask:handler:]. If you can, this would simplify > this CL enormously. We switched the bookmark bar code to the local monitor yesterday and it worked. I'm planning on removing event hooks entirely in the next few days; please make sure that you can switch this CL to local monitors so that I don't break you.
On 2012/12/20 19:15:14, Avi wrote: > On 2012/12/19 17:48:32, Avi wrote: > > In crbug.com/166680, Nico wonders if you can do this kind of work with > +[NSEvent > > addLocalMonitorForEventsMatchingMask:handler:]. If you can, this would > simplify > > this CL enormously. > > We switched the bookmark bar code to the local monitor yesterday and it worked. > I'm planning on removing event hooks entirely in the next few days; please make > sure that you can switch this CL to local monitors so that I don't break you. Thanks Avi!!! Changed to use local monitor.
LGTM with style nit. Thanks! I'm going to remove the event hook stuff now. https://codereview.chromium.org/11498008/diff/49001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/49001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:216: if ([event window] == self) http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Blocks "Code inside blocks should be indented four spaces." You need to bump the contents of this braced scope two more spaces over.
Nice! (Feel free to land when you want.) https://codereview.chromium.org/11498008/diff/49001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/11498008/diff/49001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:208: - (void)begunTracking:(NSNotification*)notification { grammar nit: I think it's either "beganTracking" or "hasBegunTracking". ("didBeginTracking" would work too I think.) https://codereview.chromium.org/11498008/diff/49001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_mac.mm:224: NSNotificationCenter* nc = [NSNotificationCenter defaultCenter]; nit: the style guide says "no abbreviations". s/nc/center/ for example.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/11498008/56001
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/11498008/56001
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/11498008/56001
Message was sent while issue was closed.
Change committed as 175488 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
