Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688413007:
Add display::GetDisplayNearestView (Closed)
|
Created:
3 years, 10 months ago by Jinsuk Kim Modified:
3 years, 9 months ago CC:
chromium-reviews, asanka, yusukes+watch_chromium.org, tzik, posciak+watch_chromium.org, chfremer+watch_chromium.org, miu+watch_chromium.org, extensions-reviews_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, chromium-apps-reviews_chromium.org, xjz+watch_chromium.org, nhiroki, feature-media-reviews_chromium.org, feature-vr-reviews_chromium.org, mcasas+watch+vc_chromium.org, Lei Zhang, tfarina, shuchen+watch_chromium.org, tommycli, mac-reviews_chromium.org, James Su, kinuko+fileapi, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd display::GetDisplayNearestView
Add a method that gets the display with a given gfx::NativeView.
The present API |display::GetDisplayNearestWindow| was also updated
to accept gfx::NativeWindow to match its signature.
BUG=671401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=benwells@chromium.org,sky@chromium.org,boliu@chromium.org,msw@chromium.org,halliwell@chromium.org,bshe@chromium.org,alexclarke@chromium.org
Review-Url: https://codereview.chromium.org/2688413007
Cr-Commit-Position: refs/heads/master@{#457016}
Committed: https://chromium.googlesource.com/chromium/src/+/93bb5a29073d2cb41b20d3cc6cf59459a3906043
Patch Set 1 #
Total comments: 2
Patch Set 2 : add missing files #
Total comments: 34
Patch Set 3 : fix builds #Patch Set 4 : fix tests #Patch Set 5 : GetDisplayNearestView #Patch Set 6 : updated comment #Patch Set 7 : WindowForView #
Total comments: 2
Patch Set 8 : no |WindowForView| yet #Patch Set 9 : WindowForView #Patch Set 10 : test GetDisplayNearestView #
Total comments: 2
Patch Set 11 : rebase #Patch Set 12 : updated callsites #Patch Set 13 : fix tests #
Total comments: 2
Patch Set 14 : updated log #
Total comments: 8
Patch Set 15 : nits #Patch Set 16 : updated log #2 #
Total comments: 4
Patch Set 17 : updated log #3 #Messages
Total messages: 197 (143 generated)
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org, oshima@chromium.org, tapted@chromium.org
Please see https://codereview.chromium.org/2688113002/#msg26 for the background. Notes: - Couldn't put it in the target //ui/gfx since GetTopLevel has a dependency on //ui/aura, which causes a cyclic dependency. Defined a separate target //ui/gfx:view_util for that. In fact using the method in almost any target under /ui leads to cyclic dependency. I avoided it by not using the method in under /ui at all. This resulted in a few other method signature changes under /ui, such as ui::GetScaleFactorForNative{View -> Window}. - Tested the build for linux/android only. Other platforms like mac may have a problem. Please review the overall direction first. Will handle all the build/tests soon.
Patchset #1 (id:1) has been deleted
One more: Didn't move |GetViewForWindow| to ui/ since it was not necessary for |display::GetDisplayNearestWindow| that this CL was created for. But I can move it too if you think that's better.
https://codereview.chromium.org/2688413007/diff/20001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/2688413007/diff/20001/ui/gfx/BUILD.gn#newcode521 ui/gfx/BUILD.gn:521: "view_util.h", did you forget to upload these files?
https://codereview.chromium.org/2688413007/diff/20001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/2688413007/diff/20001/ui/gfx/BUILD.gn#newcode521 ui/gfx/BUILD.gn:521: "view_util.h", On 2017/02/14 16:25:46, boliu wrote: > did you forget to upload these files? Oops... added them.
https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1425: if (touch_emulator_) { nit: curlies not needed https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1961: touch_emulator_.reset( touch_emulator_ = base::MakeUnique<TouchEmulator>(..) https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:670: return gfx::Size(default_bounds_.right() * GetScaleFactorForNativeView(), gfx::ScaleToFlooredSize() ? https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:320: return ui::GetScaleFactorForNativeWindow(gfx::GetTopLevel(GetNativeView())); if callers still need the virtual method in RWHV, this should probably just be return current_device_scale_factor_; (content OWNERS might have other ideas though) That is, after stuff that has (or is) a RenderWidgetHostViewBase is using current_device_scale_factor(), the remaining things outside of content wanting GetScaleFactorForNativeView() can probably just go back to what they were doing before. However, those methods are a LOT more expensive than just using current_device_scale_factor_ . For example, in ScreenMac::GetDisplayNearestWindow() has a comment // Note the following line calls -[NSWindow // _bestScreenBySpaceAssignmentOrGeometry] which is quite expensive and // performs IPC with the window server process. NSScreen* match_screen = [window screen]; it would be nice to avoid that. i.e. my vote would be to use a content-public/exported virtual method on RenderWidgetHostView that just returns current_device_scale_factor_ here. But there's usually a resistance to adding new content-public methods. You'll need a content OWNER for this. https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:83: return current_device_scale_factor_; Can any callers just use this instead? (i.e. rather than adding GetScaleFactorForNativeView() - it's confusing having both) https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:694: float RenderWidgetHostViewMac::ViewScaleFactor() const { This method should just be deleted. Callers should either use current_device_scale_factor() from the parent class, or the GetScaleFactorForNativeView() method that you're adding. https://codereview.chromium.org/2688413007/diff/40001/ui/base/layout.cc File ui/base/layout.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/base/layout.cc#newco... ui/base/layout.cc:22: #include "ui/gfx/view_util.h" is this needed? https://codereview.chromium.org/2688413007/diff/40001/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/display/screen.cc#ne... ui/display/screen.cc:9: #include "ui/gfx/view_util.h" is this needed? https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/BUILD.gn#newcode533 ui/gfx/BUILD.gn:533: sources += [ "view_util_mac.mm" ] foo_mac should be automatically stripped - this can just be listed in sources= https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/BUILD.gn#newcode542 ui/gfx/BUILD.gn:542: # structure rather than random samplings of ui/gfx and ui/gfx/mac. Note this comment. We should put the new component into its own directory, add DEPS, etc. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/DEPS File ui/gfx/DEPS (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/DEPS#newcode11 ui/gfx/DEPS:11: "+ui/aura/window.h", These don't belong here - we are not adding a library dependency, so these headers should not be permitted under ui/gfx. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util.h File ui/gfx/view_util.h (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util.h#newc... ui/gfx/view_util.h:5: #ifndef UI_GFX_VIEW_UTIL_H_ view_util isn't a good name -- neither for the component or the header (e.g. sounds like it should be related to ui/views, but it's not at all. also "foo_util.h" has a risk of becoming a grab-bag of unrelated things -- it's too generic). I think this is most related to ui/gfx/native_widget_types.h so I suggest calling this header - ui/gfx/native_widget/native_widget_properties.h or - ui/native_widget/native_widget_properties.h or - ui/display/native_widget_properties.h oshima@ may have an opinion here (I don't have OWNERS powers around here :) https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util.h#newc... ui/gfx/view_util.h:14: GFX_EXPORT gfx::NativeWindow GetTopLevel(gfx::NativeView view); gfx:: prefixes are not needed -- more elsewhere https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_androi... File ui/gfx/view_util_android.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_androi... ui/gfx/view_util_android.cc:5: #include <jni.h> is this needed? https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_androi... ui/gfx/view_util_android.cc:8: #include "ui/gfx/view_util.h" move to top, blank line after https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_aura.cc File ui/gfx/view_util_aura.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_aura.c... ui/gfx/view_util_aura.cc:6: #include "ui/gfx/view_util.h" move to top https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_mac.mm File ui/gfx/view_util_mac.mm (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_mac.mm... ui/gfx/view_util_mac.mm:5: #include "ui/gfx//platform_util.h" there's lots wrong here - please run trybots
Description was changed from ========== Pass gfx::NativeWindow to |display::GetDisplayNearestWindow()| Previously |display::GetDisplayNearestWindow()| takes gfx::NativeView. This CL changes the parameter to gfx::NativeWindow to match its method name. Also moved a utility method platform_util.cc::GetTopLevel() in chrome to ui/gfx for transition. Now it is utilized not only for chrome but also for content. BUG=671401 ========== to ========== Pass gfx::NativeWindow to |display::GetDisplayNearestWindow()| Previously |display::GetDisplayNearestWindow()| takes gfx::NativeView. This CL changes the parameter to gfx::NativeWindow to match its method name. Also moved a utility method platform_util.cc::GetTopLevel() in chrome to ui/gfx for transition. Now it is utilized not only for chrome but also for content. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jinsukkim@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jinsukkim@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jinsukkim@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...
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 jinsukkim@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@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 ========== Pass gfx::NativeWindow to |display::GetDisplayNearestWindow()| Previously |display::GetDisplayNearestWindow()| takes gfx::NativeView. This CL changes the parameter to gfx::NativeWindow to match its method name. Also moved a utility method platform_util.cc::GetTopLevel() in chrome to ui/gfx for transition. Now it is utilized not only for chrome but also for content. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Pass gfx::NativeWindow to |display::GetDisplayNearestWindow()| Previously |display::GetDisplayNearestWindow()| took gfx::NativeView. This CL changes the parameter to gfx::NativeWindow to match its method name. Also moved a utility method platform_util.cc::GetTopLevel() in chrome to ui/gfx for transition. Now it is utilized not only for chrome but also for content. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Still got a couple of failing tests. Will fix them soon. Would you take another look in the mean time for any other potential issues? https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1425: if (touch_emulator_) { On 2017/02/15 00:36:10, tapted wrote: > nit: curlies not needed Done. https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1961: touch_emulator_.reset( On 2017/02/15 00:36:10, tapted wrote: > touch_emulator_ = base::MakeUnique<TouchEmulator>(..) Done. https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:670: return gfx::Size(default_bounds_.right() * GetScaleFactorForNativeView(), On 2017/02/15 00:36:10, tapted wrote: > gfx::ScaleToFlooredSize() ? Done. https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:320: return ui::GetScaleFactorForNativeWindow(gfx::GetTopLevel(GetNativeView())); On 2017/02/15 00:36:10, tapted wrote: > if callers still need the virtual method in RWHV, this should probably just be > return current_device_scale_factor_; (content OWNERS might have other ideas > though) > > That is, after stuff that has (or is) a RenderWidgetHostViewBase is using > current_device_scale_factor(), the remaining things outside of content wanting > GetScaleFactorForNativeView() can probably just go back to what they were doing > before. > > However, those methods are a LOT more expensive than just using > current_device_scale_factor_ . > > For example, in ScreenMac::GetDisplayNearestWindow() has a comment > > // Note the following line calls -[NSWindow > // _bestScreenBySpaceAssignmentOrGeometry] which is quite expensive and > // performs IPC with the window server process. > NSScreen* match_screen = [window screen]; > > it would be nice to avoid that. > > > i.e. my vote would be to use a content-public/exported virtual method on > RenderWidgetHostView that just returns current_device_scale_factor_ here. > > But there's usually a resistance to adding new content-public methods. You'll > need a content OWNER for this. Thanks for the suggestion. Done as suggested. I think the new method is useful so I'm hoping its addition is justifiable. https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:83: return current_device_scale_factor_; On 2017/02/15 00:36:10, tapted wrote: > Can any callers just use this instead? > > (i.e. rather than adding GetScaleFactorForNativeView() - it's confusing having > both) > Replaced this with virtual GetScaleFactorForNativeView() instead. The method is useful to embedders but this class xxx_base is not public. https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2688413007/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:694: float RenderWidgetHostViewMac::ViewScaleFactor() const { On 2017/02/15 00:36:10, tapted wrote: > This method should just be deleted. Callers should either use > current_device_scale_factor() from the parent class, or the > GetScaleFactorForNativeView() method that you're adding. Done. https://codereview.chromium.org/2688413007/diff/40001/ui/base/layout.cc File ui/base/layout.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/base/layout.cc#newco... ui/base/layout.cc:22: #include "ui/gfx/view_util.h" On 2017/02/15 00:36:10, tapted wrote: > is this needed? Removed. https://codereview.chromium.org/2688413007/diff/40001/ui/display/screen.cc File ui/display/screen.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/display/screen.cc#ne... ui/display/screen.cc:9: #include "ui/gfx/view_util.h" On 2017/02/15 00:36:10, tapted wrote: > is this needed? Removed. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/BUILD.gn#newcode533 ui/gfx/BUILD.gn:533: sources += [ "view_util_mac.mm" ] On 2017/02/15 00:36:10, tapted wrote: > foo_mac should be automatically stripped - this can just be listed in sources= Done. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/BUILD.gn#newcode542 ui/gfx/BUILD.gn:542: # structure rather than random samplings of ui/gfx and ui/gfx/mac. On 2017/02/15 00:36:10, tapted wrote: > Note this comment. We should put the new component into its own directory, add > DEPS, etc. Yes moved it under ui/gfx/native_widget for now. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/DEPS File ui/gfx/DEPS (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/DEPS#newcode11 ui/gfx/DEPS:11: "+ui/aura/window.h", On 2017/02/15 00:36:10, tapted wrote: > These don't belong here - we are not adding a library dependency, so these > headers should not be permitted under ui/gfx. Removed https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util.h File ui/gfx/view_util.h (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util.h#newc... ui/gfx/view_util.h:5: #ifndef UI_GFX_VIEW_UTIL_H_ On 2017/02/15 00:36:10, tapted wrote: > view_util isn't a good name -- neither for the component or the header (e.g. > sounds like it should be related to ui/views, but it's not at all. also > "foo_util.h" has a risk of becoming a grab-bag of unrelated things -- it's too > generic). > > I think this is most related to ui/gfx/native_widget_types.h > > so I suggest calling this header > - ui/gfx/native_widget/native_widget_properties.h or > - ui/native_widget/native_widget_properties.h or > - ui/display/native_widget_properties.h > > oshima@ may have an opinion here (I don't have OWNERS powers around here :) Point taken. Tentatively chose the first one. Will wait for oshima@'s input too. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util.h#newc... ui/gfx/view_util.h:14: GFX_EXPORT gfx::NativeWindow GetTopLevel(gfx::NativeView view); On 2017/02/15 00:36:10, tapted wrote: > gfx:: prefixes are not needed -- more elsewhere Done. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_androi... File ui/gfx/view_util_android.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_androi... ui/gfx/view_util_android.cc:5: #include <jni.h> On 2017/02/15 00:36:10, tapted wrote: > is this needed? Done. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_androi... ui/gfx/view_util_android.cc:8: #include "ui/gfx/view_util.h" On 2017/02/15 00:36:10, tapted wrote: > move to top, blank line after Done. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_aura.cc File ui/gfx/view_util_aura.cc (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_aura.c... ui/gfx/view_util_aura.cc:6: #include "ui/gfx/view_util.h" On 2017/02/15 00:36:10, tapted wrote: > move to top Done. https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_mac.mm File ui/gfx/view_util_mac.mm (right): https://codereview.chromium.org/2688413007/diff/40001/ui/gfx/view_util_mac.mm... ui/gfx/view_util_mac.mm:5: #include "ui/gfx//platform_util.h" On 2017/02/15 00:36:10, tapted wrote: > there's lots wrong here - please run trybots Testing now...
The CQ bit was checked by jinsukkim@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Just did a super-quick skim - a content owner should take a look before I push you in a direction they don't like. (it might also be nice to defer the content change -- I wasn't expecting all of that when I suggested moving platform_util to gfx -- there's probably a nice way to split this patch) https://codereview.chromium.org/2688413007/diff/190001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2688413007/diff/190001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:664: return gfx::ScaleToFlooredSize(default_bounds_.size(), note this won't be the same as the old code if default_bounds_.origin() can ever be something other than 0,0. (I mentioned ScaleToFlooredSize just because it looked weird to multiply two axes separately - you might need a gfx::Size local var to apply ScaleToFlooredSize to) https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/DEPS File ui/gfx/native_widget/DEPS (right): https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/D... ui/gfx/native_widget/DEPS:2: "+ui/android/view_android.h", I would just add the paths (drop the foo.h) https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/n... File ui/gfx/native_widget/native_widget_properties_mac.mm (right): https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/n... ui/gfx/native_widget/native_widget_properties_mac.mm:7: #include "ui/gfx/native_widget/native_widget_properties.h" move to top https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/n... ui/gfx/native_widget/native_widget_properties_mac.mm:13: #if !defined(USE_AURA) these #ifs can be dropped (this file shouldn't be compiled at all if USE_AURA is set) https://codereview.chromium.org/2688413007/diff/190001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2688413007/diff/190001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1062: gfx::NativeWindow window = gfx::GetTopLevel(GetWidget()->GetNativeView()); GetWidget()->GetNativeWindow() https://codereview.chromium.org/2688413007/diff/190001/ui/views/drag_utils.cc File ui/views/drag_utils.cc (right): https://codereview.chromium.org/2688413007/diff/190001/ui/views/drag_utils.cc... ui/views/drag_utils.cc:19: gfx::NativeWindow window = gfx::GetTopLevel(widget->GetNativeView()); widget->GetNativeWindow()
Patchset #5 (id:190001) has been deleted
The CQ bit was checked by jinsukkim@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...
Patchset #5 (id:210001) has been deleted
Patchset #4 (id:170001) has been deleted
On 2017/02/16 11:45:09, tapted wrote: > Just did a super-quick skim - a content owner should take a look before I push > you in a direction they don't like. > > (it might also be nice to defer the content change -- I wasn't expecting all of > that when I suggested moving platform_util to gfx -- there's probably a nice way > to split this patch) > > https://codereview.chromium.org/2688413007/diff/190001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/2688413007/diff/190001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_android.cc:664: return > gfx::ScaleToFlooredSize(default_bounds_.size(), > note this won't be the same as the old code if default_bounds_.origin() can ever > be something other than 0,0. (I mentioned ScaleToFlooredSize just because it > looked weird to multiply two axes separately - you might need a gfx::Size local > var to apply ScaleToFlooredSize to) > > https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/DEPS > File ui/gfx/native_widget/DEPS (right): > > https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/D... > ui/gfx/native_widget/DEPS:2: "+ui/android/view_android.h", > I would just add the paths (drop the foo.h) > > https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/n... > File ui/gfx/native_widget/native_widget_properties_mac.mm (right): > > https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/n... > ui/gfx/native_widget/native_widget_properties_mac.mm:7: #include > "ui/gfx/native_widget/native_widget_properties.h" > move to top > > https://codereview.chromium.org/2688413007/diff/190001/ui/gfx/native_widget/n... > ui/gfx/native_widget/native_widget_properties_mac.mm:13: #if !defined(USE_AURA) > these #ifs can be dropped (this file shouldn't be compiled at all if USE_AURA is > set) > > https://codereview.chromium.org/2688413007/diff/190001/ui/views/controls/text... > File ui/views/controls/textfield/textfield.cc (right): > > https://codereview.chromium.org/2688413007/diff/190001/ui/views/controls/text... > ui/views/controls/textfield/textfield.cc:1062: gfx::NativeWindow window = > gfx::GetTopLevel(GetWidget()->GetNativeView()); > GetWidget()->GetNativeWindow() > > https://codereview.chromium.org/2688413007/diff/190001/ui/views/drag_utils.cc > File ui/views/drag_utils.cc (right): > > https://codereview.chromium.org/2688413007/diff/190001/ui/views/drag_utils.cc... > ui/views/drag_utils.cc:19: gfx::NativeWindow window = > gfx::GetTopLevel(widget->GetNativeView()); > widget->GetNativeWindow() Apology I deleted the erroneous patch after uploading a new one, not knowing you're on it.... But the comment are clear - I'll address them soon.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2017/02/16 11:45:09, tapted wrote: > Just did a super-quick skim - a content owner should take a look before I push > you in a direction they don't like. > > (it might also be nice to defer the content change -- I wasn't expecting all of > that when I suggested moving platform_util to gfx -- there's probably a nice way > to split this patch) > Yes the CL got really messy. I'll think about how to split it or figure out how not to have the change propagate too widely.
Taking a step back.. and looking more at the call sites in this CL. It seems a lot of things don't really want |display::GetDisplayNearestWindow()| -- they just want *either* of float ScaleFactorForNativeWindow(gfx::NativeWindow); float ScaleFactorForNativeView(gfx::NativeView); ScaleFactorForNativeView already exists in ui/base/layout.h (maybe this header needs a better name, but that's orthogonal) -- why not just add an overloaded ScaleFactorForNativeWindow rather than replacing ScaleFactorForNativeView? layout_mac.mm already has an optimized version of GetScaleFactorForNativeView for Mac which doesn't use the slow codepaths in gfx::ScreenMac. Adding ScaleFactorForNativeWindow there will allow the optimized version to be used for windows as well. We can probably just add layout_android.cc to cover the window -> view mapping that android wants. In this plan I would *not* move GetTopLevel() out of platform_util -- that's orthogonal. Instead, I would just put the bits from platform_util_mac.mm and platform_util_android.cc that you need into layout_mac.mm (already exists) and layout_android.cc (new file). This doesn't help you with what you need for https://codereview.chromium.org/2688113002/, but it should move a lot of callsites using GetDisplayNearestWindow() and GetTopLevel() off of those gfx::Screen/platform_util APIs and onto the thing they really want in ui/base/layout.h to get the device scale factor.
On 2017/02/16 22:26:33, tapted wrote: > Taking a step back.. and looking more at the call sites in this CL. It seems a > lot of things don't really want |display::GetDisplayNearestWindow()| -- they > just want *either* of > > float ScaleFactorForNativeWindow(gfx::NativeWindow); > float ScaleFactorForNativeView(gfx::NativeView); > > ScaleFactorForNativeView already exists in ui/base/layout.h (maybe this header > needs a better name, but that's orthogonal) -- why not just add an overloaded > ScaleFactorForNativeWindow rather than replacing ScaleFactorForNativeView? > The problem is the circular dependency around gfx::GetTopLevel. |float ScaleFactorForNativeView(gfx::NativeView)| needs it to get |NativeWindow| used for |GetDisplayNearestWindow|. Then we get dep cycle //ui/gfx//native_widget (as of now) -> //ui/aura -> //ui/display -> //ui/gfx/native_widget
On 2017/02/16 23:17:37, Jinsuk Kim wrote: > On 2017/02/16 22:26:33, tapted wrote: > > Taking a step back.. and looking more at the call sites in this CL. It seems a > > lot of things don't really want |display::GetDisplayNearestWindow()| -- they > > just want *either* of > > > > float ScaleFactorForNativeWindow(gfx::NativeWindow); > > float ScaleFactorForNativeView(gfx::NativeView); > > > > ScaleFactorForNativeView already exists in ui/base/layout.h (maybe this header > > needs a better name, but that's orthogonal) -- why not just add an overloaded > > ScaleFactorForNativeWindow rather than replacing ScaleFactorForNativeView? > > > The problem is the circular dependency around gfx::GetTopLevel. > |float ScaleFactorForNativeView(gfx::NativeView)| needs it to get |NativeWindow| > used for |GetDisplayNearestWindow|. Then we get dep cycle > > //ui/gfx//native_widget (as of now) -> > //ui/aura -> > //ui/display -> > //ui/gfx/native_widget This is why I suggested we *not* move GetTopLevel() out of platform_util. Also note that the aura version of `GetTopLevel` doesn't actually need an aura dependency since gfx::NativeWindow and gfx::NativeView are the same thing -- there should be no aura symbol required.
On 2017/02/16 23:21:29, tapted wrote: > On 2017/02/16 23:17:37, Jinsuk Kim wrote: > > On 2017/02/16 22:26:33, tapted wrote: > > > Taking a step back.. and looking more at the call sites in this CL. It seems > a > > > lot of things don't really want |display::GetDisplayNearestWindow()| -- they > > > just want *either* of > > > > > > float ScaleFactorForNativeWindow(gfx::NativeWindow); > > > float ScaleFactorForNativeView(gfx::NativeView); > > > > > > ScaleFactorForNativeView already exists in ui/base/layout.h (maybe this > header > > > needs a better name, but that's orthogonal) -- why not just add an > overloaded > > > ScaleFactorForNativeWindow rather than replacing ScaleFactorForNativeView? > > > > > The problem is the circular dependency around gfx::GetTopLevel. > > |float ScaleFactorForNativeView(gfx::NativeView)| needs it to get > |NativeWindow| > > used for |GetDisplayNearestWindow|. Then we get dep cycle > > > > //ui/gfx//native_widget (as of now) -> > > //ui/aura -> > > //ui/display -> > > //ui/gfx/native_widget > > This is why I suggested we *not* move GetTopLevel() out of platform_util. > > Also note that the aura version of `GetTopLevel` doesn't actually need an aura > dependency since gfx::NativeWindow and gfx::NativeView are the same thing -- > there should be no aura symbol required. Ah, there's a missing bit here: we don't actually need `GetTopLevel` to map from gfx::NativeView to gfx::NativeWindow for the purposes of obtaining a gfx::Display under Aura -- using a "non-toplevel" aura::Window should work just the same. I think relying on this will simplify things a lot.
On 2017/02/16 23:26:45, tapted wrote: > On 2017/02/16 23:21:29, tapted wrote: > > On 2017/02/16 23:17:37, Jinsuk Kim wrote: > > > On 2017/02/16 22:26:33, tapted wrote: > > > > Taking a step back.. and looking more at the call sites in this CL. It > seems > > a > > > > lot of things don't really want |display::GetDisplayNearestWindow()| -- > they > > > > just want *either* of > > > > > > > > float ScaleFactorForNativeWindow(gfx::NativeWindow); > > > > float ScaleFactorForNativeView(gfx::NativeView); > > > > > > > > ScaleFactorForNativeView already exists in ui/base/layout.h (maybe this > > header > > > > needs a better name, but that's orthogonal) -- why not just add an > > overloaded > > > > ScaleFactorForNativeWindow rather than replacing ScaleFactorForNativeView? > > > > > > > The problem is the circular dependency around gfx::GetTopLevel. > > > |float ScaleFactorForNativeView(gfx::NativeView)| needs it to get > > |NativeWindow| > > > used for |GetDisplayNearestWindow|. Then we get dep cycle > > > > > > //ui/gfx//native_widget (as of now) -> > > > //ui/aura -> > > > //ui/display -> > > > //ui/gfx/native_widget > > > > This is why I suggested we *not* move GetTopLevel() out of platform_util. > > > > Also note that the aura version of `GetTopLevel` doesn't actually need an aura > > dependency since gfx::NativeWindow and gfx::NativeView are the same thing -- > > there should be no aura symbol required. > > Ah, there's a missing bit here: we don't actually need `GetTopLevel` to map from > gfx::NativeView to gfx::NativeWindow for the purposes of obtaining a > gfx::Display under Aura -- using a "non-toplevel" aura::Window should work just > the same. I think relying on this will simplify things a lot. Thanks for the info/idea. Will look into that.
Patchset #5 (id:250001) has been deleted
The CQ bit was checked by jinsukkim@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@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...
Patchset #5 (id:270001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jinsukkim@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_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 jinsukkim@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by jinsukkim@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jinsukkim@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by jinsukkim@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #10 (id:390001) has been deleted
Patchset #9 (id:370001) has been deleted
Patchset #5 (id:290001) has been deleted
Patchset #5 (id:310001) has been deleted
Patchset #5 (id:330001) has been deleted
Patchset #5 (id:350001) has been deleted
Patchset #5 (id:410001) has been deleted
The CQ bit was checked by jinsukkim@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Patchset #5 (id:430001) has been deleted
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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by jinsukkim@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_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:450001) has been deleted
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Patchset #5 (id:470001) has been deleted
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.
Sorry for the long silence. I tried a few things but none of them work nicely
for what I wanted (having GetDisplayNearestWindow(gfx::NativeWindow)' in one
patch. I'd like to proceed in the following way in 3 steps:
1. Add display::GetDisplayNearestView(gfx::NativeView). I think this is better
(as a separate first step) than replacing
GetDisplayNearestWindow(gfx::Native{View -> Window}) which will require too many
call sites be updated and mixed with main change together.
2. Do the replacing |GetDisplayNearestWindow(gfx::Native{View -> Window})| to
match its signature, and update call sites.
3. Add ui::GetScaleFactorForNativeWindow which will use GetDisplayNearestWindow,
and simplify some call sites calling |GetDisplayNearestView| just for scale
factor, using ui::GetScaleFactorForNative(View|Window)
Let me know if the plan looks okay. I think this will make it clear to see what
each step tries to do, and help split changes in manageable size.
On 2017/03/02 11:43:54, Jinsuk Kim wrote:
> Sorry for the long silence. I tried a few things but none of them work nicely
> for what I wanted (having GetDisplayNearestWindow(gfx::NativeWindow)' in one
> patch. I'd like to proceed in the following way in 3 steps:
>
> 1. Add display::GetDisplayNearestView(gfx::NativeView). I think this is
better
> (as a separate first step) than replacing
> GetDisplayNearestWindow(gfx::Native{View -> Window}) which will require too
many
> call sites be updated and mixed with main change together.
yeah - a separate method sgtm. Be sure to make it clear (e.g. in the method
comment) that it may still using the window that contains the view (i.e. if a
window is spread over two displays, the location of the view within that window
won't influence the result)
>
> 2. Do the replacing |GetDisplayNearestWindow(gfx::Native{View -> Window})| to
> match its signature, and update call sites.
I would do step 3 before this - that will probably result in fewer updates (i.e.
some calls to GetDisplayNearestWindow should go away completely)
>
> 3. Add ui::GetScaleFactorForNativeWindow which will use
GetDisplayNearestWindow,
> and simplify some call sites calling |GetDisplayNearestView| just for scale
> factor, using ui::GetScaleFactorForNative(View|Window)
Note, on Mac, this shouldn't use GetDisplayNearestWindow -- that method can be
quite slow since it needs to talk to the window server. But the scale factor
should be baked into the window already (or the view).
>
> Let me know if the plan looks okay. I think this will make it clear to see
what
> each step tries to do, and help split changes in manageable size.
Description was changed from ========== Pass gfx::NativeWindow to |display::GetDisplayNearestWindow()| Previously |display::GetDisplayNearestWindow()| took gfx::NativeView. This CL changes the parameter to gfx::NativeWindow to match its method name. Also moved a utility method platform_util.cc::GetTopLevel() in chrome to ui/gfx for transition. Now it is utilized not only for chrome but also for content. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add display::GetDisplayNearestView Add a method that gets the display with a given gfx::NativeView. The present API |display::GetDisplayNearestWindow| will be updated in a follow-up CL so that it accepts gfx::NativeWindow to match its signature. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2017/03/02 23:01:36, tapted wrote:
> On 2017/03/02 11:43:54, Jinsuk Kim wrote:
> > Sorry for the long silence. I tried a few things but none of them work
nicely
> > for what I wanted (having GetDisplayNearestWindow(gfx::NativeWindow)' in one
> > patch. I'd like to proceed in the following way in 3 steps:
> >
> > 1. Add display::GetDisplayNearestView(gfx::NativeView). I think this is
> better
> > (as a separate first step) than replacing
> > GetDisplayNearestWindow(gfx::Native{View -> Window}) which will require too
> many
> > call sites be updated and mixed with main change together.
>
> yeah - a separate method sgtm. Be sure to make it clear (e.g. in the method
> comment) that it may still using the window that contains the view (i.e. if a
> window is spread over two displays, the location of the view within that
window
> won't influence the result)
>
Added the comment as suggested. The step 1 is ready for review.
> >
> > 2. Do the replacing |GetDisplayNearestWindow(gfx::Native{View -> Window})|
to
> > match its signature, and update call sites.
>
> I would do step 3 before this - that will probably result in fewer updates
(i.e.
> some calls to GetDisplayNearestWindow should go away completely)
>
Yes will do.
> >
> > 3. Add ui::GetScaleFactorForNativeWindow which will use
> GetDisplayNearestWindow,
> > and simplify some call sites calling |GetDisplayNearestView| just for scale
> > factor, using ui::GetScaleFactorForNative(View|Window)
>
> Note, on Mac, this shouldn't use GetDisplayNearestWindow -- that method can be
> quite slow since it needs to talk to the window server. But the scale factor
> should be baked into the window already (or the view).
>
> >
> > Let me know if the plan looks okay. I think this will make it clear to see
> what
> > each step tries to do, and help split changes in manageable size.
oshima@ Main change is in ui/display/screen.h where the new method is added. Could you take a look? I think the others can be TBR'd since they are just mechanical changes following it.
This still doesn't feel right to me - seems like a lot of repetition / unnecessary stuff to maintain. I was kinda hoping something like this would work: https://codereview.chromium.org/2721203007 (am I crazy?) But I haven't had time today to try that out. (and it needs "something" for Android). Also, sorry in advance if this is another dead.
The CQ bit was checked by jinsukkim@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by jinsukkim@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...
Patchset #7 (id:530001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/03 06:00:29, tapted wrote: > This still doesn't feel right to me - seems like a lot of repetition / > unnecessary stuff to maintain. > > I was kinda hoping something like this would work: > https://codereview.chromium.org/2721203007 (am I crazy?) > > But I haven't had time today to try that out. (and it needs "something" for > Android). Also, sorry in advance if this is another dead. That works! Updated screen_base.cc as well. WindowForView is not in use yet - it can be put to use when the method |GetDisplayNearestWindow| gest updated to have a right param type. Let me handle it together with other platforms in the follow up CL if you're okay.
https://codereview.chromium.org/2688413007/diff/550001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2688413007/diff/550001/ui/display/screen.h#ne... ui/display/screen.h:61: // TODO(jinsukkim): Fix the parameter type to |gfx::NativeWindow|. I think the CL is simple enough that we should pursue this now. And we need to force a bit more "fallout" before we know if this approach is going to work - as it is, nothing is calling WindowForView(). So it is dead code, and we try not to add dead code to Chrome unless we are sure it will be used.
The CQ bit was checked by jinsukkim@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.
https://codereview.chromium.org/2688413007/diff/550001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2688413007/diff/550001/ui/display/screen.h#ne... ui/display/screen.h:61: // TODO(jinsukkim): Fix the parameter type to |gfx::NativeWindow|. On 2017/03/06 00:01:49, tapted wrote: > I think the CL is simple enough that we should pursue this now. And we need to > force a bit more "fallout" before we know if this approach is going to work - as > it is, nothing is calling WindowForView(). So it is dead code, and we try not to > add dead code to Chrome unless we are sure it will be used. No problem. Will add it when it gets actually put to use. Removed.
On 2017/03/06 06:06:19, Jinsuk Kim wrote: > https://codereview.chromium.org/2688413007/diff/550001/ui/display/screen.h > File ui/display/screen.h (right): > > https://codereview.chromium.org/2688413007/diff/550001/ui/display/screen.h#ne... > ui/display/screen.h:61: // TODO(jinsukkim): Fix the parameter type to > |gfx::NativeWindow|. > On 2017/03/06 00:01:49, tapted wrote: > > I think the CL is simple enough that we should pursue this now. And we need to > > force a bit more "fallout" before we know if this approach is going to work - > as > > it is, nothing is calling WindowForView(). So it is dead code, and we try not > to > > add dead code to Chrome unless we are sure it will be used. > > No problem. Will add it when it gets actually put to use. Removed. I didn't suggest removing it. Note that the same now applies for GetDisplayNearestView - nothing is calling it. So it's hard to see how this fits in the bigger picture. Do you have a WIP for your step 2?
> > I didn't suggest removing it. Note that the same now applies for > GetDisplayNearestView - nothing is calling it. So it's hard to see how this fits > in the bigger picture. > > Do you have a WIP for your step 2? Wish you made what you'd like to see clearer :/ Retored WindowForView in patch #9. Patch #10 is WIP. This is a part of the whole change (only ui/). No content/chrome callsites are updated but I hope this is enough to show how it works. Checked this on windows/mac/android/linux and confirmed targets under ui/ like ui/views ui/display build fine, and the test display_unittests passes. Let me know if there's something else you'd like to see. Once it reaches to the point where you feel comfortable, I'll revert the test patch from this CL, and use it for complete change in the follow up CL.
This looks good! I think you should continue with this until it is landable. I understand that's it's effectively the "follow-up" but - I don't see a path that would allow the follow-up to be split any further - The CL without the "follow-up" is simple enough (and effectively dead code), so doesn't need to be split out IMO - The changes in ui/views and ui/message_center are just mechanical - if you leave the content/scalefactor stuff unchanged, you may find there's actually not much else to change. But if - you have a plan for splitting the "follow-up" already, or - there's something orthogonal you can achieve with the change in patchset 9 to achieve a better split, or - there's some reason I've missed that the necessary changes under src/content become less than mechanical. then we may need to rethink things. I still don't like the idea of starting a mass rename based only of patchset 9: without the compiler enforcing that only gfx::NativeWindow goes to NearestWindow it will be easy to make mistakes. As for making things clear, I provided a template CL and said 'it needs "something" for Android' - that's more than you will get from most reviewers :) https://codereview.chromium.org/2688413007/diff/610001/ui/display/win/screen_... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2688413007/diff/610001/ui/display/win/screen_... ui/display/win/screen_win.cc:417: } This is identical to GetDisplayNearestWindow. Is there a configuration on windows where screen_aura.cc is not included in the build? I think we just need to rename GetDisplayNearestWindow -> GetDisplayNearestView above so that aura's WindowForView is used.
Will include rename changes here after cross-platform checks. I'm thinking of simplifying those scale factor calls in a separate patch to make this CL as small as possible. https://codereview.chromium.org/2688413007/diff/610001/ui/display/win/screen_... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2688413007/diff/610001/ui/display/win/screen_... ui/display/win/screen_win.cc:417: } On 2017/03/09 00:47:42, tapted wrote: > This is identical to GetDisplayNearestWindow. Is there a configuration on > windows where screen_aura.cc is not included in the build? I think we just need > to rename GetDisplayNearestWindow -> GetDisplayNearestView above so that aura's > WindowForView is used. Right this is not necessary since Windows always uses aura. Works fine without overriding this method. Will remove it in the next patch.
Approximately: - 80 files call GetDisplayNearestWindow (find . \( -name "*.cc" -o -name ".h" -o -name "*.mm" \) | xargs egrep -l 'GetDisplayNearestWindow\([^)]' | wc -l) - 20 files call GetDisplayNearestWindow for getting scale factor only. So about 60 files needs updating.
On 2017/03/09 05:31:11, Jinsuk Kim wrote: > Approximately: > - 80 files call GetDisplayNearestWindow (find . \( -name "*.cc" -o -name ".h" -o > -name "*.mm" \) | xargs egrep -l 'GetDisplayNearestWindow\([^)]' | wc -l) > > - 20 files call GetDisplayNearestWindow for getting scale factor only. > > So about 60 files needs updating. Note that not all of these need updating, and many of call sites are in test files. E.g., stuff under ash (37 files) is only compiled under aura - they can be ignored (at least in the first pass) - "Window" already works and is correct in most cases. Same for *chromeos* files (5), and *aura* files (12). screen_win stuff can probably be skipped too. I counted 95 files total (git grep -l 'GetDisplayNearestWindow([^)]'). 10 files are in ui/display. There's probably only 30 files that need updating in the first pass.
> E.g., stuff under ash (37 files) is only compiled under aura - they can be > ignored (at least in the first pass) - "Window" already works and is correct in > most cases. Same for *chromeos* files (5), and *aura* files (12). screen_win > stuff can probably be skipped too. > > I counted 95 files total (git grep -l 'GetDisplayNearestWindow([^)]'). 10 files > are in ui/display. There's probably only 30 files that need updating in the > first pass. Sounds good. In fact I also got the # of files not using aura among them(27) but didn't mention it :)
The CQ bit was checked by jinsukkim@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@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...
Patchset #12 (id:650001) has been deleted
The CQ bit was checked by jinsukkim@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.
patchset 13 lgtm but please update the cl description
Description was changed from ========== Add display::GetDisplayNearestView Add a method that gets the display with a given gfx::NativeView. The present API |display::GetDisplayNearestWindow| will be updated in a follow-up CL so that it accepts gfx::NativeWindow to match its signature. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add display::GetDisplayNearestView Add a method that gets the display with a given gfx::NativeView. The present API |display::GetDisplayNearestWindow| was also updated to accept gfx::NativeWindow to match its signature. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Add display::GetDisplayNearestView Add a method that gets the display with a given gfx::NativeView. The present API |display::GetDisplayNearestWindow| was also updated to accept gfx::NativeWindow to match its signature. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add display::GetDisplayNearestView Add a method that gets the display with a given gfx::NativeView. The present API |display::GetDisplayNearestWindow| was also updated to accept gfx::NativeWindow to match its signature. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=benwells@chromium.org,sky@chromium.org,boliu@chromium.org,msw@chromium.or... ==========
The CQ bit was checked by jinsukkim@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 jinsukkim@chromium.org
On 2017/03/14 10:32:30, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/14 09:17:51, tapted wrote: > patchset 13 lgtm but please update the cl description TBR'd lots of people - I suppose this kind of widespread, mechanical refactoring can be handled this way according to the guideline: https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md
On 2017/03/14 10:38:11, Jinsuk Kim wrote: > On 2017/03/14 09:17:51, tapted wrote: > > patchset 13 lgtm but please update the cl description > > TBR'd lots of people - I suppose this kind of widespread, mechanical refactoring > can be handled this way according to the guideline: > > https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md since oshima isn't in that list I'm assuming you're waiting for an lg from oshima@ (i.e. for a ui/display OWNER). You also need to add those to in TBR to the reviewers list (from that link, "Add the reviewer‘s email address in the code review tool’s reviewer field like normal.") and re-send email so they see the CL. But usually that's done after the API owners has given lg.
just one q. https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... File ui/display/screen_android.cc (right): https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... ui/display/screen_android.cc:16: NOTREACHED() << "Inherited class should override |GetDisplayNearestView|."; It's not clear what went wrong with this log message. I assume this shouldn't be called on Android, no?
Added those in TBR to revewers list too. https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... File ui/display/screen_android.cc (right): https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... ui/display/screen_android.cc:16: NOTREACHED() << "Inherited class should override |GetDisplayNearestView|."; On 2017/03/14 11:53:24, oshima wrote: > It's not clear what went wrong with this log message. > > I assume this shouldn't be called on Android, no? Correct. Android code here is just to make the build works. Android should not use this method to convert View -> Window for GEtDisplayNearestXXXX. Updated the log message. See if that's clearer.
On 2017/03/14 23:23:27, Jinsuk Kim wrote: > Added those in TBR to revewers list too. What is each person tbr'ing? > > https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... > File ui/display/screen_android.cc (right): > > https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... > ui/display/screen_android.cc:16: NOTREACHED() << "Inherited class should > override |GetDisplayNearestView|."; > On 2017/03/14 11:53:24, oshima wrote: > > It's not clear what went wrong with this log message. > > > > I assume this shouldn't be called on Android, no? > > Correct. Android code here is just to make the build works. > Android should not use this method to convert View -> Window > for GEtDisplayNearestXXXX. Updated the log message. See if > that's clearer.
On 2017/03/14 23:23:27, Jinsuk Kim wrote: > Added those in TBR to revewers list too. What is each person tbr'ing? > > https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... > File ui/display/screen_android.cc (right): > > https://codereview.chromium.org/2688413007/diff/690001/ui/display/screen_andr... > ui/display/screen_android.cc:16: NOTREACHED() << "Inherited class should > override |GetDisplayNearestView|."; > On 2017/03/14 11:53:24, oshima wrote: > > It's not clear what went wrong with this log message. > > > > I assume this shouldn't be called on Android, no? > > Correct. Android code here is just to make the build works. > Android should not use this method to convert View -> Window > for GEtDisplayNearestXXXX. Updated the log message. See if > that's clearer.
lgtm with nits (not sure what I was TBR'ed for, so I reviewed the whole thing...). Please do specify the responsibilities of each person you're asking for review. https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen.h#ne... ui/display/screen.h:99: static gfx::NativeWindow WindowForView(gfx::NativeView view); optional nit: GetWindowForView? https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen_andr... File ui/display/screen_android.cc (right): https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen_andr... ui/display/screen_android.cc:13: // Android cannot use implement this method here to convert |NativeView| nit: "use implement" grammar, missing something?
On 2017/03/14 23:29:21, benwells wrote: > On 2017/03/14 23:23:27, Jinsuk Kim wrote: > > Added those in TBR to revewers list too. > > What is each person tbr'ing? > Sorry for the confusion. First time to use TBR... I'm still waiting for lgtm from ui/display owner. Should have added you and the others after that. Feel free to comment or approve on the directories you're the owner of.
https://codereview.chromium.org/2688413007/diff/710001/chromecast/graphics/ca... File chromecast/graphics/cast_screen.h (right): https://codereview.chromium.org/2688413007/diff/710001/chromecast/graphics/ca... chromecast/graphics/cast_screen.h:32: gfx::NativeWindow window) const override; you'll need to update the cc file as well for this to compile?
On 2017/03/14 23:38:43, Jinsuk Kim wrote: > On 2017/03/14 23:29:21, benwells wrote: > > On 2017/03/14 23:23:27, Jinsuk Kim wrote: > > > Added those in TBR to revewers list too. > > > > What is each person tbr'ing? > > > > Sorry for the confusion. First time to use TBR... > I'm still waiting for lgtm from ui/display owner. > Should have added you and the others after that. > Feel free to comment or approve on the directories > you're the owner of. It's really helpful if you list the folders you want people to review. There is a lot of overlap in the OWNERS files, so listing what you want each person to review makes people's lives easier.
My comment goes for all similar places. Also, when you add a ton of reviewers please indicate what files you expect each reviewer to review. https://codereview.chromium.org/2688413007/diff/710001/ui/views/mus/screen_mus.h File ui/views/mus/screen_mus.h (right): https://codereview.chromium.org/2688413007/diff/710001/ui/views/mus/screen_mu... ui/views/mus/screen_mus.h:34: gfx::NativeWindow window) const override; Make sure the types and names in declaration and definition match (see style guide).
Patchset #15 (id:730001) has been deleted
Sorry again for the confusion. benwells@ ui/apps_list, extensions/ halliwell@ chromecast/ bshe@ chrome/browser/android/vr_shell msw@ chrome/browser/ui/views, components/constrained_window boliu@ contents/browser, ui/android alexclarke@ headless/ sky@ ui/aura, ui/base, ui/snapshot oshima@ ui/display tapted@ ui/views https://codereview.chromium.org/2688413007/diff/710001/chromecast/graphics/ca... File chromecast/graphics/cast_screen.h (right): https://codereview.chromium.org/2688413007/diff/710001/chromecast/graphics/ca... chromecast/graphics/cast_screen.h:32: gfx::NativeWindow window) const override; On 2017/03/14 23:46:44, halliwell wrote: > you'll need to update the cc file as well for this to compile? cc file already has the right signature. Build is okay (I guess aura is on in this case). Just fixed the mismatch between the h/cc. https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen.h#ne... ui/display/screen.h:99: static gfx::NativeWindow WindowForView(gfx::NativeView view); On 2017/03/14 23:38:13, msw wrote: > optional nit: GetWindowForView? Done. https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen_andr... File ui/display/screen_android.cc (right): https://codereview.chromium.org/2688413007/diff/710001/ui/display/screen_andr... ui/display/screen_android.cc:13: // Android cannot use implement this method here to convert |NativeView| On 2017/03/14 23:38:13, msw wrote: > nit: "use implement" grammar, missing something? Done. https://codereview.chromium.org/2688413007/diff/710001/ui/views/mus/screen_mus.h File ui/views/mus/screen_mus.h (right): https://codereview.chromium.org/2688413007/diff/710001/ui/views/mus/screen_mu... ui/views/mus/screen_mus.h:34: gfx::NativeWindow window) const override; On 2017/03/14 23:58:12, sky wrote: > Make sure the types and names in declaration and definition match (see style > guide). Updated cc accordingly. Checked other files in this CL to make sure they have the matched signature.
https://codereview.chromium.org/2688413007/diff/770001/ui/display/screen_andr... File ui/display/screen_android.cc (right): https://codereview.chromium.org/2688413007/diff/770001/ui/display/screen_andr... ui/display/screen_android.cc:17: "instance has correct implementation of the method " make sure to use the correct Screen instance that has proper implementation of |GetDisplayNearestView| for Android.
PTAL https://codereview.chromium.org/2688413007/diff/770001/ui/display/screen_andr... File ui/display/screen_android.cc (right): https://codereview.chromium.org/2688413007/diff/770001/ui/display/screen_andr... ui/display/screen_android.cc:17: "instance has correct implementation of the method " On 2017/03/15 01:38:48, oshima wrote: > make sure to use the correct Screen instance that has proper implementation of > |GetDisplayNearestView| for Android. Done.
https://codereview.chromium.org/2688413007/diff/770001/extensions/shell/brows... File extensions/shell/browser/shell_screen.h (right): https://codereview.chromium.org/2688413007/diff/770001/extensions/shell/brows... extensions/shell/browser/shell_screen.h:45: gfx::NativeWindow window) const override; Shouldn't there be a matching .cc change?
https://codereview.chromium.org/2688413007/diff/770001/extensions/shell/brows... File extensions/shell/browser/shell_screen.h (right): https://codereview.chromium.org/2688413007/diff/770001/extensions/shell/brows... extensions/shell/browser/shell_screen.h:45: gfx::NativeWindow window) const override; On 2017/03/15 01:59:38, benwells wrote: > Shouldn't there be a matching .cc change? cc file already has the right signature. Build is okay (I guess aura is on in this case). Cleaned up the code by fixing the mismatch between the h/cc.
contents/browser, ui/android lgtm
ui/display lgtm
The CQ bit was checked by jinsukkim@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.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2688413007/#ps790001 (title: "updated log #3")
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": 790001, "attempt_start_ts": 1489560325592760,
"parent_rev": "99c1f4221e4374df51ebb65b24be45400dff150a", "commit_rev":
"93bb5a29073d2cb41b20d3cc6cf59459a3906043"}
Message was sent while issue was closed.
Description was changed from ========== Add display::GetDisplayNearestView Add a method that gets the display with a given gfx::NativeView. The present API |display::GetDisplayNearestWindow| was also updated to accept gfx::NativeWindow to match its signature. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=benwells@chromium.org,sky@chromium.org,boliu@chromium.org,msw@chromium.or... ========== to ========== Add display::GetDisplayNearestView Add a method that gets the display with a given gfx::NativeView. The present API |display::GetDisplayNearestWindow| was also updated to accept gfx::NativeWindow to match its signature. BUG=671401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=benwells@chromium.org,sky@chromium.org,boliu@chromium.org,msw@chromium.or... Review-Url: https://codereview.chromium.org/2688413007 Cr-Commit-Position: refs/heads/master@{#457016} Committed: https://chromium.googlesource.com/chromium/src/+/93bb5a29073d2cb41b20d3cc6cf5... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:790001) as https://chromium.googlesource.com/chromium/src/+/93bb5a29073d2cb41b20d3cc6cf5...
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 457016 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:790001) has been created in https://codereview.chromium.org/2747143005/ by tzik@chromium.org. The reason for reverting is: This CL causes a compile failure on a Mac bot: https://build.chromium.org/p/chromium/builders/Mac/builds/24801 [38396/49613] CXX obj/extensions/browser/browser_tests/system_display_apitest.o FAILED: obj/extensions/browser/browser_tests/system_display_apitest.o (snip) ../../extensions/browser/api/system_display/system_display_apitest.cc:58:37: error: non-virtual member function marked 'override' hides virtual member function gfx::NativeView window) const override { ^ ../../ui/display/screen.h:61:19: note: hidden overloaded virtual function 'display::Screen::GetDisplayNearestWindow' declared here: type mismatch at 1st parameter ('gfx::NativeWindow' (aka 'NSWindow *') vs 'gfx::NativeView' (aka 'NSView *')) virtual Display GetDisplayNearestWindow(gfx::NativeWindow window) const = 0; ^ ../../extensions/browser/api/system_display/system_display_apitest.cc:200:61: error: allocating an object of abstract class type 'extensions::MockScreen' : provider_(new MockDisplayInfoProvider), screen_(new MockScreen) {} ^ ../../ui/display/screen.h:61:19: note: unimplemented pure virtual method 'GetDisplayNearestWindow' in 'MockScreen' virtual Display GetDisplayNearestWindow(gfx::NativeWindow window) const = 0; ^ 2 errors generated.. |
