Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
416243004:
Enable touch text selection on Athena (Closed)
|
Created:
6 years, 5 months ago by mohsen Modified:
6 years, 4 months ago CC:
chromium-reviews, mfomitchev Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionEnable touch text selection on Athena
Touch text selection controllers are created using ViewsDelegate. So, a
ViewsDelegate implementation is added. Also, changed Athena overview
mode to only show windows of type WINDOW_TYPE_NORMAL.
BUG=386350
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287208
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added override comment + Rebased #
Total comments: 2
Patch Set 3 : Passed windows list to overview mode #
Total comments: 7
Patch Set 4 : Moved AthenaViewsDelegate to athena_launcher #Patch Set 5 : Get windows list from container in overview mode #
Total comments: 2
Patch Set 6 : Got rid of windows list in overview mode #
Total comments: 7
Patch Set 7 : nits #
Total comments: 2
Patch Set 8 : Used count_if #Patch Set 9 : Rebased #Patch Set 10 : Fixed HomeCard issue #
Messages
Total messages: 31 (0 generated)
sadrul@: can you take a look, before sending to an OWNER?
For the changes in window_overview_mode.cc, it may make sense to wait until Mikhail's CL lands. https://codereview.chromium.org/416243004/diff/1/athena/main/athena_main.cc File athena/main/athena_main.cc (right): https://codereview.chromium.org/416243004/diff/1/athena/main/athena_main.cc#n... athena/main/athena_main.cc:84: virtual void OnBeforeWidgetInit( // views::ViewsDelegate: https://codereview.chromium.org/416243004/diff/1/athena/wm/window_overview_mo... File athena/wm/window_overview_mode.cc (left): https://codereview.chromium.org/416243004/diff/1/athena/wm/window_overview_mo... athena/wm/window_overview_mode.cc:113: aura::Window::Windows windows = container_->children(); Mikhail has a CL up (https://codereview.chromium.org/420603011/) after which this change won't be necessary.
cc: mfomitchev@ https://codereview.chromium.org/416243004/diff/1/athena/main/athena_main.cc File athena/main/athena_main.cc (right): https://codereview.chromium.org/416243004/diff/1/athena/main/athena_main.cc#n... athena/main/athena_main.cc:84: virtual void OnBeforeWidgetInit( On 2014/07/25 20:43:14, sadrul wrote: > // views::ViewsDelegate: Done. https://codereview.chromium.org/416243004/diff/1/athena/wm/window_overview_mo... File athena/wm/window_overview_mode.cc (left): https://codereview.chromium.org/416243004/diff/1/athena/wm/window_overview_mo... athena/wm/window_overview_mode.cc:113: aura::Window::Windows windows = container_->children(); On 2014/07/25 20:43:14, sadrul wrote: > Mikhail has a CL up (https://codereview.chromium.org/420603011/) after which > this change won't be necessary. I talked to Mikhail and his patch might take a while to be ready to land. Maybe we can land this CL to unblock touch selection work and then Mikhail fixes this part along with his patch?
oshima@: sadrul@ is OOO and I was going to send this CL to you for final OWNER review, anyway. Can you take a look, please?
https://codereview.chromium.org/416243004/diff/60001/athena/wm/window_overvie... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/60001/athena/wm/window_overvie... athena/wm/window_overview_mode.cc:320: WindowOverviewModeDelegate* delegate) { My suggestion would be to have WindowOverviewMode::Create() take in an extra aura::Window::Windows parameter. The caller (WindowManagerImpl, in this case), would send the list of windows (container_->children(), for now) the overview-mode should operate on. Mikhail's patch will change the list sent, and it won't be necessary to make any changes for the overview mode again for that. WindowOverviewModeImpl will apply the necessary filters on this list upon construction, so that it can avoid having to do this in all places (DoScroll, SetInitialWindowStates etc.)
https://codereview.chromium.org/416243004/diff/60001/athena/wm/window_overvie... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/60001/athena/wm/window_overvie... athena/wm/window_overview_mode.cc:320: WindowOverviewModeDelegate* delegate) { On 2014/07/31 23:54:43, sadrul wrote: > My suggestion would be to have WindowOverviewMode::Create() take in an extra > aura::Window::Windows parameter. The caller (WindowManagerImpl, in this case), > would send the list of windows (container_->children(), for now) the > overview-mode should operate on. Mikhail's patch will change the list sent, and > it won't be necessary to make any changes for the overview mode again for that. > > WindowOverviewModeImpl will apply the necessary filters on this list upon > construction, so that it can avoid having to do this in all places (DoScroll, > SetInitialWindowStates etc.) Yep, that makes perfect sense. Done.
LGTM
https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... athena/wm/window_overview_mode.cc:106: } you shouldn't need window_. what you're passing is the child of the container, and you can use kWindowOverviewState to tell if the window needs to be restored.
https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... athena/wm/window_overview_mode.cc:106: } On 2014/08/01 16:09:38, oshima wrote: > you shouldn't need window_. what you're passing is the child of the container, > and you can use kWindowOverviewState to tell if the window needs to be restored. > The filtering here is done to avoid showing popup windows (e.g. touch-selection handle widgets) in overview.
one more. https://codereview.chromium.org/416243004/diff/80001/athena/main/athena_main.cc File athena/main/athena_main.cc (right): https://codereview.chromium.org/416243004/diff/80001/athena/main/athena_main.... athena/main/athena_main.cc:119: views::ViewsDelegate::views_delegate = &views_delegate_; can you move this to athena_launcher ?
https://codereview.chromium.org/416243004/diff/80001/athena/main/athena_main.cc File athena/main/athena_main.cc (right): https://codereview.chromium.org/416243004/diff/80001/athena/main/athena_main.... athena/main/athena_main.cc:119: views::ViewsDelegate::views_delegate = &views_delegate_; On 2014/08/01 17:22:11, oshima wrote: > can you move this to athena_launcher ? Done. Please take a look...
https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... athena/wm/window_overview_mode.cc:106: } On 2014/08/01 16:15:46, sadrul wrote: > On 2014/08/01 16:09:38, oshima wrote: > > you shouldn't need window_. what you're passing is the child of the > container, > > and you can use kWindowOverviewState to tell if the window needs to be > restored. > > > > The filtering here is done to avoid showing popup windows (e.g. touch-selection > handle widgets) in overview. you can filter the child of container. No need to pass wnidows.
https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... athena/wm/window_overview_mode.cc:106: } On 2014/08/01 20:04:10, oshima wrote: > On 2014/08/01 16:15:46, sadrul wrote: > > On 2014/08/01 16:09:38, oshima wrote: > > > you shouldn't need window_. what you're passing is the child of the > > container, > > > and you can use kWindowOverviewState to tell if the window needs to be > > restored. > > > > > > > The filtering here is done to avoid showing popup windows (e.g. > touch-selection > > handle widgets) in overview. > > you can filter the child of container. No need to pass wnidows. This is in preparation for Mikhail's patch, where the order of the Windows in overview mode may not necessarily be the stacking order of container_->children()
On 2014/08/01 20:05:41, sadrul wrote: > https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... > File athena/wm/window_overview_mode.cc (right): > > https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... > athena/wm/window_overview_mode.cc:106: } > On 2014/08/01 20:04:10, oshima wrote: > > On 2014/08/01 16:15:46, sadrul wrote: > > > On 2014/08/01 16:09:38, oshima wrote: > > > > you shouldn't need window_. what you're passing is the child of the > > > container, > > > > and you can use kWindowOverviewState to tell if the window needs to be > > > restored. > > > > > > > > > > The filtering here is done to avoid showing popup windows (e.g. > > touch-selection > > > handle widgets) in overview. > > > > you can filter the child of container. No need to pass wnidows. > > This is in preparation for Mikhail's patch, where the order of the Windows in > overview mode may not necessarily be the stacking order of > container_->children() That change should be in his CL. And I do think we should use something else to maintain and controls the activity orders not the vector of the aura window.
https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/80001/athena/wm/window_overvie... athena/wm/window_overview_mode.cc:106: } On 2014/08/01 20:05:41, sadrul wrote: > On 2014/08/01 20:04:10, oshima wrote: > > On 2014/08/01 16:15:46, sadrul wrote: > > > On 2014/08/01 16:09:38, oshima wrote: > > > > you shouldn't need window_. what you're passing is the child of the > > > container, > > > > and you can use kWindowOverviewState to tell if the window needs to be > > > restored. > > > > > > > > > > The filtering here is done to avoid showing popup windows (e.g. > > touch-selection > > > handle widgets) in overview. > > > > you can filter the child of container. No need to pass wnidows. > > This is in preparation for Mikhail's patch, where the order of the Windows in > overview mode may not necessarily be the stacking order of > container_->children() I changed it such that the window list is not passed, but the filtered list is still created and populated in the constructor using children of the container. Later, if need be, Mikhail can easily pass his list and populate the list here easily. oshima@: Is it looking better now?
https://codereview.chromium.org/416243004/diff/140001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/140001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:300: aura::Window::Windows windows_; you can just use container's children and property for now. later, we should have a class (not a just a list of windows) to maintain/controls the order.
https://codereview.chromium.org/416243004/diff/140001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/140001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:300: aura::Window::Windows windows_; On 2014/08/01 20:27:14, oshima wrote: > you can just use container's children and property for now. > > later, we should have a class (not a just a list of windows) to > maintain/controls the order. OK. I think I see what you say. My only concern would be in that case we need to check the property on all windows on every scroll and it might degrade the performance. Is that fine?
On 2014/08/01 20:44:35, mohsen wrote: > https://codereview.chromium.org/416243004/diff/140001/athena/wm/window_overvi... > File athena/wm/window_overview_mode.cc (right): > > https://codereview.chromium.org/416243004/diff/140001/athena/wm/window_overvi... > athena/wm/window_overview_mode.cc:300: aura::Window::Windows windows_; > On 2014/08/01 20:27:14, oshima wrote: > > you can just use container's children and property for now. > > > > later, we should have a class (not a just a list of windows) to > > maintain/controls the order. > > OK. I think I see what you say. My only concern would be in that case we need to > check the property on all windows on every scroll and it might degrade the > performance. Is that fine? I doubt that it'll be an issue, and this is just short term solution. We should soon have a separate object that maintains the list of activities.
Updated. Please check if that's what you have in mind...
almost https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (left): https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:225: aura::Window::Windows windows = container_->children(); const aura::Window::Windows& and you may use windows below. https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:153: continue; Check the type here and skip or create a property. https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:185: if (!window->GetProperty(kWindowOverviewState)) HasProperty
https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (left): https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:225: aura::Window::Windows windows = container_->children(); On 2014/08/01 22:27:46, oshima wrote: > const aura::Window::Windows& and you may use windows below. Done. Here and other places like this. https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:153: continue; On 2014/08/01 22:27:46, oshima wrote: > Check the type here and skip or create a property. I just wanted to check type only in one place and check property in the rest (otherwise, I could just check the type everywhere). Since we need to loop once at the top to count normal windows, I created the property in the same loop. Anyways, Done. https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:185: if (!window->GetProperty(kWindowOverviewState)) On 2014/08/01 22:27:46, oshima wrote: > HasProperty Is there a HasProperty function? Can't find it!
lgtm https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:185: if (!window->GetProperty(kWindowOverviewState)) On 2014/08/01 22:27:46, oshima wrote: > HasProperty Oops sorry. it's my mistkae. https://codereview.chromium.org/416243004/diff/180001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/180001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:134: } I see. you could use count_if, but I'll leave it to you.
https://codereview.chromium.org/416243004/diff/180001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/416243004/diff/180001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:134: } On 2014/08/01 23:10:09, oshima wrote: > I see. you could use count_if, but I'll leave it to you. Yep, Done.
slgtm nice!
Because of a new change, whenever a window is added to the default container, overview mode is closed. It causes a problem for this patch: when you tap on search box in HomeCard, touch selection is activated which adds touch selection widgets to the default container which causes overview mode to close. I have changed that logic to only close overview mode when the new window is of type WINDOW_TYPE_NORMAL. Can you take another look, please...
lgtm
slgtm Forgot to ask. You may land this first, but could you please add tests in a separate CL? thanks
On 2014/08/02 05:02:16, oshima wrote: > slgtm > > Forgot to ask. You may land this first, but > could you please add tests in a separate CL? > > thanks Sure.
The CQ bit was checked by mohsen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/416243004/180002
Message was sent while issue was closed.
Change committed as 287208 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
