Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(110)

Issue 1139623003: Removes the redundant IMB::OnFocus call in DNWA::HandleNativeFocus(). (Closed)

Created:
5 years, 7 months ago by Shu Chen
Modified:
5 years, 7 months ago
Reviewers:
ananta, yukawa, sky
CC:
chromium-reviews, nona+watch_chromium.org, James Su, shuchen+watch_chromium.org, penghuang+watch_chromium.org, yusukes+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Removes the redundant IMB::OnFocus call in DNWA::HandleNativeFocus(). Currently HWNDMessageHandler::OnSetFocus happens before WM_ACTIVATE is handled in HWNDMessageHandler::OnWndProc(). Root cause: when WM_ACTIVATE comes, in HWNDMessageHandler::OnWndProc(), it will call DefWindowProc() first, which may result in generating WM_SETFOCUS message and call OnWndProc() recursively and therefore OnSetFocus() is called. After the recursive OnWndProc() returns, PostProcessActivateMessage() is called to handle WM_ACTIVATE. So DWTHW::HandleNativeFocus() happens before DNWA::HandleActivationChanged(). Therefore, IMB::OnFocus() will get NULL for host_->GetTextInputClient(), which will result in the IMB instance set to the host InputMethod as the focused TextInputClient (which should be the RWHVA). It would be risky to fix that order in HWNDMessageHandler. Instead, we can remove the IMB::OnFocus call in DWTHW::HandleNativeFocus() which seems to be redundant, because DNWA::OnWindowFocused() can do the thing, which happens after the top window is activated. TBR=ananta@chromium.org,sky@chromium.org BUG=486604 TEST=Verified on local win build. Committed: https://crrev.com/4719aba37c66ae6f677977107819119494d70748 Cr-Commit-Position: refs/heads/master@{#330707}

Patch Set 1 #

Patch Set 2 : #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+0 lines, -6 lines) Patch
M ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc View 1 1 chunk +0 lines, -6 lines 4 comments Download

Messages

Total messages: 13 (3 generated)
Shu Chen
Hi yukawa@ & sadrul@, can you please review this? Thanks Yukawa and I have done ...
5 years, 7 months ago (2015-05-15 06:58:47 UTC) #2
yukawa
LGTM from the viewpoint of Windows IME message handling.
5 years, 7 months ago (2015-05-15 07:24:31 UTC) #3
sadrul
--> sky@/ananta@ I am not really a good reviewer for Windows code, but left a ...
5 years, 7 months ago (2015-05-16 02:24:26 UTC) #5
Shu Chen
TBR'ed ananta@ & sky@ as it is not easy to identify issues by reviewing without ...
5 years, 7 months ago (2015-05-20 06:28:48 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139623003/20001
5 years, 7 months ago (2015-05-20 06:29:19 UTC) #8
Shu Chen
https://codereview.chromium.org/1139623003/diff/20001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1139623003/diff/20001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode827 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:827: } On 2015/05/16 02:24:25, sadrul wrote: > The X11 ...
5 years, 7 months ago (2015-05-20 06:30:52 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 7 months ago (2015-05-20 07:11:27 UTC) #10
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/4719aba37c66ae6f677977107819119494d70748 Cr-Commit-Position: refs/heads/master@{#330707}
5 years, 7 months ago (2015-05-20 07:12:06 UTC) #11
sky
https://codereview.chromium.org/1139623003/diff/20001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1139623003/diff/20001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode822 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:822: // TODO(beng): inform the native_widget_delegate_. I think it's worth ...
5 years, 7 months ago (2015-05-22 17:29:03 UTC) #12
Shu Chen
5 years, 7 months ago (2015-05-25 01:14:25 UTC) #13
Message was sent while issue was closed.
https://codereview.chromium.org/1139623003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/1139623003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:822: // TODO(beng):
inform the native_widget_delegate_.
On 2015/05/22 17:29:02, sky wrote:
> I think it's worth a comment here as to why you don't need to call to
> InputMethod.

Thanks for your comment. My later cl soon to refactor the IMF (ownership change
for InputMethod) will make window tree host to call OnFocus/OnBlur of
InputMethod.

Powered by Google App Engine
This is Rietveld 408576698