Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2593323002:
Use the physical-pixel space for native IME on linux platform. (Closed)
|
Created:
4 years ago by Shu Chen Modified:
3 years, 10 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, nektar+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, dmazzoni+watch_chromium.org, aboxhall+watch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, je_julie, darin-cc_chromium.org, kalyank, yuzo+watch_chromium.org, oshima+watch_chromium.org, piman+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, tfarina, shuchen+watch_chromium.org, dtseng+watch_chromium.org, danakj+watch_chromium.org, James Su, davemoore+watch_chromium.org, yukawa Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse the physical-pixel space for native IME on linux platform.
What's more, this cl makes a clear spec for the coordinates system in
ui::TextInputClient.
BUG=475718, 360334
Review-Url: https://codereview.chromium.org/2593323002
Cr-Commit-Position: refs/heads/master@{#451980}
Committed: https://chromium.googlesource.com/chromium/src/+/da4307a16fe8c0bbecc8158e21ffef8190e23623
Patch Set 1 #
Total comments: 11
Patch Set 2 : refactor the cl, removed the GetDeviceScaleFactor method. #
Total comments: 2
Patch Set 3 : Use the physical-pixel space for native IME on linux platform. #
Messages
Total messages: 55 (32 generated)
The CQ bit was checked by shuchen@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...
shuchen@chromium.org changed reviewers: + elijahtaylor@chromium.org, msw@chromium.org, sadrul@chromium.org, satorux@chromium.org, yukawa@chromium.org
Guys, can you please review this cl? Thanks! msw@: chrome/browser/ui/views/ime_driver/... ui/views/... elijahtaylor@: components/arc/ime/... yukawa@: ui/base/ime/... sadrul@: content/browser/renderer_host/... satorux@: (stamp) chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc https://codereview.chromium.org/2593323002/diff/1/components/arc/ime/arc_ime_... File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2593323002/diff/1/components/arc/ime/arc_ime_... components/arc/ime/arc_ime_service.cc:277: return 1.0f; There might be a TODO for this.
https://codereview.chromium.org/2593323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2593323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1293: ->GetDisplayNearestWindow(window_).device_scale_factor(); just return |device_scale_factor_|? https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_au... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_au... ui/base/ime/input_method_auralinux.cc:285: client->GetScaleFactor())); Can you update the doc [1] to mention that this is in physical-pixel space? Alternatively, can this remain in DIP space, and the actual code where this is used [2] does any necessary conversion to pixel-space? [1] https://cs.chromium.org/chromium/src/ui/base/ime/linux/linux_input_method_con... [2] https://cs.chromium.org/chromium/src/chrome/browser/ui/libgtkui/x11_input_met... https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (left): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:240: display::win::ScreenWin::DIPToScreenRect(toplevel_window_handle_, Does the old code not do the right thing, because it looks like that too tries to convert from DIP to pixel-space? https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:649: gfx::ScaleToEnclosingRect(dip_rect, client->GetScaleFactor()); Can you use ConvertRectToDIP/ConvertRectToPixel instead? https://cs.chromium.org/chromium/src/ui/compositor/dip_util.h?q=ConvertRectTo... https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/text_input_clie... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/text_input_clie... ui/base/ime/text_input_client.h:89: virtual float GetScaleFactor() const = 0; Unfortunately, there are several types of scale factors (device scale factor, ui scale factor, page scale factor etc.). So I would suggest calling this GetDeviceScaleFactor()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2593323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2593323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1293: ->GetDisplayNearestWindow(window_).device_scale_factor(); On 2016/12/22 01:30:22, sadrul wrote: > just return |device_scale_factor_|? Ah, I didn't notice that. Thanks for pointing it out. I've removed the GetScaleFactor method. https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_au... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_au... ui/base/ime/input_method_auralinux.cc:285: client->GetScaleFactor())); On 2016/12/22 01:30:22, sadrul wrote: > Can you update the doc [1] to mention that this is in physical-pixel space? > Alternatively, can this remain in DIP space, and the actual code where this is > used [2] does any necessary conversion to pixel-space? > > [1] > https://cs.chromium.org/chromium/src/ui/base/ime/linux/linux_input_method_con... > [2] > https://cs.chromium.org/chromium/src/chrome/browser/ui/libgtkui/x11_input_met... I've leveraged [2] and removed the GetScaleFactor method. https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (left): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:240: display::win::ScreenWin::DIPToScreenRect(toplevel_window_handle_, On 2016/12/22 01:30:22, sadrul wrote: > Does the old code not do the right thing, because it looks like that too tries > to convert from DIP to pixel-space? The old code does the right thing but I think it would be better to use the consistent way to do the coordinates transform cross platforms (win/linux). https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:649: gfx::ScaleToEnclosingRect(dip_rect, client->GetScaleFactor()); On 2016/12/22 01:30:22, sadrul wrote: > Can you use ConvertRectToDIP/ConvertRectToPixel instead? > > https://cs.chromium.org/chromium/src/ui/compositor/dip_util.h?q=ConvertRectTo... I don't think so, because that would require a Layer instance which doesn't available in ui/base/ime space. https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/text_input_clie... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/2593323002/diff/1/ui/base/ime/text_input_clie... ui/base/ime/text_input_client.h:89: virtual float GetScaleFactor() const = 0; On 2016/12/22 01:30:22, sadrul wrote: > Unfortunately, there are several types of scale factors (device scale factor, ui > scale factor, page scale factor etc.). So I would suggest calling this > GetDeviceScaleFactor() Done. I've removed the GetScaleFactor method.
The CQ bit was checked by shuchen@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...
Description was changed from ========== Exposes the screen scale factor from ui::TextInputClient to ui::InputMethod. By adding the method TextInputClient::GetScaleFactor(), the platfor-dependent ui::InputMethod implementations can pass the correct non-dip coordinates to the native IME on those platforms (e.g. Windows/Linux/etc.). What's more, this cl makes a clear spec for the coordinates system in ui::TextInputClient. BUG=475718,360334 ========== to ========== Exposes the screen scale factor from ui::TextInputClient to ui::InputMethod. By adding the method TextInputClient::GetScaleFactor(), the platfor-dependent ui::InputMethod implementations can pass the correct non-dip coordinates to the native IME on those platforms (e.g. Windows/Linux/etc.). What's more, this cl makes a clear spec for the coordinates system in ui::TextInputClient. BUG=475718,360334 ==========
shuchen@chromium.org changed reviewers: - elijahtaylor@chromium.org, msw@chromium.org, satorux@chromium.org
Updated the reviews after the cl refactoring.
lgtm https://codereview.chromium.org/2593323002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtkui/x11_input_method_context_impl_gtk.cc (right): https://codereview.chromium.org/2593323002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtkui/x11_input_method_context_impl_gtk.cc:118: rect, views::LinuxUI::instance()->GetDeviceScaleFactor()); Consider using gfx::ConvertRectToPixel (https://cs.chromium.org/chromium/src/ui/gfx/geometry/dip_util.h?dr=CSs&q=Conv... ... sorry, last link I gave in previous CL was for the wrong version of the function)
Oh, please make sure to update the CL description too.
Description was changed from ========== Exposes the screen scale factor from ui::TextInputClient to ui::InputMethod. By adding the method TextInputClient::GetScaleFactor(), the platfor-dependent ui::InputMethod implementations can pass the correct non-dip coordinates to the native IME on those platforms (e.g. Windows/Linux/etc.). What's more, this cl makes a clear spec for the coordinates system in ui::TextInputClient. BUG=475718,360334 ========== to ========== Use the physical-pixel space for native IME on linux platform. What's more, this cl makes a clear spec for the coordinates system in ui::TextInputClient. BUG=475718,360334 ==========
On 2016/12/22 03:26:23, sadrul wrote: > Oh, please make sure to update the CL description too. Done. :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2593323002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtkui/x11_input_method_context_impl_gtk.cc (right): https://codereview.chromium.org/2593323002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtkui/x11_input_method_context_impl_gtk.cc:118: rect, views::LinuxUI::instance()->GetDeviceScaleFactor()); On 2016/12/22 03:25:53, sadrul (OOO) wrote: > Consider using gfx::ConvertRectToPixel > (https://cs.chromium.org/chromium/src/ui/gfx/geometry/dip_util.h?dr=CSs&q=Conv... > ... sorry, last link I gave in previous CL was for the wrong version of the > function) Done.
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2593323002/#ps40001 (title: "Use the physical-pixel space for native IME on linux platform.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
shuchen@chromium.org changed reviewers: + erg@chromium.org - yukawa@chromium.org
+erg@ for rubber stamp.
The CQ bit was checked by shuchen@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
shuchen@chromium.org changed reviewers: + estade@chromium.org
+estade@ for OWNERS rubber stamp for x11_input_method_context_impl_gtk.cc.
lgtm
The CQ bit was checked by shuchen@chromium.org
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shuchen@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shuchen@chromium.org
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
Exceeded global retry quota
The CQ bit was checked by shuchen@chromium.org
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shuchen@chromium.org
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": 40001, "attempt_start_ts": 1487750772221740,
"parent_rev": "b95cf24b16a79138e0f4c01375b3af62512dae2e", "commit_rev":
"da4307a16fe8c0bbecc8158e21ffef8190e23623"}
Message was sent while issue was closed.
Description was changed from ========== Use the physical-pixel space for native IME on linux platform. What's more, this cl makes a clear spec for the coordinates system in ui::TextInputClient. BUG=475718,360334 ========== to ========== Use the physical-pixel space for native IME on linux platform. What's more, this cl makes a clear spec for the coordinates system in ui::TextInputClient. BUG=475718,360334 Review-Url: https://codereview.chromium.org/2593323002 Cr-Commit-Position: refs/heads/master@{#451980} Committed: https://chromium.googlesource.com/chromium/src/+/da4307a16fe8c0bbecc8158e21ff... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/da4307a16fe8c0bbecc8158e21ff... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
