Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042903002:
Support app window draggable areas in mash. (Closed)
|
Created:
4 years, 6 months ago by msw Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, sky Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport app window draggable areas in mash.
Some Chrome app windows use custom frames, with draggable regions.
Support NonClientView-powered window dragging without a standard frame.
Add a new 'RemoveStandardFrame' mus window property and helper function.
Add a new EmptyDraggableNonClientFrameView for hit testing without painting.
(parallels existing behavior w/Widget::InitParams::remove_standard_frame)
(avoids painting a frame, still handles non-client events/captures/drags)
Override UpdateDraggableRegions for mash app windows to:
- Specify insets that cover all draggable regions.
- Invert the region to specify "additional client areas".
BUG=617302
TEST=Wallpaper picker extensions window can be dragged in mash.
R=jamescook@chromium.org
Committed: https://crrev.com/b8d01e3bfa7e5febe399d48709805adf1259b800
Cr-Commit-Position: refs/heads/master@{#400305}
Patch Set 1 #Patch Set 2 : First crack at implementing region struct and traits. #Patch Set 3 : Sync and rebase. #Patch Set 4 : Compiling with WindowRegion struct change... #Patch Set 5 : Wallpaper app dragging is working. #Patch Set 6 : Partial cleanup. #Patch Set 7 : Favor std::vector over mojo::Array. #Patch Set 8 : Use insets and inverted draggable region with a stub frame view. #Patch Set 9 : Cleanup. #
Total comments: 13
Patch Set 10 : Address comments. #
Total comments: 4
Messages
Total messages: 37 (15 generated)
Description was changed from ========== Support app window draggable areas in mash. BUG=617302 ========== to ========== Support app window draggable areas in mash. Some Chrome app windows use custom frames, with draggable regions. Support NonClientView-powered window dragging without a standard frame. Add a new 'RemoveStandardFrame' mus window property and helper function. Respect this custom frame behavior with a new StubNonClientFrameView. (parallels existing behavior w/Widget::InitParams::remove_standard_frame) (avoids painting a frame, still handles non-client events/captures/drags) Override UpdateDraggableRegions for mash app windows to: - Specify insets that cover all draggable regions. - Invert the region to specify "additional client areas". BUG=617302 TEST=Wallpaper picker extensions window can be dragged in mash. R=jamescook@chromium.org ==========
msw@chromium.org changed reviewers: + jamescook@chromium.org
Hey James, please take a look; thanks!
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042903002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... File ash/mus/non_client_frame_controller.cc (right): https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... ash/mus/non_client_frame_controller.cc:88: // This class supports draggable regions for windows with non-standard frames. Nice class comment. optional: Maybe cite an example of one of these windows? and/or clarify that "non-standard frames" usually means that the window draws its own frame, like some packaged apps? https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... ash/mus/non_client_frame_controller.cc:90: class StubNonClientFrameView : public views::NonClientFrameView { optional: I wish there was a better name for this. EmptyNonClientFrameView? Or failing that, NonClientFrameViewStub? AlwaysDragNonClientFrameView? Dunno. https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:148: ash::Shell::GetContainer(ash::Shell::GetPrimaryRootWindow(), aside: This is going to need to get fixed someday to use WmRootWindowController::ConfigureWidgetInitParamsForContainer. But today is not that day. https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:337: gfx::Insets insets(GetDraggableRegion()->getBounds().bottom(), 0, 0, 0); Can GetDraggableRegion return null? I think it can in ShellNativeAppWindow::GetDraggableRegion() and we might resuscitate app_shell for mus ash. https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:341: inverted_region.setRect(GetDraggableRegion()->getBounds()); optional: Cache GetDraggableRegion result, since you use it 3 times? https://codereview.chromium.org/2042903002/diff/160001/components/mus/public/... File components/mus/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2042903002/diff/160001/components/mus/public/... components/mus/public/interfaces/window_manager.mojom:53: // A flag controlling the presence of standard frame components. Type: bool I would duplicate the comment from ui/views/widget.h initparams.
msw@chromium.org changed reviewers: + ben@chromium.org, tapted@chromium.org
Hey James, please take another look; thanks! +tapted for c/b/ui/views/apps/OWNERS +ben for components/mus/OWNERS https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... File ash/mus/non_client_frame_controller.cc (right): https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... ash/mus/non_client_frame_controller.cc:88: // This class supports draggable regions for windows with non-standard frames. On 2016/06/14 23:44:56, James Cook wrote: > Nice class comment. > > optional: Maybe cite an example of one of these windows? > > and/or clarify that "non-standard frames" usually means that the window draws > its own frame, like some packaged apps? Done. https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... ash/mus/non_client_frame_controller.cc:90: class StubNonClientFrameView : public views::NonClientFrameView { On 2016/06/14 23:44:56, James Cook wrote: > optional: I wish there was a better name for this. EmptyNonClientFrameView? Or > failing that, NonClientFrameViewStub? AlwaysDragNonClientFrameView? Dunno. Went with EmptyDraggableNonClientFrameView, but I'm open to revision. https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:148: ash::Shell::GetContainer(ash::Shell::GetPrimaryRootWindow(), On 2016/06/14 23:44:56, James Cook wrote: > aside: This is going to need to get fixed someday to use > WmRootWindowController::ConfigureWidgetInitParamsForContainer. But today is not > that day. Acknowledged. https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:337: gfx::Insets insets(GetDraggableRegion()->getBounds().bottom(), 0, 0, 0); On 2016/06/14 23:44:56, James Cook wrote: > Can GetDraggableRegion return null? I think it can in > ShellNativeAppWindow::GetDraggableRegion() and we might resuscitate app_shell > for mus ash. Done. https://codereview.chromium.org/2042903002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:341: inverted_region.setRect(GetDraggableRegion()->getBounds()); On 2016/06/14 23:44:56, James Cook wrote: > optional: Cache GetDraggableRegion result, since you use it 3 times? Done. https://codereview.chromium.org/2042903002/diff/160001/components/mus/public/... File components/mus/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2042903002/diff/160001/components/mus/public/... components/mus/public/interfaces/window_manager.mojom:53: // A flag controlling the presence of standard frame components. Type: bool On 2016/06/14 23:44:56, James Cook wrote: > I would duplicate the comment from ui/views/widget.h initparams. Done.
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042903002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Nice patch. https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... File ash/mus/non_client_frame_controller.cc (right): https://codereview.chromium.org/2042903002/diff/160001/ash/mus/non_client_fra... ash/mus/non_client_frame_controller.cc:90: class StubNonClientFrameView : public views::NonClientFrameView { On 2016/06/15 17:06:40, msw wrote: > On 2016/06/14 23:44:56, James Cook wrote: > > optional: I wish there was a better name for this. EmptyNonClientFrameView? Or > > failing that, NonClientFrameViewStub? AlwaysDragNonClientFrameView? Dunno. > > Went with EmptyDraggableNonClientFrameView, but I'm open to revision. SGTM
c/b/ui/views/apps lgtm (only a CL description nit - it still mentions the old name, StubNonClientFrameView)
mus lgtm
Description was changed from ========== Support app window draggable areas in mash. Some Chrome app windows use custom frames, with draggable regions. Support NonClientView-powered window dragging without a standard frame. Add a new 'RemoveStandardFrame' mus window property and helper function. Respect this custom frame behavior with a new StubNonClientFrameView. (parallels existing behavior w/Widget::InitParams::remove_standard_frame) (avoids painting a frame, still handles non-client events/captures/drags) Override UpdateDraggableRegions for mash app windows to: - Specify insets that cover all draggable regions. - Invert the region to specify "additional client areas". BUG=617302 TEST=Wallpaper picker extensions window can be dragged in mash. R=jamescook@chromium.org ========== to ========== Support app window draggable areas in mash. Some Chrome app windows use custom frames, with draggable regions. Support NonClientView-powered window dragging without a standard frame. Add a new 'RemoveStandardFrame' mus window property and helper function. Add a new EmptyDraggableNonClientFrameView for hit testing without painting. (parallels existing behavior w/Widget::InitParams::remove_standard_frame) (avoids painting a frame, still handles non-client events/captures/drags) Override UpdateDraggableRegions for mash app windows to: - Specify insets that cover all draggable regions. - Invert the region to specify "additional client areas". BUG=617302 TEST=Wallpaper picker extensions window can be dragged in mash. R=jamescook@chromium.org ==========
The CQ bit was checked by msw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042903002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
+dcheng for window_manager.mojom security review.
msw@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for window_manager.mojom security review.
https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:154: mojo::ConvertTo<std::vector<uint8_t>>(init_params->remove_standard_frame); How does a bool turn into a vector of bytes? (Am I interpreting this correctly?) More generically... why do we need to do this? Why is it defined in mus' property_type_converters.h and not somewhere else? It seems like a recipe for violating ODR.
https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:154: mojo::ConvertTo<std::vector<uint8_t>>(init_params->remove_standard_frame); On 2016/06/16 09:04:36, dcheng wrote: > How does a bool turn into a vector of bytes? > (Am I interpreting this correctly?) Yes, see the impl here: https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/property_t... > More generically... why do we need to do this? We keep a map of named std::vector<uint8_t> properties on the mus::Window, shared across the services that use mus::Window: https://cs.chromium.org/chromium/src/components/mus/public/cpp/window.h?rcl=0... This is a similar pattern to aura::Window's map of templated WindowProperty structs: https://cs.chromium.org/chromium/src/ui/aura/window.h?rcl=1466075871&l=304 This CL demonstrates one use case: informing the WM not to paint a frame, but to still support draggable areas. > Why is it defined in mus' > property_type_converters.h and not somewhere else? It seems like a recipe for > violating ODR. It seems natural that the converters for properties set on mus windows would be part of the mus public cpp client library.
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:154: mojo::ConvertTo<std::vector<uint8_t>>(init_params->remove_standard_frame); On 2016/06/16 09:04:36, dcheng wrote: > How does a bool turn into a vector of bytes? > (Am I interpreting this correctly?) > > More generically... why do we need to do this? Why is it defined in mus' > property_type_converters.h and not somewhere else? It seems like a recipe for > violating ODR. property_type_converters contains common types used by most clients of mus. It won't contain all converters, but certainly the common ones.
https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2042903002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:154: mojo::ConvertTo<std::vector<uint8_t>>(init_params->remove_standard_frame); On 2016/06/16 16:54:21, msw wrote: > On 2016/06/16 09:04:36, dcheng wrote: > > How does a bool turn into a vector of bytes? > > (Am I interpreting this correctly?) > Yes, see the impl here: > https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/property_t... > > > More generically... why do we need to do this? > > We keep a map of named std::vector<uint8_t> properties on the mus::Window, > shared across the services that use mus::Window: > https://cs.chromium.org/chromium/src/components/mus/public/cpp/window.h?rcl=0... > This is a similar pattern to aura::Window's map of templated WindowProperty > structs: > https://cs.chromium.org/chromium/src/ui/aura/window.h?rcl=1466075871&l=304 > This CL demonstrates one use case: informing the WM not to paint a frame, but to > still support draggable areas. > > > Why is it defined in mus' > > property_type_converters.h and not somewhere else? It seems like a recipe for > > violating ODR. > > It seems natural that the converters for properties set on mus windows would be > part of the mus public cpp client library. Right, but there's nothing mus-specific about the templates parameters in this particular case. If someone ever defines the TypeConverter template in another translation unit with the same template arguments, but defines the body differently, then we have an ODR violation. More generally... why type converter? We do want to remove these, now that we have struct traits. Maybe these should just be mus utilities (BoolToWindowProperty), etc?
lgtm
The CQ bit was checked by msw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042903002/180001
Message was sent while issue was closed.
Description was changed from ========== Support app window draggable areas in mash. Some Chrome app windows use custom frames, with draggable regions. Support NonClientView-powered window dragging without a standard frame. Add a new 'RemoveStandardFrame' mus window property and helper function. Add a new EmptyDraggableNonClientFrameView for hit testing without painting. (parallels existing behavior w/Widget::InitParams::remove_standard_frame) (avoids painting a frame, still handles non-client events/captures/drags) Override UpdateDraggableRegions for mash app windows to: - Specify insets that cover all draggable regions. - Invert the region to specify "additional client areas". BUG=617302 TEST=Wallpaper picker extensions window can be dragged in mash. R=jamescook@chromium.org ========== to ========== Support app window draggable areas in mash. Some Chrome app windows use custom frames, with draggable regions. Support NonClientView-powered window dragging without a standard frame. Add a new 'RemoveStandardFrame' mus window property and helper function. Add a new EmptyDraggableNonClientFrameView for hit testing without painting. (parallels existing behavior w/Widget::InitParams::remove_standard_frame) (avoids painting a frame, still handles non-client events/captures/drags) Override UpdateDraggableRegions for mash app windows to: - Specify insets that cover all draggable regions. - Invert the region to specify "additional client areas". BUG=617302 TEST=Wallpaper picker extensions window can be dragged in mash. R=jamescook@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Support app window draggable areas in mash. Some Chrome app windows use custom frames, with draggable regions. Support NonClientView-powered window dragging without a standard frame. Add a new 'RemoveStandardFrame' mus window property and helper function. Add a new EmptyDraggableNonClientFrameView for hit testing without painting. (parallels existing behavior w/Widget::InitParams::remove_standard_frame) (avoids painting a frame, still handles non-client events/captures/drags) Override UpdateDraggableRegions for mash app windows to: - Specify insets that cover all draggable regions. - Invert the region to specify "additional client areas". BUG=617302 TEST=Wallpaper picker extensions window can be dragged in mash. R=jamescook@chromium.org ========== to ========== Support app window draggable areas in mash. Some Chrome app windows use custom frames, with draggable regions. Support NonClientView-powered window dragging without a standard frame. Add a new 'RemoveStandardFrame' mus window property and helper function. Add a new EmptyDraggableNonClientFrameView for hit testing without painting. (parallels existing behavior w/Widget::InitParams::remove_standard_frame) (avoids painting a frame, still handles non-client events/captures/drags) Override UpdateDraggableRegions for mash app windows to: - Specify insets that cover all draggable regions. - Invert the region to specify "additional client areas". BUG=617302 TEST=Wallpaper picker extensions window can be dragged in mash. R=jamescook@chromium.org Committed: https://crrev.com/b8d01e3bfa7e5febe399d48709805adf1259b800 Cr-Commit-Position: refs/heads/master@{#400305} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b8d01e3bfa7e5febe399d48709805adf1259b800 Cr-Commit-Position: refs/heads/master@{#400305} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
