Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341483004:
Fix event dispatching and window resizing in high-dpi mode. (Closed)
DescriptionFix event dispatching and window resizing in high-dpi mode.
TODO:
1. Currently there are no hover effects for backwards, forwards, refresh
and app menu button.
2. Minimize, Maximize and Close buttons are not receiving events in the
bottom half area, which will be fixed in another CL by changing client_area.
BUG=646926, 646927
TEST=manual (with --force-device-scale-factor=2)
Committed: https://crrev.com/8ee02a33407c0456fcd4dd4ee919f66c12a88669
Cr-Commit-Position: refs/heads/master@{#424542}
Patch Set 1 #Patch Set 2 : client area #
Total comments: 2
Patch Set 3 : common function #Patch Set 4 : remove client area changes #
Total comments: 2
Messages
Total messages: 41 (22 generated)
The CQ bit was checked by riajiang@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 #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== . BUG= ========== to ========== Fix event dispatching in high-dpi mode. Currently there are no hover effects for backwards, forwards, refresh and app menu button. BUG=646926 TEST=mus_public_unittests + manual (with --force-device-scale-factor=2) ==========
riajiang@chromium.org changed reviewers: + sadrul@chromium.org
Description was changed from ========== Fix event dispatching in high-dpi mode. Currently there are no hover effects for backwards, forwards, refresh and app menu button. BUG=646926 TEST=mus_public_unittests + manual (with --force-device-scale-factor=2) ========== to ========== Fix event dispatching in high-dpi mode. Currently there are no hover effects for backwards, forwards, refresh and app menu button. BUG=646926 TEST=mus_public_unittests + manual (with --force-device-scale-factor=2) ==========
riajiang@chromium.org changed reviewers: + sky@chromium.org
PTAL. Thanks!
I only looked at this briefly. It seems that you are wanting to treat some values as pixels and others as dips. As with aura and views we should assume all units in the API are dips unless otherwise noted. For the record I prefer pixels everywhere, but that isn't what we have with aura and views, so for consistency we should go with dips here too. https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... File services/ui/public/cpp/window_tree_client.cc (right): https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... services/ui/public/cpp/window_tree_client.cc:919: // TODO(riajiang): Change to use display::GetDisplayWithDisplayId() after You could at least make this code use a common function.
On 2016/10/06 16:38:13, sky wrote: > I only looked at this briefly. It seems that you are wanting to treat some > values as pixels and others as dips. As with aura and views we should assume all > units in the API are dips unless otherwise noted. For the record I prefer pixels > everywhere, but that isn't what we have with aura and views, so for consistency > we should go with dips here too. It is my understanding that the mus ws will talk in pixels and the client lib is responsible for the conversion between pixels and dips? Since SetClientArea is called from client app to set the value in ws, the client lib was converting dips to pixels and since OnClientAreaChanged is called by ws, the client lib would convert that back to dips? https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... File services/ui/public/cpp/window_tree_client.cc (right): https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... services/ui/public/cpp/window_tree_client.cc:919: // TODO(riajiang): Change to use display::GetDisplayWithDisplayId() after On 2016/10/06 16:38:13, sky wrote: > You could at least make this code use a common function. Yes! Done.
Description was changed from ========== Fix event dispatching in high-dpi mode. Currently there are no hover effects for backwards, forwards, refresh and app menu button. BUG=646926 TEST=mus_public_unittests + manual (with --force-device-scale-factor=2) ========== to ========== Fix event dispatching and window resizing in high-dpi mode. Currently there are no hover effects for backwards, forwards, refresh and app menu button. BUG=646926, 646927 TEST=mus_public_unittests + manual (with --force-device-scale-factor=2) ==========
sky@chromium.org changed reviewers: + rjkroege@chromium.org
+rjkroege convinced me the ws really needs to speak both, otherwise it has to be implemented in every language we want to use mus from. I would be fine with dip only as a stepping stone as it seems as though dips are going to be here in the near future. Of course you'll have to make sure that would work for the renderer side of things. I'm not sure what the renderer wants. None-the-less I don't understand what's different about client area in your patch that you're only converting it. All bounds/size would have to be converted too, right? Is this just a conversion of one place?
On 2016/10/06 21:54:29, sky wrote: > +rjkroege convinced me the ws really needs to speak both, otherwise it has to be > implemented in every language we want to use mus from. I think for a fishy-milestone, we can live with doing conversions in the c++ client lib? :) > > I would be fine with dip only as a stepping stone as it seems as though dips are > going to be here in the near future. Of course you'll have to make sure that > would work for the renderer side of things. I'm not sure what the renderer > wants. The renderer gets the events in dip right now. So this should be OK. > None-the-less I don't understand what's different about client area in > your patch that you're only converting it. All bounds/size would have to be > converted too, right? Is this just a conversion of one place? Yes, this looks a bit weird here. Let's move the client-area conversion in the patch you are working on for fixing bounds etc.
How is the renderer getting dips now? On Thu, Oct 6, 2016 at 6:47 PM, <sadrul@chromium.org> wrote: > On 2016/10/06 21:54:29, sky wrote: >> +rjkroege convinced me the ws really needs to speak both, otherwise it has >> to > be >> implemented in every language we want to use mus from. > > I think for a fishy-milestone, we can live with doing conversions in the c++ > client lib? :) > >> >> I would be fine with dip only as a stepping stone as it seems as though >> dips > are >> going to be here in the near future. Of course you'll have to make sure >> that >> would work for the renderer side of things. I'm not sure what the renderer >> wants. > > The renderer gets the events in dip right now. So this should be OK. > >> None-the-less I don't understand what's different about client area in >> your patch that you're only converting it. All bounds/size would have to >> be >> converted too, right? Is this just a conversion of one place? > > Yes, this looks a bit weird here. Let's move the client-area conversion in > the > patch you are working on for fixing bounds etc. > > https://codereview.chromium.org/2341483004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/07 03:17:26, sky wrote: > How is the renderer getting dips now? Ooh, sorry, you meant with mus. No, with mus, the renderer gets physical pixels right now. But in chrome proper, the renderer gets dip. In mus with > > On Thu, Oct 6, 2016 at 6:47 PM, <mailto:sadrul@chromium.org> wrote: > > On 2016/10/06 21:54:29, sky wrote: > >> +rjkroege convinced me the ws really needs to speak both, otherwise it has > >> to > > be > >> implemented in every language we want to use mus from. > > > > I think for a fishy-milestone, we can live with doing conversions in the c++ > > client lib? :) > > > >> > >> I would be fine with dip only as a stepping stone as it seems as though > >> dips > > are > >> going to be here in the near future. Of course you'll have to make sure > >> that > >> would work for the renderer side of things. I'm not sure what the renderer > >> wants. > > > > The renderer gets the events in dip right now. So this should be OK. > > > >> None-the-less I don't understand what's different about client area in > >> your patch that you're only converting it. All bounds/size would have to > >> be > >> converted too, right? Is this just a conversion of one place? > > > > Yes, this looks a bit weird here. Let's move the client-area conversion in > > the > > patch you are working on for fixing bounds etc. > > > > https://codereview.chromium.org/2341483004/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
On 2016/10/07 03:17:26, sky wrote: > How is the renderer getting dips now? Ooh, sorry, you meant with mus. No, with mus, the renderer gets physical pixels right now. But once the browser is fixed to receive events in dip, the renderer will get events in dip too (because the events to the renderer go through the browser, even with mus). > > On Thu, Oct 6, 2016 at 6:47 PM, <mailto:sadrul@chromium.org> wrote: > > On 2016/10/06 21:54:29, sky wrote: > >> +rjkroege convinced me the ws really needs to speak both, otherwise it has > >> to > > be > >> implemented in every language we want to use mus from. > > > > I think for a fishy-milestone, we can live with doing conversions in the c++ > > client lib? :) > > > >> > >> I would be fine with dip only as a stepping stone as it seems as though > >> dips > > are > >> going to be here in the near future. Of course you'll have to make sure > >> that > >> would work for the renderer side of things. I'm not sure what the renderer > >> wants. > > > > The renderer gets the events in dip right now. So this should be OK. > > > >> None-the-less I don't understand what's different about client area in > >> your patch that you're only converting it. All bounds/size would have to > >> be > >> converted too, right? Is this just a conversion of one place? > > > > Yes, this looks a bit weird here. Let's move the client-area conversion in > > the > > patch you are working on for fixing bounds etc. > > > > https://codereview.chromium.org/2341483004/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
Description was changed from ========== Fix event dispatching and window resizing in high-dpi mode. Currently there are no hover effects for backwards, forwards, refresh and app menu button. BUG=646926, 646927 TEST=mus_public_unittests + manual (with --force-device-scale-factor=2) ========== to ========== Fix event dispatching and window resizing in high-dpi mode. TODO: 1. Currently there are no hover effects for backwards, forwards, refresh and app menu button. 2. Minimize, Maximize and Close buttons are not receiving events in the bottom half area, which will be fixed in another CL by changing client_area. BUG=646926, 646927 TEST=manual (with --force-device-scale-factor=2) ==========
Description was changed from ========== Fix event dispatching and window resizing in high-dpi mode. TODO: 1. Currently there are no hover effects for backwards, forwards, refresh and app menu button. 2. Minimize, Maximize and Close buttons are not receiving events in the bottom half area, which will be fixed in another CL by changing client_area. BUG=646926, 646927 TEST=manual (with --force-device-scale-factor=2) ========== to ========== Fix event dispatching and window resizing in high-dpi mode. TODO: 1. Currently there are no hover effects for backwards, forwards, refresh and app menu button. 2. Minimize, Maximize and Close buttons are not receiving events in the bottom half area, which will be fixed in another CL by changing client_area. BUG=646926, 646927 TEST=manual (with --force-device-scale-factor=2) ==========
I removed the changes about client_area in WindowTreeClient from this CL and added a TODO in the description for now.
Thanks for the clarification Sadrul. Ria, can you clarify WindowTreeHostMus to disable the transform?
https://codereview.chromium.org/2341483004/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2341483004/diff/100001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1804: located_event->set_root_location_f(location_in_pixel_in_host); is set_root_location_f in DIP or DDP? In screen space? event.h doesn't say. But probably should (maybe in a different CL?)
On 2016/10/06 16:38:13, sky wrote: > I only looked at this briefly. It seems that you are wanting to treat some > values as pixels and others as dips. As with aura and views we should assume all > units in the API are dips unless otherwise noted. For the record I prefer pixels > everywhere, but that isn't what we have with aura and views, so for consistency > we should go with dips here too. It's perhaps idealistic of me but... (and not in this CL) but is there any reason not to have different types for DIP and physical pixels so that the compiler could detect attempts to inadvertently assign between them? And function signatures could indicate the expected size? sky@: wdyt > > https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... > File services/ui/public/cpp/window_tree_client.cc (right): > > https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... > services/ui/public/cpp/window_tree_client.cc:919: // TODO(riajiang): Change to > use display::GetDisplayWithDisplayId() after > You could at least make this code use a common function.
Rob, I think you are going to get real types for dips and pixels soon. -Scott On Mon, Oct 10, 2016 at 1:54 PM, <rjkroege@chromium.org> wrote: > On 2016/10/06 16:38:13, sky wrote: >> I only looked at this briefly. It seems that you are wanting to treat some >> values as pixels and others as dips. As with aura and views we should >> assume > all >> units in the API are dips unless otherwise noted. For the record I prefer > pixels >> everywhere, but that isn't what we have with aura and views, so for > consistency >> we should go with dips here too. > > It's perhaps idealistic of me but... (and not in this CL) but is there any > reason not to have different types for DIP and physical pixels so that the > compiler could detect attempts to inadvertently assign between them? And > function signatures could indicate the expected size? sky@: wdyt > >> >> > https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... >> File services/ui/public/cpp/window_tree_client.cc (right): >> >> > https://codereview.chromium.org/2341483004/diff/60001/services/ui/public/cpp/... >> services/ui/public/cpp/window_tree_client.cc:919: // TODO(riajiang): >> Change to >> use display::GetDisplayWithDisplayId() after >> You could at least make this code use a common function. > > > > https://codereview.chromium.org/2341483004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2341483004/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2341483004/diff/100001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1804: located_event->set_root_location_f(location_in_pixel_in_host); On 2016/10/10 20:49:31, rjkroege wrote: > is set_root_location_f in DIP or DDP? In screen space? event.h doesn't say. But > probably should (maybe in a different CL?) crbug.com/608547 is for this specific case, to replace 'root location' with a 'system location', and better define what exactly it means. For now, unfortunately, this 'root location' can be in physical pixel space in some places (e.g. immediately when the event is created from a native event), and in DIP in other places (e.g. during the dispatch phase). So there isn't a good way to clarify this with a comment in event.h In this specific case, however, the event location is in physical pixel space (which is why the local var-name has '_in_pixel_' in it)
On 2016/10/07 16:34:29, sky wrote: > Thanks for the clarification Sadrul. > > Ria, can you clarify WindowTreeHostMus to disable the transform? I think we were not considering device scale factor when we first call set_transform_events(false) in WTHM (your TODO)? We don't set the transform to be false anymore because we want to do TransformEventForDeviceScaleFactor in aura::WindowEventDispatcher. This was from this CL: https://codereview.chromium.org/1906603002. It was failing a DragDropTest but not anymore.
LGTM
The CQ bit was checked by riajiang@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 riajiang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix event dispatching and window resizing in high-dpi mode. TODO: 1. Currently there are no hover effects for backwards, forwards, refresh and app menu button. 2. Minimize, Maximize and Close buttons are not receiving events in the bottom half area, which will be fixed in another CL by changing client_area. BUG=646926, 646927 TEST=manual (with --force-device-scale-factor=2) ========== to ========== Fix event dispatching and window resizing in high-dpi mode. TODO: 1. Currently there are no hover effects for backwards, forwards, refresh and app menu button. 2. Minimize, Maximize and Close buttons are not receiving events in the bottom half area, which will be fixed in another CL by changing client_area. BUG=646926, 646927 TEST=manual (with --force-device-scale-factor=2) ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix event dispatching and window resizing in high-dpi mode. TODO: 1. Currently there are no hover effects for backwards, forwards, refresh and app menu button. 2. Minimize, Maximize and Close buttons are not receiving events in the bottom half area, which will be fixed in another CL by changing client_area. BUG=646926, 646927 TEST=manual (with --force-device-scale-factor=2) ========== to ========== Fix event dispatching and window resizing in high-dpi mode. TODO: 1. Currently there are no hover effects for backwards, forwards, refresh and app menu button. 2. Minimize, Maximize and Close buttons are not receiving events in the bottom half area, which will be fixed in another CL by changing client_area. BUG=646926, 646927 TEST=manual (with --force-device-scale-factor=2) Committed: https://crrev.com/8ee02a33407c0456fcd4dd4ee919f66c12a88669 Cr-Commit-Position: refs/heads/master@{#424542} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8ee02a33407c0456fcd4dd4ee919f66c12a88669 Cr-Commit-Position: refs/heads/master@{#424542}
Message was sent while issue was closed.
Patchset #5 (id:120001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
