Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709433002:
Add HeadlessFocusClient to fix document.hasFocus() issues. (Closed)
|
Created:
3 years, 10 months ago by irisu Modified:
3 years, 9 months ago Reviewers:
Eric Seckler CC:
chromium-reviews, Sami Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd HeadlessFocusClient to fix document.hasFocus() issues.
BUG=685139
Review-Url: https://codereview.chromium.org/2709433002
Cr-Commit-Position: refs/heads/master@{#453584}
Committed: https://chromium.googlesource.com/chromium/src/+/559af091bb5d7f598e7259a13fe219f5f4a184a2
Patch Set 1 #Patch Set 2 : Add test and use Delegate to focus RenderWidgetHost #
Total comments: 6
Patch Set 3 : Set FocusClient and add test #
Total comments: 10
Patch Set 4 : Fix nits #Patch Set 5 : Add HeadlessFocusClient #
Total comments: 2
Patch Set 6 : Only focus one window #
Total comments: 6
Patch Set 7 : Remove unnecessary imports #
Messages
Total messages: 33 (16 generated)
irisu@chromium.org changed reviewers: + eseckler@chromium.org
Hey Eric, I'm getting this weird issue where if I run the test with: cr run headless_browsertests --gtest_filter=HeadlessWebContentsTest.Focus --single-process There's a fatal error: [138922:138922:0221/144911.147681:445215356239:FATAL:lock_impl_posix.cc(65)] Check failed: rv == 0 (22 vs. 0). Invalid argument I'm having the same issue with the Navigation and WindowOpen tests. However, if I just run: cr run headless_browsertests then all tests pass. Any ideas?
On 2017/02/21 04:00:38, irisu wrote: > Hey Eric, I'm getting this weird issue where if I run the test with: > cr run headless_browsertests --gtest_filter=HeadlessWebContentsTest.Focus > --single-process > There's a fatal error: > [138922:138922:0221/144911.147681:445215356239:FATAL:lock_impl_posix.cc(65)] > Check failed: rv == 0 (22 vs. 0). Invalid argument > I'm having the same issue with the Navigation and WindowOpen tests. > > However, if I just run: cr run headless_browsertests > then all tests pass. Any ideas? Hmm, sounds like --single-process mode isn't supported for the headless browsertests, or headless in general. Not sure what's causing that, but it could well be that we haven't tried to run single-process mode in a while :P Interestingly, I'm getting a different stacktrace. Filed a bug about that -> https://crbug.com/694424 But I guess that's really only important if you wanted to use gdb? We mostly just debug using printf -.- https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:103: contents->GetTopLevelRenderWidgetHostView()->GetRenderWidgetHost()->Focus(); Hm, the content shell does: contents->GetRenderViewHost()->GetWidget()->Focus(); Which probably amounts to the same, but I'm not sure. Do you have a rationale for calling focus() on the top-level RWH rather than the current view's RWH? :) https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:190: web_contents_delegate_->ActivateContents(web_contents_.get()); Ah, okay. I thought ActivateContents would be called when required by the content layer. Seems that's not true :) I looked into why the above web_contents_->Focus() call doesn't seem to work. It ends up in RenderWidgetHostViewAura::Focus() here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... Turns out, that doesn't have any effect because we currently don't set a aura::client::FocusClient. Maybe the better solution is to set one. The content shell uses a TestFocusClient, which is probably also fine for us, at least for the moment: https://cs.chromium.org/chromium/src/content/shell/browser/shell_platform_dat... We can probably set it when we initialize the WindowTreeHost (on its root window) in HeadlessBrowserImpl::PlatformCreateWindow(). The change to ActivateContents() is probably useful nevertheless, we just don't have to call it from here then.
https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:103: contents->GetTopLevelRenderWidgetHostView()->GetRenderWidgetHost()->Focus(); On 2017/02/21 09:28:02, Eric Seckler wrote: > Hm, the content shell does: > contents->GetRenderViewHost()->GetWidget()->Focus(); > > Which probably amounts to the same, but I'm not sure. Do you have a rationale > for calling focus() on the top-level RWH rather than the current view's RWH? :) No, I don't. Done. https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:190: web_contents_delegate_->ActivateContents(web_contents_.get()); On 2017/02/21 09:28:02, Eric Seckler wrote: > Ah, okay. I thought ActivateContents would be called when required by the > content layer. Seems that's not true :) > > I looked into why the above web_contents_->Focus() call doesn't seem to work. It > ends up in RenderWidgetHostViewAura::Focus() here: > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > Turns out, that doesn't have any effect because we currently don't set a > aura::client::FocusClient. Maybe the better solution is to set one. The content > shell uses a TestFocusClient, which is probably also fine for us, at least for > the moment: > > https://cs.chromium.org/chromium/src/content/shell/browser/shell_platform_dat... > > We can probably set it when we initialize the WindowTreeHost (on its root > window) in HeadlessBrowserImpl::PlatformCreateWindow(). > > The change to ActivateContents() is probably useful nevertheless, we just don't > have to call it from here then. Done. I'm getting a presubmit warning for using TestFocusClient in production, but I'm assuming that's fine. "You might be calling functions intended only for testing from production code. It is OK to ignore this warning if you know what you are doing, as the heuristics used to detect the situation are not perfect. The commit queue will not block on this warning."
lgtm % nits. Thanks, great work! :) https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:190: web_contents_delegate_->ActivateContents(web_contents_.get()); On 2017/02/22 05:34:35, irisu wrote: > On 2017/02/21 09:28:02, Eric Seckler wrote: > > Ah, okay. I thought ActivateContents would be called when required by the > > content layer. Seems that's not true :) > > > > I looked into why the above web_contents_->Focus() call doesn't seem to work. > It > > ends up in RenderWidgetHostViewAura::Focus() here: > > > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > > > Turns out, that doesn't have any effect because we currently don't set a > > aura::client::FocusClient. Maybe the better solution is to set one. The > content > > shell uses a TestFocusClient, which is probably also fine for us, at least for > > the moment: > > > > > https://cs.chromium.org/chromium/src/content/shell/browser/shell_platform_dat... > > > > We can probably set it when we initialize the WindowTreeHost (on its root > > window) in HeadlessBrowserImpl::PlatformCreateWindow(). > > > > The change to ActivateContents() is probably useful nevertheless, we just > don't > > have to call it from here then. > > Done. I'm getting a presubmit warning for using TestFocusClient in production, > but I'm assuming that's fine. > "You might be calling functions intended only for testing from > production code. It is OK to ignore this warning if you know what > you are doing, as the heuristics used to detect the situation are > not perfect. The commit queue will not block on this warning." Yeah, it's a reasonable warning, but given that the TestFocusClient is also used by content_shell, I'd consider ignoring it appropriate in this case :) https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_browser_impl.cc (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_browser_impl.cc:53: : focus_client_(new aura::test::TestFocusClient()), Let's get rid of the init here, since you reset it during PlatformCreateWindow(). (Otherwise, it'd need to be inside an "#if defined(USE_AURA)".) https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_browser_impl.h (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_browser_impl.h:24: namespace aura { nit: let's move this into the "#if defined(USE_AURA)" section above. https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_browser_impl.h:91: nit: move blank line after "#endif" https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:25: #include "content/public/browser/render_widget_host_view.h" nit: do we still need this include? https://codereview.chromium.org/2709433002/diff/40001/headless/lib/headless_w... File headless/lib/headless_web_contents_browsertest.cc (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/headless_w... headless/lib/headless_web_contents_browsertest.cc:89: EXPECT_TRUE(EvaluateScript(web_contents, "document.hasFocus()") nit: Let's add a todo here - focus of two web contents should eventually be independent of each other (for the purposes of isolation). Then, both web_contents and web_contents2 should be focused at this point. (Side note - in that future, we may also want a way to explicitly relinquish focus of a WebContents.)
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:190: web_contents_delegate_->ActivateContents(web_contents_.get()); On 2017/02/22 09:00:03, Eric Seckler wrote: > On 2017/02/22 05:34:35, irisu wrote: > > On 2017/02/21 09:28:02, Eric Seckler wrote: > > > Ah, okay. I thought ActivateContents would be called when required by the > > > content layer. Seems that's not true :) > > > > > > I looked into why the above web_contents_->Focus() call doesn't seem to > work. > > It > > > ends up in RenderWidgetHostViewAura::Focus() here: > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > > > > > Turns out, that doesn't have any effect because we currently don't set a > > > aura::client::FocusClient. Maybe the better solution is to set one. The > > content > > > shell uses a TestFocusClient, which is probably also fine for us, at least > for > > > the moment: > > > > > > > > > https://cs.chromium.org/chromium/src/content/shell/browser/shell_platform_dat... > > > > > > We can probably set it when we initialize the WindowTreeHost (on its root > > > window) in HeadlessBrowserImpl::PlatformCreateWindow(). > > > > > > The change to ActivateContents() is probably useful nevertheless, we just > > don't > > > have to call it from here then. > > > > Done. I'm getting a presubmit warning for using TestFocusClient in production, > > but I'm assuming that's fine. > > "You might be calling functions intended only for testing from > > production code. It is OK to ignore this warning if you know what > > you are doing, as the heuristics used to detect the situation are > > not perfect. The commit queue will not block on this warning." > > Yeah, it's a reasonable warning, but given that the TestFocusClient is also used > by content_shell, I'd consider ignoring it appropriate in this case :) Seems like we'll also have to add a build dependency on the test lib (//ui/aura:test_support): [4652/4655] LINK ./chrome FAILED: chrome [..] ../../headless/lib/browser/headless_browser_impl_aura.cc:35: error: undefined reference to 'aura::test::TestFocusClient::TestFocusClient()' The alternative would be to try and convince aura owners that TestFocusClient (maybe renamed to e.g. FakeFocusClient) should live in /ui/aura, and not the test support library. Or we just go ahead and build our own one now.
On 2017/02/22 10:29:00, Eric Seckler wrote: > https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... > File headless/lib/browser/headless_web_contents_impl.cc (right): > > https://codereview.chromium.org/2709433002/diff/20001/headless/lib/browser/he... > headless/lib/browser/headless_web_contents_impl.cc:190: > web_contents_delegate_->ActivateContents(web_contents_.get()); > On 2017/02/22 09:00:03, Eric Seckler wrote: > > On 2017/02/22 05:34:35, irisu wrote: > > > On 2017/02/21 09:28:02, Eric Seckler wrote: > > > > Ah, okay. I thought ActivateContents would be called when required by the > > > > content layer. Seems that's not true :) > > > > > > > > I looked into why the above web_contents_->Focus() call doesn't seem to > > work. > > > It > > > > ends up in RenderWidgetHostViewAura::Focus() here: > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > > > > > > > Turns out, that doesn't have any effect because we currently don't set a > > > > aura::client::FocusClient. Maybe the better solution is to set one. The > > > content > > > > shell uses a TestFocusClient, which is probably also fine for us, at least > > for > > > > the moment: > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/shell/browser/shell_platform_dat... > > > > > > > > We can probably set it when we initialize the WindowTreeHost (on its root > > > > window) in HeadlessBrowserImpl::PlatformCreateWindow(). > > > > > > > > The change to ActivateContents() is probably useful nevertheless, we just > > > don't > > > > have to call it from here then. > > > > > > Done. I'm getting a presubmit warning for using TestFocusClient in > production, > > > but I'm assuming that's fine. > > > "You might be calling functions intended only for testing from > > > production code. It is OK to ignore this warning if you know what > > > you are doing, as the heuristics used to detect the situation are > > > not perfect. The commit queue will not block on this warning." > > > > Yeah, it's a reasonable warning, but given that the TestFocusClient is also > used > > by content_shell, I'd consider ignoring it appropriate in this case :) > > Seems like we'll also have to add a build dependency on the test lib > (//ui/aura:test_support): > > [4652/4655] LINK ./chrome > FAILED: chrome [..] > ../../headless/lib/browser/headless_browser_impl_aura.cc:35: error: undefined > reference to 'aura::test::TestFocusClient::TestFocusClient()' > > The alternative would be to try and convince aura owners that TestFocusClient > (maybe renamed to e.g. FakeFocusClient) should live in /ui/aura, and not the > test support library. Or we just go ahead and build our own one now. If we add //ui/aura:test_support, it looks like we'll have to set testonly = true for heaps of build targets, including "chrome_initial" which sounds like shouldn't be done. It seems like moving TestFocusClient to being FakeFocusClient would be the neater option, wdyt?
https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_browser_impl.cc (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_browser_impl.cc:53: : focus_client_(new aura::test::TestFocusClient()), On 2017/02/22 09:00:04, Eric Seckler wrote: > Let's get rid of the init here, since you reset it during > PlatformCreateWindow(). (Otherwise, it'd need to be inside an "#if > defined(USE_AURA)".) Done. https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_browser_impl.h (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_browser_impl.h:24: namespace aura { On 2017/02/22 09:00:04, Eric Seckler wrote: > nit: let's move this into the "#if defined(USE_AURA)" section above. Done. https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_browser_impl.h:91: On 2017/02/22 09:00:04, Eric Seckler wrote: > nit: move blank line after "#endif" Done. https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:25: #include "content/public/browser/render_widget_host_view.h" On 2017/02/22 09:00:04, Eric Seckler wrote: > nit: do we still need this include? Done. https://codereview.chromium.org/2709433002/diff/40001/headless/lib/headless_w... File headless/lib/headless_web_contents_browsertest.cc (right): https://codereview.chromium.org/2709433002/diff/40001/headless/lib/headless_w... headless/lib/headless_web_contents_browsertest.cc:89: EXPECT_TRUE(EvaluateScript(web_contents, "document.hasFocus()") On 2017/02/22 09:00:04, Eric Seckler wrote: > nit: Let's add a todo here - focus of two web contents should eventually be > independent of each other (for the purposes of isolation). Then, both > web_contents and web_contents2 should be focused at this point. > > (Side note - in that future, we may also want a way to explicitly relinquish > focus of a WebContents.) Done.
On 2017/02/23 00:02:14, irisu wrote: > If we add //ui/aura:test_support, it looks like we'll have to set testonly = > true for heaps of build targets, including "chrome_initial" which sounds like > shouldn't be done. > It seems like moving TestFocusClient to being FakeFocusClient would be the > neater option, wdyt? Ah, I see. Yeah, we can't set testonly = true. The chrome build target depends on headless_lib :) Moving the TestFocusClient sounds fine, but you'll have to go through ui/aura owners for that. Maybe in a separate patch, since that's going to require changes in lots of places. If you want to avoid that, I'm also happy to introduce an even simpler HeadlessFocusClient. It probably wouldn't have to track which window is currently focused, but could just set multiple to be in focus at once. We only have aura::Windows that correspond to a WebContents (apart from the root window), and we don't care if all of them are in focus simultaneously - we actually want that.
Patchset #5 (id:80001) has been deleted
On 2017/02/23 09:26:08, Eric Seckler wrote: > On 2017/02/23 00:02:14, irisu wrote: > > If we add //ui/aura:test_support, it looks like we'll have to set testonly = > > true for heaps of build targets, including "chrome_initial" which sounds like > > shouldn't be done. > > It seems like moving TestFocusClient to being FakeFocusClient would be the > > neater option, wdyt? > > Ah, I see. Yeah, we can't set testonly = true. The chrome build target depends > on headless_lib :) > > Moving the TestFocusClient sounds fine, but you'll have to go through ui/aura > owners for that. Maybe in a separate patch, since that's going to require > changes in lots of places. > > If you want to avoid that, I'm also happy to introduce an even simpler > HeadlessFocusClient. It probably wouldn't have to track which window is > currently focused, but could just set multiple to be in focus at once. We only > have aura::Windows that correspond to a WebContents (apart from the root > window), and we don't care if all of them are in focus simultaneously - we > actually want that. Added HeadlessFocusClient
https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_focus_client.cc (right): https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_focus_client.cc:51: aura::Window* HeadlessFocusClient::GetFocusedWindow() { I'm not sure what we'd want to return here. Just returning one of the focused windows for now. :)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add focus for RenderWidgetHost in HeadlessWebContentsImpl::OpenURL. BUG=685139 ========== to ========== Add HeadlessFocusClient to fix document.hasFocus() issues. BUG=685139 ==========
https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... File headless/lib/browser/headless_focus_client.cc (right): https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... headless/lib/browser/headless_focus_client.cc:51: aura::Window* HeadlessFocusClient::GetFocusedWindow() { On 2017/02/24 04:48:31, irisu wrote: > I'm not sure what we'd want to return here. Just returning one of the focused > windows for now. :) Hmm, I see. Looks like aura doesn't really support multiple focused windows - GetFocusedWindow() is used for a variety of things, via others aura::Window::HasFocus(), even RWHVAura::HasFocus() relies on it :( Let's just track a single window for focus then. To make focus tab-independent, we'll have to move to multiple WindowTreeHosts afterall, I think. Sorry for pointing you in the wrong direction!
On 2017/02/27 08:39:38, Eric Seckler wrote: > https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... > File headless/lib/browser/headless_focus_client.cc (right): > > https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... > headless/lib/browser/headless_focus_client.cc:51: aura::Window* > HeadlessFocusClient::GetFocusedWindow() { > On 2017/02/24 04:48:31, irisu wrote: > > I'm not sure what we'd want to return here. Just returning one of the focused > > windows for now. :) > > Hmm, I see. Looks like aura doesn't really support multiple focused windows - > GetFocusedWindow() is used for a variety of things, via others > aura::Window::HasFocus(), even RWHVAura::HasFocus() relies on it :( > > Let's just track a single window for focus then. To make focus tab-independent, > we'll have to move to multiple WindowTreeHosts afterall, I think. Sorry for > pointing you in the wrong direction! That's all good. :) So later on we'd want a WindowTreeHost for each web contents - is that what you mean?
On 2017/02/28 00:08:41, irisu wrote: > On 2017/02/27 08:39:38, Eric Seckler wrote: > > > https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... > > File headless/lib/browser/headless_focus_client.cc (right): > > > > > https://codereview.chromium.org/2709433002/diff/100001/headless/lib/browser/h... > > headless/lib/browser/headless_focus_client.cc:51: aura::Window* > > HeadlessFocusClient::GetFocusedWindow() { > > On 2017/02/24 04:48:31, irisu wrote: > > > I'm not sure what we'd want to return here. Just returning one of the > focused > > > windows for now. :) > > > > Hmm, I see. Looks like aura doesn't really support multiple focused windows - > > GetFocusedWindow() is used for a variety of things, via others > > aura::Window::HasFocus(), even RWHVAura::HasFocus() relies on it :( > > > > Let's just track a single window for focus then. To make focus > tab-independent, > > we'll have to move to multiple WindowTreeHosts afterall, I think. Sorry for > > pointing you in the wrong direction! > > That's all good. :) > > So later on we'd want a WindowTreeHost for each web contents - is that what you > mean? Yep, exactly. Assuming that works, of course! I think that would separate individual tabs fundamentally :)
lgtm % nits Thank you! https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_browser_impl.cc (right): https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_browser_impl.cc:23: #include "ui/aura/test/test_focus_client.h" nit: unnecessary https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_browser_impl_aura.cc (right): https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_browser_impl_aura.cc:13: #include "ui/aura/test/test_focus_client.h" nit: unnecessary https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_focus_client.h (right): https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_focus_client.h:8: #include "base/compiler_specific.h" nit: unnecessary (I think)
Patchset #7 (id:140001) has been deleted
https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_browser_impl.cc (right): https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_browser_impl.cc:23: #include "ui/aura/test/test_focus_client.h" On 2017/02/28 07:56:44, Eric Seckler wrote: > nit: unnecessary Done. https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_browser_impl_aura.cc (right): https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_browser_impl_aura.cc:13: #include "ui/aura/test/test_focus_client.h" On 2017/02/28 07:56:44, Eric Seckler wrote: > nit: unnecessary Done. https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... File headless/lib/browser/headless_focus_client.h (right): https://codereview.chromium.org/2709433002/diff/120001/headless/lib/browser/h... headless/lib/browser/headless_focus_client.h:8: #include "base/compiler_specific.h" On 2017/02/28 07:56:44, Eric Seckler wrote: > nit: unnecessary (I think) Done.
The CQ bit was checked by irisu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org Link to the patchset: https://codereview.chromium.org/2709433002/#ps160001 (title: "Remove unnecessary imports")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488283106813710,
"parent_rev": "35275d0100464cc6f401e8307ae9dc2dc32d6cd8", "commit_rev":
"559af091bb5d7f598e7259a13fe219f5f4a184a2"}
Message was sent while issue was closed.
Description was changed from ========== Add HeadlessFocusClient to fix document.hasFocus() issues. BUG=685139 ========== to ========== Add HeadlessFocusClient to fix document.hasFocus() issues. BUG=685139 Review-Url: https://codereview.chromium.org/2709433002 Cr-Commit-Position: refs/heads/master@{#453584} Committed: https://chromium.googlesource.com/chromium/src/+/559af091bb5d7f598e7259a13fe2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/559af091bb5d7f598e7259a13fe2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
