Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005953002:
[mac][views] Notify web_contents focused. (Closed)
Description[mac][views] Notify web_contents focused.
This fix allow to FocusManager know about web_contents was focused.
It also fix a lot of test for mac_views_browser by example:
BrowserFocusTest.FocusOnReload,
BrowserFocusTest.InterstitialFocus ...
BUG=610561, 21875
Committed: https://crrev.com/4d67971369b316f1d7e4e0330412a3d94b4418ba
Cr-Commit-Position: refs/heads/master@{#397353}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 5
Patch Set 4 : git log -2 #
Messages
Total messages: 31 (11 generated)
Description was changed from ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG= ========== to ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG= ==========
yamaxim@yandex-team.ru changed reviewers: + erikchen@chromium.org
yamaxim@yandex-team.ru changed reviewers: + tapted@chromium.org
tapted@chromium.org changed reviewers: + avi@chromium.org
+avi is the guru here (and this might fix http://crbug.com/610561 as well, which is behind a chrome://flag in regular builds - not just mac_views_browser builds - with the thing below, that's two things to add to BUG=) https://codereview.chromium.org/2005953002/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_view_mac.mm:186: web_contents_->NotifyWebContentsFocused(); I don't think this is the right place for it. Aura does it in `GotFocus` but there's a comment there for Mac: void WebContentsViewMac::GotFocus() { // This is only used in the views FocusManager stuff but it bleeds through // all subclasses. http://crbug.com/21875 } So yup - nothing on Mac calls NotifyWebContentsFocused() (except embedded fullscreen widgets?). But that's fine because there's exactly one observer: views::WebView, which only exists in MacViewsBrowser. So.. IMO it's OK just to use the same GotFocus() implementation as WebContentsViewAura to hook this up. I don't think there's a significant downside for the Cocoa browser. But avi@ is the person to ask :) I guess you could set BUG+=21875 for this, since I guess that would be fixed by this too.
Description was changed from ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG= ========== to ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG=610561, 21875 ==========
https://codereview.chromium.org/2005953002/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_view_mac.mm:186: web_contents_->NotifyWebContentsFocused(); On 2016/05/27 11:50:37, tapted wrote: > I don't think this is the right place for it. Aura does it in `GotFocus` but > there's a comment there for Mac: > > void WebContentsViewMac::GotFocus() { > // This is only used in the views FocusManager stuff but it bleeds through > // all subclasses. http://crbug.com/21875 > } > > So yup - nothing on Mac calls NotifyWebContentsFocused() (except embedded > fullscreen widgets?). But that's fine because there's exactly one observer: > views::WebView, which only exists in MacViewsBrowser. > > So.. IMO it's OK just to use the same GotFocus() implementation as > WebContentsViewAura to hook this up. I don't think there's a significant > downside for the Cocoa browser. But avi@ is the person to ask :) > > I guess you could set BUG+=21875 for this, since I guess that would be fixed by > this too. Agreed. I'm trusting in the Views people that plumbing GotFocus into WebContents was the correct decision, but if it was, then let's mirror that behavior here. Trying to keep things being done in the same way on all the platforms is very important.
https://codereview.chromium.org/2005953002/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_view_mac.mm:186: web_contents_->NotifyWebContentsFocused(); On 2016/05/27 11:50:37, tapted wrote: > I don't think this is the right place for it. Aura does it in `GotFocus` but > there's a comment there for Mac: > > void WebContentsViewMac::GotFocus() { > // This is only used in the views FocusManager stuff but it bleeds through > // all subclasses. http://crbug.com/21875 > } > > So yup - nothing on Mac calls NotifyWebContentsFocused() (except embedded > fullscreen widgets?). But that's fine because there's exactly one observer: > views::WebView, which only exists in MacViewsBrowser. > > So.. IMO it's OK just to use the same GotFocus() implementation as > WebContentsViewAura to hook this up. I don't think there's a significant > downside for the Cocoa browser. But avi@ is the person to ask :) > > I guess you could set BUG+=21875 for this, since I guess that would be fixed by > this too. I wrap it with GotFocus, if you can say a better place for call GotFocus, I would like to move it.
https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_mac.mm:186: GotFocus(); Should this be necessary? RenderViewHostImpl::RenderWidgetGotFocus() calls it, so shouldn't that work for us too on the Mac?
https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_mac.mm:186: GotFocus(); On 2016/05/30 16:13:33, Avi wrote: > Should this be necessary? > > RenderViewHostImpl::RenderWidgetGotFocus() calls it, so shouldn't that work for > us too on the Mac? It's not called for mac_views because WebContentsViewMac::Focus() differs from WebContentsViewAura::Focus() that call rwhv->Focus
On 2016/05/30 16:57:21, yamaxim wrote: > https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... > File content/browser/web_contents/web_contents_view_mac.mm (right): > > https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... > content/browser/web_contents/web_contents_view_mac.mm:186: GotFocus(); > On 2016/05/30 16:13:33, Avi wrote: > > Should this be necessary? > > > > RenderViewHostImpl::RenderWidgetGotFocus() calls it, so shouldn't that work > for > > us too on the Mac? > > It's not called for mac_views because WebContentsViewMac::Focus() differs from > WebContentsViewAura::Focus() that call rwhv->Focus Then I'm not sure what to recommend. I don't remember the intricacies of the Mac code in this area. Let me see if I can find a reviewer.
https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_mac.mm:186: GotFocus(); On 2016/05/30 16:57:21, yamaxim wrote: > On 2016/05/30 16:13:33, Avi wrote: > > Should this be necessary? > > > > RenderViewHostImpl::RenderWidgetGotFocus() calls it, so shouldn't that work > for > > us too on the Mac? > > It's not called for mac_views because WebContentsViewMac::Focus() differs from > WebContentsViewAura::Focus() that call rwhv->Focus So I think we want this to be triggered by -[RenderWidgetHostViewCocoa becomeFirstResponder]. That does: - (BOOL)becomeFirstResponder { if (!renderWidgetHostView_->render_widget_host_) return NO; renderWidgetHostView_->render_widget_host_->Focus(); / * etc */ I think we just need to change that call to Focus() to be GotFocus() instead. Since the first thing RenderWidgetHostImpl::GotFocus() does is to call Focus()
On 2016/05/31 12:01:50, tapted wrote: > https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... > File content/browser/web_contents/web_contents_view_mac.mm (right): > > https://codereview.chromium.org/2005953002/diff/20001/content/browser/web_con... > content/browser/web_contents/web_contents_view_mac.mm:186: GotFocus(); > On 2016/05/30 16:57:21, yamaxim wrote: > > On 2016/05/30 16:13:33, Avi wrote: > > > Should this be necessary? > > > > > > RenderViewHostImpl::RenderWidgetGotFocus() calls it, so shouldn't that work > > for > > > us too on the Mac? > > > > It's not called for mac_views because WebContentsViewMac::Focus() differs from > > WebContentsViewAura::Focus() that call rwhv->Focus > > So I think we want this to be triggered by -[RenderWidgetHostViewCocoa > becomeFirstResponder]. > > That does: > > - (BOOL)becomeFirstResponder { > if (!renderWidgetHostView_->render_widget_host_) > return NO; > > renderWidgetHostView_->render_widget_host_->Focus(); > / * etc */ > > > I think we just need to change that call to Focus() to be GotFocus() instead. > Since the first thing RenderWidgetHostImpl::GotFocus() does is to call Focus() Thanks, it's work. (done)
lgtm https://codereview.chromium.org/2005953002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_mac.mm:229: // This is only used in the views FocusManager stuff but it bleeds through this comment isn't needed any longer
and please wait for avi's lg too
https://codereview.chromium.org/2005953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2729: renderWidgetHostView_->render_widget_host_->GotFocus(); Was the call to Focus that used to be here wrong, then? https://codereview.chromium.org/2005953002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_mac.mm:229: // This is only used in the views FocusManager stuff but it bleeds through On 2016/06/01 11:34:47, tapted wrote: > this comment isn't needed any longer Agreed.
https://codereview.chromium.org/2005953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2729: renderWidgetHostView_->render_widget_host_->GotFocus(); On 2016/06/01 13:43:38, Avi wrote: > Was the call to Focus that used to be here wrong, then? yes Now: WebContentsViewMac::Focus() then RenderWidgetHostViewCocoa becomeFirstResponder then WebContentsViewMac::GotFocus so i think it's right https://codereview.chromium.org/2005953002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2005953002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_mac.mm:229: // This is only used in the views FocusManager stuff but it bleeds through On 2016/06/01 13:43:38, Avi wrote: > On 2016/06/01 11:34:47, tapted wrote: > > this comment isn't needed any longer > > Agreed. Deleted
lgtm
The CQ bit was checked by yamaxim@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005953002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2005953002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yamaxim@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2005953002/#ps60001 (title: "git log -2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005953002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2005953002/60001
Message was sent while issue was closed.
Description was changed from ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG=610561, 21875 ========== to ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG=610561, 21875 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG=610561, 21875 ========== to ========== [mac][views] Notify web_contents focused. This fix allow to FocusManager know about web_contents was focused. It also fix a lot of test for mac_views_browser by example: BrowserFocusTest.FocusOnReload, BrowserFocusTest.InterstitialFocus ... BUG=610561, 21875 Committed: https://crrev.com/4d67971369b316f1d7e4e0330412a3d94b4418ba Cr-Commit-Position: refs/heads/master@{#397353} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4d67971369b316f1d7e4e0330412a3d94b4418ba Cr-Commit-Position: refs/heads/master@{#397353} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
