Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
551883002:
Deactivate touch selection on transform (Closed)
|
Created:
6 years, 3 months ago by mohsen Modified:
6 years, 3 months ago Reviewers:
sky CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDeactivate touch selection on transform
When a window is being transformed, position of touch selection handles
might not be valid anymore, hence deactivating touch selection.
BUG=407659
Committed: https://crrev.com/584566f50ab083374216a6125709efc72f245dfd
Cr-Commit-Position: refs/heads/master@{#294429}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Used the new propagated transformed event #
Messages
Total messages: 15 (2 generated)
mohsen@chromium.org changed reviewers: + sky@chromium.org
Please take a look...
https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_aura.cc:438: for (; window; window = window->parent()) This is going to be fragile and easy to get wrong. I think it better to change aura to propagate transform changes to descendants. If you could do that in a separate patch with test coverage that would be great.
https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_aura.cc:438: for (; window; window = window->parent()) On 2014/09/08 20:24:56, sky wrote: > This is going to be fragile and easy to get wrong. I think it better to change > aura to propagate transform changes to descendants. If you could do that in a > separate patch with test coverage that would be great. Isn't it going to be costly? E.g. if a transform is applied to the root window, lots of windows are going to be notified.
How often do transforms change? We already do this sort of notifications for some things, like modifying the hierarchy. -Scott On Tue, Sep 9, 2014 at 2:36 PM, <mohsen@chromium.org> wrote: > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_widget_host_view_aura.cc > (right): > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_widget_host_view_aura.cc:438: for > (; window; window = window->parent()) > On 2014/09/08 20:24:56, sky wrote: >> >> This is going to be fragile and easy to get wrong. I think it better > > to change >> >> aura to propagate transform changes to descendants. If you could do > > that in a >> >> separate patch with test coverage that would be great. > > > Isn't it going to be costly? E.g. if a transform is applied to the root > window, lots of windows are going to be notified. > > https://codereview.chromium.org/551883002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/09 23:31:21, sky wrote: > How often do transforms change? We already do this sort of > notifications for some things, like modifying the hierarchy. In Athena for example, when user drags title of a window, a transform is applied to the window on each scroll-update. > > -Scott > > On Tue, Sep 9, 2014 at 2:36 PM, <mailto:mohsen@chromium.org> wrote: > > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > > File content/browser/renderer_host/render_widget_host_view_aura.cc > > (right): > > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > > content/browser/renderer_host/render_widget_host_view_aura.cc:438: for > > (; window; window = window->parent()) > > On 2014/09/08 20:24:56, sky wrote: > >> > >> This is going to be fragile and easy to get wrong. I think it better > > > > to change > >> > >> aura to propagate transform changes to descendants. If you could do > > > > that in a > >> > >> separate patch with test coverage that would be great. > > > > > > Isn't it going to be costly? E.g. if a transform is applied to the root > > window, lots of windows are going to be notified. > > > > https://codereview.chromium.org/551883002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I'm still going to say this likely doesn't matter. How much children does the window have? On Tue, Sep 9, 2014 at 4:41 PM, <mohsen@chromium.org> wrote: > On 2014/09/09 23:31:21, sky wrote: >> >> How often do transforms change? We already do this sort of >> notifications for some things, like modifying the hierarchy. > > > In Athena for example, when user drags title of a window, a transform is > applied > to the window on each scroll-update. > > >> -Scott > > >> On Tue, Sep 9, 2014 at 2:36 PM, <mailto:mohsen@chromium.org> wrote: >> > >> > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... >> >> > File content/browser/renderer_host/render_widget_host_view_aura.cc >> > (right): >> > >> > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... >> >> > content/browser/renderer_host/render_widget_host_view_aura.cc:438: for >> > (; window; window = window->parent()) >> > On 2014/09/08 20:24:56, sky wrote: >> >> >> >> This is going to be fragile and easy to get wrong. I think it better >> > >> > to change >> >> >> >> aura to propagate transform changes to descendants. If you could do >> > >> > that in a >> >> >> >> separate patch with test coverage that would be great. >> > >> > >> > Isn't it going to be costly? E.g. if a transform is applied to the root >> > window, lots of windows are going to be notified. >> > >> > https://codereview.chromium.org/551883002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/551883002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/09 23:57:18, sky wrote: > I'm still going to say this likely doesn't matter. How much children > does the window have? In the Athena example, not much; probably two or three children. I was just worried about the general case. > > On Tue, Sep 9, 2014 at 4:41 PM, <mailto:mohsen@chromium.org> wrote: > > On 2014/09/09 23:31:21, sky wrote: > >> > >> How often do transforms change? We already do this sort of > >> notifications for some things, like modifying the hierarchy. > > > > > > In Athena for example, when user drags title of a window, a transform is > > applied > > to the window on each scroll-update. > > > > > >> -Scott > > > > > >> On Tue, Sep 9, 2014 at 2:36 PM, <mailto:mohsen@chromium.org> wrote: > >> > > >> > > > > > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > >> > >> > File content/browser/renderer_host/render_widget_host_view_aura.cc > >> > (right): > >> > > >> > > > > > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > >> > >> > content/browser/renderer_host/render_widget_host_view_aura.cc:438: for > >> > (; window; window = window->parent()) > >> > On 2014/09/08 20:24:56, sky wrote: > >> >> > >> >> This is going to be fragile and easy to get wrong. I think it better > >> > > >> > to change > >> >> > >> >> aura to propagate transform changes to descendants. If you could do > >> > > >> > that in a > >> >> > >> >> separate patch with test coverage that would be great. > >> > > >> > > >> > Isn't it going to be costly? E.g. if a transform is applied to the root > >> > window, lots of windows are going to be notified. > >> > > >> > https://codereview.chromium.org/551883002/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > https://codereview.chromium.org/551883002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
CL for propagating transformed notification to child hierarchy uploaded at http://crrev.com/558203002. Please take a look... On 2014/09/10 03:38:59, mohsen wrote: > On 2014/09/09 23:57:18, sky wrote: > > I'm still going to say this likely doesn't matter. How much children > > does the window have? > > In the Athena example, not much; probably two or three children. I was just > worried about the general case. > > > > > On Tue, Sep 9, 2014 at 4:41 PM, <mailto:mohsen@chromium.org> wrote: > > > On 2014/09/09 23:31:21, sky wrote: > > >> > > >> How often do transforms change? We already do this sort of > > >> notifications for some things, like modifying the hierarchy. > > > > > > > > > In Athena for example, when user drags title of a window, a transform is > > > applied > > > to the window on each scroll-update. > > > > > > > > >> -Scott > > > > > > > > >> On Tue, Sep 9, 2014 at 2:36 PM, <mailto:mohsen@chromium.org> wrote: > > >> > > > >> > > > > > > > > > > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > > >> > > >> > File content/browser/renderer_host/render_widget_host_view_aura.cc > > >> > (right): > > >> > > > >> > > > > > > > > > > > > > https://codereview.chromium.org/551883002/diff/1/content/browser/renderer_hos... > > >> > > >> > content/browser/renderer_host/render_widget_host_view_aura.cc:438: for > > >> > (; window; window = window->parent()) > > >> > On 2014/09/08 20:24:56, sky wrote: > > >> >> > > >> >> This is going to be fragile and easy to get wrong. I think it better > > >> > > > >> > to change > > >> >> > > >> >> aura to propagate transform changes to descendants. If you could do > > >> > > > >> > that in a > > >> >> > > >> >> separate patch with test coverage that would be great. > > >> > > > >> > > > >> > Isn't it going to be costly? E.g. if a transform is applied to the root > > >> > window, lots of windows are going to be notified. > > >> > > > >> > https://codereview.chromium.org/551883002/ > > > > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > > > email > > >> > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > > > > > > https://codereview.chromium.org/551883002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
Updated to use new propagated transform event. Please take a look...
LGTM
The CQ bit was checked by mohsen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/551883002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as f3677b7cd4e9ba9d8d9ee171250a8ab173627e7a
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/584566f50ab083374216a6125709efc72f245dfd Cr-Commit-Position: refs/heads/master@{#294429} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
