Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2296623002:
Fix the wrong candidate window on Mac OSX. (Closed)
|
Created:
4 years, 3 months ago by Seigo Nonaka Modified:
4 years, 3 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, James Su Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the wrong candidate window on Mac OSX.
After http://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b,
renderer no longer capturing the composition bounds by default.
To receive the latest composition bounds from renderer, we need to
update the monitor state and it was done for aura platform in above
CL. This CL introduces the similar logic to RWHVM to fix the wrong
candidate window issue.
BUG=639552
Committed: https://crrev.com/fbd66b59cd50bacb823d7aeb6396f32594583fb1
Cr-Commit-Position: refs/heads/master@{#417877}
Patch Set 1 : Fix the wrong candidate window on Mac OSX. #
Total comments: 7
Patch Set 2 : Addresses the comments #Messages
Total messages: 15 (7 generated)
Description was changed from ========== Fix the wrong candidate window on Mac OSX. After http://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b, renderer no longer capturing the composition bounds by default. To receive the latest composition bounds from renderer, we need to update the monitor state and it was done for aura platform in above CL. This CL fixes the wrong candidate window issue by updating the monitor state based on the text input focus state. BUG=639552 ========== to ========== Fix the wrong candidate window on Mac OSX. After http://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b, renderer no longer capturing the composition bounds by default. To receive the latest composition bounds from renderer, we need to update the monitor state and it was done for aura platform in above CL. This CL introduces the similar logic to RWHVM to fix the wrong candidate window issue. BUG=639552 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
nona@chromium.org changed reviewers: + asvitkine@chromium.org
Hi Alexei, could you kindly take a look? Thank you. https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:913: if (!info) This is now necessary since GetCompositionRangeInfo can return nullptr. This typically happens on browser tests.
Thanks! Given the Aura CL landed before M54 branch, it sounds like this is broken on M54 and once this lands we should merge it back to avoid the regression. Is that right? https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:878: RenderWidgetHost* host = GetRenderWidgetHost(); Nit: Remove this line and just inline render_widget_host_->GetRoutingID() into the IPC ctor. https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:881: HasFocus() && state && state->type != ui::TEXT_INPUT_TYPE_NONE; Nit: Wrong indent, maybe just run git cl format? https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:885: if (HasFocus()) { You're adding another call to HasFocus(). Please only call it once and move the result in a local var.
Thank you for your review and I'm sorry for slow reply. Looks like bot are not working well but I I verified that this patch passes the browser_tests on my local. Could you kindly take an another look? Thank you. https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:878: RenderWidgetHost* host = GetRenderWidgetHost(); On 2016/09/02 15:48:05, Alexei Svitkine wrote: > Nit: Remove this line and just inline render_widget_host_->GetRoutingID() into > the IPC ctor. Done. https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:881: HasFocus() && state && state->type != ui::TEXT_INPUT_TYPE_NONE; On 2016/09/02 15:48:05, Alexei Svitkine wrote: > Nit: Wrong indent, maybe just run git cl format? Ah yes, sorry I ran git cl format for the latest patch. Thank you. https://codereview.chromium.org/2296623002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:885: if (HasFocus()) { On 2016/09/02 15:48:05, Alexei Svitkine wrote: > You're adding another call to HasFocus(). Please only call it once and move the > result in a local var. Done.
LGTM Once this lands and has a few days to bake on Canary, please get it merged to M54 assuming the problem was introduced there.
Thank you for your review! Yes, I need to merge to M54. I'm going to verify this on canary channel, then once it is fixed will request M54 merge.
The CQ bit was checked by nona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix the wrong candidate window on Mac OSX. After http://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b, renderer no longer capturing the composition bounds by default. To receive the latest composition bounds from renderer, we need to update the monitor state and it was done for aura platform in above CL. This CL introduces the similar logic to RWHVM to fix the wrong candidate window issue. BUG=639552 ========== to ========== Fix the wrong candidate window on Mac OSX. After http://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b, renderer no longer capturing the composition bounds by default. To receive the latest composition bounds from renderer, we need to update the monitor state and it was done for aura platform in above CL. This CL introduces the similar logic to RWHVM to fix the wrong candidate window issue. BUG=639552 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix the wrong candidate window on Mac OSX. After http://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b, renderer no longer capturing the composition bounds by default. To receive the latest composition bounds from renderer, we need to update the monitor state and it was done for aura platform in above CL. This CL introduces the similar logic to RWHVM to fix the wrong candidate window issue. BUG=639552 ========== to ========== Fix the wrong candidate window on Mac OSX. After http://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b, renderer no longer capturing the composition bounds by default. To receive the latest composition bounds from renderer, we need to update the monitor state and it was done for aura platform in above CL. This CL introduces the similar logic to RWHVM to fix the wrong candidate window issue. BUG=639552 Committed: https://crrev.com/fbd66b59cd50bacb823d7aeb6396f32594583fb1 Cr-Commit-Position: refs/heads/master@{#417877} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fbd66b59cd50bacb823d7aeb6396f32594583fb1 Cr-Commit-Position: refs/heads/master@{#417877} |
