Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9808026:
Layout panels on top of their launcher icons (Closed)
|
Created:
8 years, 9 months ago by Dmitry Lomov (no reviews) Modified:
8 years, 7 months ago CC:
chromium-reviews, sadrul, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPanels at launcher
Patch Set 1 #Patch Set 2 : Patch #Patch Set 3 : Initial patch #Patch Set 4 : Added a file #Patch Set 5 : Ununsed constanrs removed #
Total comments: 16
Patch Set 6 : New version #Patch Set 7 : Patch for review #
Total comments: 22
Patch Set 8 : CR feedback #
Total comments: 6
Patch Set 9 : CR feedback #Patch Set 10 : Pre-patch with tests #
Total comments: 1
Patch Set 11 : Patch with tests #
Total comments: 12
Patch Set 12 : Attempt at LauncherViewTest #
Total comments: 12
Patch Set 13 : CR feedback #
Messages
Total messages: 57 (0 generated)
Hi folks, could you take a look? This is not final, requires at least hooking up to chrome proper and tests.
Some unused constants remoced
How come chromium-reviews isn't cc'd? https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... File ash/launcher/launcher_view.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... ash/launcher/launcher_view.cc:349: OnLauncherIconPositionsChanged()); What is it you're trying to track?
https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... File ash/launcher/launcher_view.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... ash/launcher/launcher_view.cc:349: OnLauncherIconPositionsChanged()); On 2012/03/23 20:33:12, sky wrote: > What is it you're trying to track? The fact that launcher icons changed their positions. I need that so that I can make panels follow along.
On 2012/03/23 20:35:59, Dmitry Lomov (chromium) wrote: > https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... > File ash/launcher/launcher_view.cc (right): > > https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... > ash/launcher/launcher_view.cc:349: OnLauncherIconPositionsChanged()); > On 2012/03/23 20:33:12, sky wrote: > > What is it you're trying to track? > The fact that launcher icons changed their positions. I need that so that I can > make panels follow along. Is there a reason you're not observing the model? It knows when items move.
On 2012/03/23 21:19:07, sky wrote: > On 2012/03/23 20:35:59, Dmitry Lomov (chromium) wrote: > > > https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... > > File ash/launcher/launcher_view.cc (right): > > > > > https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... > > ash/launcher/launcher_view.cc:349: OnLauncherIconPositionsChanged()); > > On 2012/03/23 20:33:12, sky wrote: > > > What is it you're trying to track? > > The fact that launcher icons changed their positions. I need that so that I > can > > make panels follow along. > > Is there a reason you're not observing the model? It knows when items move. LauncherModel moves the items, and LauncherView listens to it and updates the coordinates. If I listen to model, there is no guarantee that I get to listen to it *after* LauncherView listens, so I am ending up with obsolete coordinates (very typical issue in all observer-based designs). Note that LauncherView un-listens and re-listens to the model many times in course of its life (e.g. for dragging)
https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... File ash/launcher/launcher.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... ash/launcher/launcher.cc:192: void Launcher::AddIconsObserver(LauncherIconsObserver* observer) { Fix the order to match the header. https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launcher.h File ash/launcher/launcher.h (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... ash/launcher/launcher.h:58: void AddIconsObserver(LauncherIconsObserver* observer); Add newline between 57/58. https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... File ash/launcher/launcher_icons_observer.h (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... ash/launcher/launcher_icons_observer.h:12: class ASH_EXPORT LauncherIconsObserver { Name this LauncherViewObserver. https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... ash/launcher/launcher_icons_observer.h:15: }; Add protected virtual destructor. https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/shell/shell_main.cc File ash/shell/shell_main.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/shell/shell_main... ash/shell/shell_main.cc:93: //if ((new_window->type() != aura::client::WINDOW_TYPE_NORMAL) Keep the old code. https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/shell/shell_main... ash/shell/shell_main.cc:112: new_window->SetProperty(aura::client::kLauncherIDKey, id); What is this for? https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... File ash/wm/panel_layout_manager.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... ash/wm/panel_layout_manager.cc:46: ash::Launcher* launcher = ash::Shell::GetInstance()->launcher(); Did you make sure this is deleted before the launcher? https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... ash/wm/panel_layout_manager.cc:172: shell->launcher()->GetScreenBoundsOfItemIconForWindow(panel_win); This may be empty. https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... File ash/wm/panel_layout_manager.h (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... ash/wm/panel_layout_manager.h:51: void SetLauncher(ash::Launcher* launcher); If this positions things above the launcher, doesn't it need to know when the launcher positions changes? Such as happens when auto hidden? https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... ash/wm/panel_layout_manager.h:63: class LauncherIconsObserver : public ash::LauncherIconsObserver { Don't inline all this. https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... ash/wm/panel_layout_manager.h:66: : layout_manager_(layout_manager) {} indent 4.
Thanks for taking a look. I'll address the the stuff I didn't reply to later since it is a work-in-progress https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... File ash/wm/panel_layout_manager.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... ash/wm/panel_layout_manager.cc:46: ash::Launcher* launcher = ash::Shell::GetInstance()->launcher(); On 2012/03/23 21:41:14, sky wrote: > Did you make sure this is deleted before the launcher? Hopefully the getter will return null? https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/wm/panel_layout_... ash/wm/panel_layout_manager.cc:172: shell->launcher()->GetScreenBoundsOfItemIconForWindow(panel_win); On 2012/03/23 21:41:14, sky wrote: > This may be empty. True. The layout manager has no idea what to do in this case :)
(a bit more on Launcher(Icon|View)Observer https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... File ash/launcher/launcher_icons_observer.h (right): https://chromiumcodereview.appspot.com/9808026/diff/1011/ash/launcher/launche... ash/launcher/launcher_icons_observer.h:12: class ASH_EXPORT LauncherIconsObserver { On 2012/03/23 21:41:14, sky wrote: > Name this LauncherViewObserver. Here is a bit bigger issue. I'd rather expose LauncherView getter on Launcher, rename this class into LauncherViewObserver and move Add/RemoveObserver for it onto LauncherView. Besides being cleaner, this will help to handle auto-hide stuff as well.
Pre-code-review comments adderessed. Please take a look. I decided to make hook up to chrome proper a separate patch
How about some tests? https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launcher.h File ash/launcher/launcher.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... ash/launcher/launcher.h:33: class LauncherIconsObserver; Name this class LauncherIconObserver. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... File ash/launcher/launcher_icons_observer.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... ash/launcher/launcher_icons_observer.h:14: virtual void OnLauncherIconPositionsChanged() = 0; Document the conditions under which this is invoked. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... File ash/launcher/launcher_view.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... ash/launcher/launcher_view.cc:295: } I think you want to notify here too. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/shell.cc File ash/shell.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/shell.cc#newcode757 ash/shell.cc:757: Remove this line. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/shell/window_wat... File ash/shell/window_watcher.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/shell/window_wat... ash/shell/window_watcher.cc:45: && new_window->type() != aura::client::WINDOW_TYPE_PANEL) && on the previous line. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/wm/panel_layout_... File ash/wm/panel_layout_manager.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/wm/panel_layout_... ash/wm/panel_layout_manager.h:61: class LauncherIconsObserver : public ash::LauncherIconsObserver { Any reason you don't make PanelLayoutManager directly implement this as we do everywhere else?
Some comments. I'll see what I can do w.r.t tests. One problem is that we do not have tests that are systematically run under --aura-panels. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launcher.h File ash/launcher/launcher.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... ash/launcher/launcher.h:33: class LauncherIconsObserver; On 2012/04/03 20:39:37, sky wrote: > Name this class LauncherIconObserver. Why? It observes all icons, you cannot observe a particular icon. [I am not a native English speaker; let me know if it is imposssible gramar in English] https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... File ash/launcher/launcher_icons_observer.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... ash/launcher/launcher_icons_observer.h:14: virtual void OnLauncherIconPositionsChanged() = 0; On 2012/04/03 20:39:37, sky wrote: > Document the conditions under which this is invoked. Will do https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... File ash/launcher/launcher_view.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... ash/launcher/launcher_view.cc:295: } On 2012/04/03 20:39:37, sky wrote: > I think you want to notify here too. Will do. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/wm/panel_layout_... File ash/wm/panel_layout_manager.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/wm/panel_layout_... ash/wm/panel_layout_manager.h:61: class LauncherIconsObserver : public ash::LauncherIconsObserver { On 2012/04/03 20:39:37, sky wrote: > Any reason you don't make PanelLayoutManager directly implement this as we do > everywhere else? The fact that PanelLayoutManager observes launcher icons is its implementation detail, it is not a fact of its public interface.
Also, this won't pick up changes when auto-hide kicks in. I mentioned this to Steven and he said to punt on it for now. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launcher.h File ash/launcher/launcher.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/launcher/launche... ash/launcher/launcher.h:33: class LauncherIconsObserver; On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > On 2012/04/03 20:39:37, sky wrote: > > Name this class LauncherIconObserver. > Why? It observes all icons, you cannot observe a particular icon. > [I am not a native English speaker; let me know if it is imposssible gramar in > English] For a similar reason that we have TabModel and not TabsModel. https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/wm/panel_layout_... File ash/wm/panel_layout_manager.h (right): https://chromiumcodereview.appspot.com/9808026/diff/9014/ash/wm/panel_layout_... ash/wm/panel_layout_manager.h:61: class LauncherIconsObserver : public ash::LauncherIconsObserver { On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > On 2012/04/03 20:39:37, sky wrote: > > Any reason you don't make PanelLayoutManager directly implement this as we do > > everywhere else? > > The fact that PanelLayoutManager observes launcher icons is its implementation > detail, it is not a fact of its public interface. > There are a plethora of classes in Chrome that fit this bill. As I mentioned, we generally don't add another layer just to hide the interface.
http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.cc File ash/wm/panel_layout_manager.cc (right): http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.cc:183: if (icon_rect.width() == 0 && icon_rect.height() == 0) I believe this happens when the icon is not visible in the Launcher, e.g. it is in overflow. For now, I think we should hide the panel if that happens. At minimum we should comment here that we are explicitly doing nothing in that case. http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.cc:188: panel_container_, &icon_origin); icon_origin only appears to be needed in the if block below? We should move this to inside the if(). http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.cc:190: if (!panel_win->IsVisible()) We should probably move this early exit up to immediately after line 179? This doesn't appear to be related to the code immediately above it.
http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h File ash/launcher/launcher.h (right): http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h#newc... ash/launcher/launcher.h:33: class LauncherIconsObserver; On 2012/04/03 20:57:18, sky wrote: > On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > > On 2012/04/03 20:39:37, sky wrote: > > > Name this class LauncherIconObserver. > > Why? It observes all icons, you cannot observe a particular icon. > > [I am not a native English speaker; let me know if it is imposssible gramar in > > English] > > For a similar reason that we have TabModel and not TabsModel. And that reason is...? http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.cc File ash/wm/panel_layout_manager.cc (right): http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.cc:183: if (icon_rect.width() == 0 && icon_rect.height() == 0) On 2012/04/03 20:59:28, stevenjb (chromium) wrote: > I believe this happens when the icon is not visible in the Launcher, e.g. it is > in overflow. For now, I think we should hide the panel if that happens. Right, and used does not get any notification on incoming chat. I do not think this is a good UX. > At minimum we should comment here that we are explicitly doing nothing in that > case. I'll add comment. http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.cc:188: panel_container_, &icon_origin); On 2012/04/03 20:59:28, stevenjb (chromium) wrote: > icon_origin only appears to be needed in the if block below? We should move this > to inside the if(). You are right. http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.cc:190: if (!panel_win->IsVisible()) On 2012/04/03 20:59:28, stevenjb (chromium) wrote: > We should probably move this early exit up to immediately after line 179? This > doesn't appear to be related to the code immediately above it. Right again. http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.h File ash/wm/panel_layout_manager.h (right): http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.h:61: class LauncherIconsObserver : public ash::LauncherIconsObserver { On 2012/04/03 20:57:18, sky wrote: > On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > > On 2012/04/03 20:39:37, sky wrote: > > > Any reason you don't make PanelLayoutManager directly implement this as we > do > > > everywhere else? > > > > The fact that PanelLayoutManager observes launcher icons is its implementation > > detail, it is not a fact of its public interface. > > > > There are a plethora of classes in Chrome that fit this bill. As I mentioned, we > generally don't add another layer just to hide the interface. Widespread use of errant practice does not make it commendable.
On Tue, Apr 3, 2012 at 2:18 PM, <dslomov@chromium.org> wrote: > > http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h > File ash/launcher/launcher.h (right): > > http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h#newc... > ash/launcher/launcher.h:33: class LauncherIconsObserver; > > On 2012/04/03 20:57:18, sky wrote: >> >> On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: >> > On 2012/04/03 20:39:37, sky wrote: >> > > Name this class LauncherIconObserver. >> > Why? It observes all icons, you cannot observe a particular icon. >> > [I am not a native English speaker; let me know if it is imposssible > > gramar in >> >> > English] > > >> For a similar reason that we have TabModel and not TabsModel. > > > And that reason is...? I couldn't tell you the exact grammatical rule. It may be more of a design pattern. > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.cc > File ash/wm/panel_layout_manager.cc (right): > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > ash/wm/panel_layout_manager.cc:183: if (icon_rect.width() == 0 && > icon_rect.height() == 0) > On 2012/04/03 20:59:28, stevenjb (chromium) wrote: >> >> I believe this happens when the icon is not visible in the Launcher, > > e.g. it is >> >> in overflow. For now, I think we should hide the panel if that > > happens. > Right, and used does not get any notification on incoming chat. I do not > think this is a good UX. > > >> At minimum we should comment here that we are explicitly doing nothing > > in that >> >> case. > > I'll add comment. > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > ash/wm/panel_layout_manager.cc:188: panel_container_, &icon_origin); > On 2012/04/03 20:59:28, stevenjb (chromium) wrote: >> >> icon_origin only appears to be needed in the if block below? We should > > move this >> >> to inside the if(). > > > You are right. > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > ash/wm/panel_layout_manager.cc:190: if (!panel_win->IsVisible()) > On 2012/04/03 20:59:28, stevenjb (chromium) wrote: >> >> We should probably move this early exit up to immediately after line > > 179? This >> >> doesn't appear to be related to the code immediately above it. > > > Right again. > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.h > File ash/wm/panel_layout_manager.h (right): > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > > ash/wm/panel_layout_manager.h:61: class LauncherIconsObserver : public > ash::LauncherIconsObserver { > On 2012/04/03 20:57:18, sky wrote: >> >> On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: >> > On 2012/04/03 20:39:37, sky wrote: >> > > Any reason you don't make PanelLayoutManager directly implement > > this as we >> >> do >> > > everywhere else? >> > >> > The fact that PanelLayoutManager observes launcher icons is its > > implementation >> >> > detail, it is not a fact of its public interface. >> > > > >> There are a plethora of classes in Chrome that fit this bill. As I > > mentioned, we >> >> generally don't add another layer just to hide the interface. > > Widespread use of errant practice does not make it commendable. Agree, but in the case you're introducing another layer and more complexity for little value. -Scott
On Tue, Apr 3, 2012 at 2:27 PM, Scott Violet <sky@chromium.org> wrote: > On Tue, Apr 3, 2012 at 2:18 PM, <dslomov@chromium.org> wrote: > > > > http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h > > File ash/launcher/launcher.h (right): > > > > > http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h#newc... > > ash/launcher/launcher.h:33: class LauncherIconsObserver; > > > > On 2012/04/03 20:57:18, sky wrote: > >> > >> On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > >> > On 2012/04/03 20:39:37, sky wrote: > >> > > Name this class LauncherIconObserver. > >> > Why? It observes all icons, you cannot observe a particular icon. > >> > [I am not a native English speaker; let me know if it is imposssible > > > > gramar in > >> > >> > English] > > > > > >> For a similar reason that we have TabModel and not TabsModel. > > > > > > And that reason is...? > > I couldn't tell you the exact grammatical rule. It may be more of a > design pattern. > > > > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.cc > > File ash/wm/panel_layout_manager.cc (right): > > > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > > ash/wm/panel_layout_manager.cc:183: if (icon_rect.width() == 0 && > > icon_rect.height() == 0) > > On 2012/04/03 20:59:28, stevenjb (chromium) wrote: > >> > >> I believe this happens when the icon is not visible in the Launcher, > > > > e.g. it is > >> > >> in overflow. For now, I think we should hide the panel if that > > > > happens. > > Right, and used does not get any notification on incoming chat. I do not > > think this is a good UX. > > > > > >> At minimum we should comment here that we are explicitly doing nothing > > > > in that > >> > >> case. > > > > I'll add comment. > > > > > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > > ash/wm/panel_layout_manager.cc:188: panel_container_, &icon_origin); > > On 2012/04/03 20:59:28, stevenjb (chromium) wrote: > >> > >> icon_origin only appears to be needed in the if block below? We should > > > > move this > >> > >> to inside the if(). > > > > > > You are right. > > > > > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > > ash/wm/panel_layout_manager.cc:190: if (!panel_win->IsVisible()) > > On 2012/04/03 20:59:28, stevenjb (chromium) wrote: > >> > >> We should probably move this early exit up to immediately after line > > > > 179? This > >> > >> doesn't appear to be related to the code immediately above it. > > > > > > Right again. > > > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.h > > File ash/wm/panel_layout_manager.h (right): > > > > > http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... > > > > ash/wm/panel_layout_manager.h:61: class LauncherIconsObserver : public > > ash::LauncherIconsObserver { > > On 2012/04/03 20:57:18, sky wrote: > >> > >> On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > >> > On 2012/04/03 20:39:37, sky wrote: > >> > > Any reason you don't make PanelLayoutManager directly implement > > > > this as we > >> > >> do > >> > > everywhere else? > >> > > >> > The fact that PanelLayoutManager observes launcher icons is its > > > > implementation > >> > >> > detail, it is not a fact of its public interface. > >> > > > > > > >> There are a plethora of classes in Chrome that fit this bill. As I > > > > mentioned, we > >> > >> generally don't add another layer just to hide the interface. > > > > Widespread use of errant practice does not make it commendable. > > Agree, but in the case you're introducing another layer and more > complexity for little value. > Having a clean interface without exposing private details is a great value. I just cannot make myself do it, sorry. It is as if I needed to push private method to public for no reason. > > -Scott >
http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h File ash/launcher/launcher.h (right): http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h#newc... ash/launcher/launcher.h:33: class LauncherIconsObserver; On 2012/04/03 21:18:23, Dmitry Lomov (chromium) wrote: > On 2012/04/03 20:57:18, sky wrote: > > On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > > > On 2012/04/03 20:39:37, sky wrote: > > > > Name this class LauncherIconObserver. > > > Why? It observes all icons, you cannot observe a particular icon. > > > [I am not a native English speaker; let me know if it is imposssible gramar > in > > > English] > > > > For a similar reason that we have TabModel and not TabsModel. > > And that reason is...? > Typically observer and delegate classes are named with the type of thing they are watching. The details of which ones or how many are not part of the class name. Also, in english one always says "Bird Watcher", not "Birds Watcher", even though more than one bird is typically watched. http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.h File ash/wm/panel_layout_manager.h (right): http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.... ash/wm/panel_layout_manager.h:61: class LauncherIconsObserver : public ash::LauncherIconsObserver { I have to agree with sky here - we make "correctness" vs. "simplicity" tradeoffs all the time. In Chrome, Observer and Delegate interfaces are generally inherited because it makes the code easier to follow. On 2012/04/03 21:18:23, Dmitry Lomov (chromium) wrote: > On 2012/04/03 20:57:18, sky wrote: > > On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: > > > On 2012/04/03 20:39:37, sky wrote: > > > > Any reason you don't make PanelLayoutManager directly implement this as we > > do > > > > everywhere else? > > > > > > The fact that PanelLayoutManager observes launcher icons is its > implementation > > > detail, it is not a fact of its public interface. > > > > > > > There are a plethora of classes in Chrome that fit this bill. As I mentioned, > we > > generally don't add another layer just to hide the interface. > Widespread use of errant practice does not make it commendable. >
On Tue, Apr 3, 2012 at 3:09 PM, <stevenjb@chromium.org> wrote: > > http://codereview.chromium.**org/9808026/diff/9014/ash/** > launcher/launcher.h<http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h> > File ash/launcher/launcher.h (right): > > http://codereview.chromium.**org/9808026/diff/9014/ash/** > launcher/launcher.h#newcode33<http://codereview.chromium.org/9808026/diff/9014/ash/launcher/launcher.h#newcode33> > ash/launcher/launcher.h:33: class LauncherIconsObserver; > On 2012/04/03 21:18:23, Dmitry Lomov (chromium) wrote: > >> On 2012/04/03 20:57:18, sky wrote: >> > On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: >> > > On 2012/04/03 20:39:37, sky wrote: >> > > > Name this class LauncherIconObserver. >> > > Why? It observes all icons, you cannot observe a particular icon. >> > > [I am not a native English speaker; let me know if it is >> > imposssible gramar > >> in >> > > English] >> > >> > For a similar reason that we have TabModel and not TabsModel. >> > > And that reason is...? >> > > Typically observer and delegate classes are named with the type of thing > they are watching. The details of which ones or how many are not part of > the class name. Also, in english one always says "Bird Watcher", not > "Birds Watcher", even though more than one bird is typically watched. Ok, as I said I am not the native English speaker so it is fine. > > > http://codereview.chromium.**org/9808026/diff/9014/ash/wm/** > panel_layout_manager.h<http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.h> > File ash/wm/panel_layout_manager.h (right): > > http://codereview.chromium.**org/9808026/diff/9014/ash/wm/** > panel_layout_manager.h#**newcode61<http://codereview.chromium.org/9808026/diff/9014/ash/wm/panel_layout_manager.h#newcode61> > ash/wm/panel_layout_manager.h:**61: class LauncherIconsObserver : public > ash::LauncherIconsObserver { > I have to agree with sky here - we make "correctness" vs. "simplicity" > tradeoffs all the time. In Chrome, Observer and Delegate interfaces are > generally inherited because it makes the code easier to follow. I am sorry, as I said I cannot agree to do that. > > > On 2012/04/03 21:18:23, Dmitry Lomov (chromium) wrote: > >> On 2012/04/03 20:57:18, sky wrote: >> > On 2012/04/03 20:50:07, Dmitry Lomov (chromium) wrote: >> > > On 2012/04/03 20:39:37, sky wrote: >> > > > Any reason you don't make PanelLayoutManager directly implement >> > this as we > >> > do >> > > > everywhere else? >> > > >> > > The fact that PanelLayoutManager observes launcher icons is its >> implementation >> > > detail, it is not a fact of its public interface. >> > > >> > >> > There are a plethora of classes in Chrome that fit this bill. As I >> > mentioned, > >> we >> > generally don't add another layer just to hide the interface. >> Widespread use of errant practice does not make it commendable. >> > > > http://codereview.chromium.**org/9808026/<http://codereview.chromium.org/9808... >
On Tue, Apr 3, 2012 at 4:12 PM, Dmitry Lomov <dslomov@google.com> wrote: > I have to agree with sky here - we make "correctness" vs. "simplicity" >> tradeoffs all the time. In Chrome, Observer and Delegate interfaces are >> generally inherited because it makes the code easier to follow. > > > I am sorry, as I said I cannot agree to do that. > While I agree this approach may be desirable when a class gets big and unwieldy (Browser is an example), I think it is premature to consider it here. Please make the changes requested by the reviewers. -Ben
On Wed, Apr 4, 2012 at 9:18 AM, Ben Goodger <beng@google.com> wrote: > On Tue, Apr 3, 2012 at 4:12 PM, Dmitry Lomov <dslomov@google.com> wrote: > >> I have to agree with sky here - we make "correctness" vs. "simplicity" >>> tradeoffs all the time. In Chrome, Observer and Delegate interfaces are >>> generally inherited because it makes the code easier to follow. >> >> >> I am sorry, as I said I cannot agree to do that. >> > > While I agree this approach may be desirable when a class gets big and > unwieldy (Browser is an example), I think it is premature to consider it > here. Please make the changes requested by the reviewers. > When it gets big and unwieldy, it will be too late. Only consenting to this under explicit pressure, please note my votum separatum. > > -Ben >
PTAL
https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/launcher/launch... File ash/launcher/launcher_view.h (right): https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/launcher/launch... ash/launcher/launcher_view.h:69: nit: single blank line here https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell.h File ash/shell.h (right): https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell.h#newcode327 ash/shell.h:327: // Manages layout of panels. Owned by container of panels. nit: s/container of panels/PanelContainer/ (co it can be searched for). https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell/window_wa... File ash/shell/window_watcher.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell/window_wa... ash/shell/window_watcher.cc:19: ash::internal::kShellWindowId_PanelContainer)) { nit: ash:: should be intended 4 spaces past 'panel_' https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/wm/panel_layout... File ash/wm/panel_layout_manager.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/wm/panel_layout... ash/wm/panel_layout_manager.cc:180: if (icon_rect.width() == 0 && icon_rect.height() == 0) This should be icon_rect.IsEmpty() to be consistent with GetScreenBoundsOfItemIconForWindow(). Also, please add a comment describing when this can happen, and what we are doing for that case, e.g. "Empty rect indicates the icon is not visible in the launcher, so do not change the panel's position."
https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell.h File ash/shell.h (right): https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell.h#newcode327 ash/shell.h:327: // Manages layout of panels. Owned by container of panels. On 2012/04/05 20:48:47, stevenjb (chromium) wrote: > nit: s/container of panels/PanelContainer/ (co it can be searched for). Changed, but there is no such entity as PanelContainer.
lgtm https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell.h File ash/shell.h (right): https://chromiumcodereview.appspot.com/9808026/diff/18001/ash/shell.h#newcode327 ash/shell.h:327: // Manages layout of panels. Owned by container of panels. On 2012/04/05 21:17:34, Dmitry Lomov (chromium) wrote: > On 2012/04/05 20:48:47, stevenjb (chromium) wrote: > > nit: s/container of panels/PanelContainer/ (co it can be searched for). > > Changed, but there is no such entity as PanelContainer. > PanelContainer is the name of the container (also the ID, minus the common internal::kShellWindowId_ prefix).
I see no issues. Where you planning on doing tests now or later?
On 2012/04/05 22:30:05, sky wrote: > I see no issues. Where you planning on doing tests now or later? I'd prefer doing tests in separate CL, coming shortly (mainly because this has been sitting here for too long; also, the value of tests is not big as we do not automatically run tests with '--aura-panels' yet). Let me know if you feel its ok or not?
chrome culture 101: tests now, not later.
Fair enough On Apr 5, 2012 6:17 PM, <ben@chromium.org> wrote: > chrome culture 101: tests now, not later. > > https://chromiumcodereview.**appspot.com/9808026/<https://chromiumcodereview.... >
For browser_tests, we sometimes have code like this to test with a specific
command line argument:
class PanelBrowserNavigatorTest : public BrowserNavigatorTest {
protected:
virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
command_line->AppendSwitch(switches::kEnablePanels);
}
};
I don't see any similar examples yet in ash/ but I suspect you can do something
like:
virtual void SetUp() OVERRIDE {
CommandLine::ForCurrentProcess()->
AppendSwitch(switches::kAuraPanelManager);
AshTestBase::SetUp();
}
On Fri, Apr 6, 2012 at 1:52 PM, <stevenjb@chromium.org> wrote: > For browser_tests, we sometimes have code like this to test with a specific > command line argument: > > class PanelBrowserNavigatorTest : public BrowserNavigatorTest { > protected: > virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE { > command_line->AppendSwitch(**switches::kEnablePanels); > } > }; > > I don't see any similar examples yet in ash/ but I suspect you can do > something > like: > > > virtual void SetUp() OVERRIDE { > CommandLine::**ForCurrentProcess()-> > AppendSwitch(switches::**kAuraPanelManager); > AshTestBase::SetUp(); > } > Right that is what I was thinking too. Unfortunately there is no corresponding CommandLine::RemoveSwitch, for TearDown. I am adding tests "under --aura-panels" switch right now, but I'll probably do what you suggest in a separate patch. > > > http://codereview.chromium.**org/9808026/<http://codereview.chromium.org/9808... >
On Fri, Apr 6, 2012 at 2:44 PM, Dmitry Lomov <dslomov@google.com> wrote: > > > On Fri, Apr 6, 2012 at 1:52 PM, <stevenjb@chromium.org> wrote: > >> For browser_tests, we sometimes have code like this to test with a >> specific >> command line argument: >> >> class PanelBrowserNavigatorTest : public BrowserNavigatorTest { >> protected: >> virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE { >> command_line->AppendSwitch(**switches::kEnablePanels); >> } >> }; >> >> I don't see any similar examples yet in ash/ but I suspect you can do >> something >> like: >> >> >> virtual void SetUp() OVERRIDE { >> CommandLine::**ForCurrentProcess()-> >> AppendSwitch(switches::**kAuraPanelManager); >> AshTestBase::SetUp(); >> } >> > > Right that is what I was thinking too. > Unfortunately there is no corresponding CommandLine::RemoveSwitch, for > TearDown. > (In case it is not obvious, this is not a problem for InProcessBrowserTests (such as PanelBrowserNavigatorTest), since they run in one-process-per-test mode. > I am adding tests "under --aura-panels" switch right now, but I'll > probably do what you suggest in a separate patch. > > >> >> >> http://codereview.chromium.**org/9808026/<http://codereview.chromium.org/9808... >> > >
Here is an attempt at tests (unfinished). I run into the issue below - suggestions are most welcome. https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... File ash/wm/panel_layout_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... ash/wm/panel_layout_manager_unittest.cc:54: EXPECT_FALSE(icon_rect.IsEmpty()); This test fails. The reason for the failure is that, immediately after the item is added to the launcher, the icon for the item is not initially visible. It only becomes visible after the animation is performed (see LauncherView::LauncherItemAdded). What would be the good way to either wait for animation to end in the test or disable animations for test?
Will panels trigger code that needs to know the bounds of the launcher item on creation? -Scott On Tue, Apr 10, 2012 at 12:13 PM, <dslomov@chromium.org> wrote: > Here is an attempt at tests (unfinished). I run into the issue below - > suggestions are most welcome. > > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > File ash/wm/panel_layout_manager_unittest.cc (right): > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > ash/wm/panel_layout_manager_unittest.cc:54: > EXPECT_FALSE(icon_rect.IsEmpty()); > This test fails. The reason for the failure is that, immediately after > the item is added to the launcher, the icon for the item is not > initially visible. It only becomes visible after the animation is > performed (see LauncherView::LauncherItemAdded). What would be the good > way to either wait for animation to end in the test or disable > animations for test? > > https://chromiumcodereview.appspot.com/9808026/
On Tue, Apr 10, 2012 at 1:38 PM, Scott Violet <sky@chromium.org> wrote: > Will panels trigger code that needs to know the bounds of the launcher > item on creation? > I am not sure I understand. Yes, creating the panel does trigger launcher item creation, see CreatePanelWindow in panel_layout_manager_unittest.cc > > -Scott > > On Tue, Apr 10, 2012 at 12:13 PM, <dslomov@chromium.org> wrote: > > Here is an attempt at tests (unfinished). I run into the issue below - > > suggestions are most welcome. > > > > > > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > > File ash/wm/panel_layout_manager_unittest.cc (right): > > > > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > > ash/wm/panel_layout_manager_unittest.cc:54: > > EXPECT_FALSE(icon_rect.IsEmpty()); > > This test fails. The reason for the failure is that, immediately after > > the item is added to the launcher, the icon for the item is not > > initially visible. It only becomes visible after the animation is > > performed (see LauncherView::LauncherItemAdded). What would be the good > > way to either wait for animation to end in the test or disable > > animations for test? > > > > https://chromiumcodereview.appspot.com/9808026/ >
On Tue, Apr 10, 2012 at 1:50 PM, Dmitry Lomov <dslomov@chromium.org> wrote: > > > On Tue, Apr 10, 2012 at 1:38 PM, Scott Violet <sky@chromium.org> wrote: >> >> Will panels trigger code that needs to know the bounds of the launcher >> item on creation? > > > I am not sure I understand. Yes, creating the panel does trigger launcher > item creation, see CreatePanelWindow in panel_layout_manager_unittest.cc But will real code need the target bounds when the launcher item is created? Either way, best to fix LauncherView so it isn't susceptible to this. I think this could be addressed by making LauncherView cache the last_visible_index and having GetIdealBoundsOfItemIcon check against that rather the visible. -Scott >> >> >> -Scott >> >> On Tue, Apr 10, 2012 at 12:13 PM, <dslomov@chromium.org> wrote: >> > Here is an attempt at tests (unfinished). I run into the issue below - >> > suggestions are most welcome. >> > >> > >> > >> > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... >> > File ash/wm/panel_layout_manager_unittest.cc (right): >> > >> > >> > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... >> > ash/wm/panel_layout_manager_unittest.cc:54: >> > EXPECT_FALSE(icon_rect.IsEmpty()); >> > This test fails. The reason for the failure is that, immediately after >> > the item is added to the launcher, the icon for the item is not >> > initially visible. It only becomes visible after the animation is >> > performed (see LauncherView::LauncherItemAdded). What would be the good >> > way to either wait for animation to end in the test or disable >> > animations for test? >> > >> > https://chromiumcodereview.appspot.com/9808026/ > >
On Tue, Apr 10, 2012 at 4:09 PM, Scott Violet <sky@chromium.org> wrote: > On Tue, Apr 10, 2012 at 1:50 PM, Dmitry Lomov <dslomov@chromium.org> > wrote: > > > > > > On Tue, Apr 10, 2012 at 1:38 PM, Scott Violet <sky@chromium.org> wrote: > >> > >> Will panels trigger code that needs to know the bounds of the launcher > >> item on creation? > > > > > > I am not sure I understand. Yes, creating the panel does trigger launcher > > item creation, see CreatePanelWindow in panel_layout_manager_unittest.cc > > But will real code need the target bounds when the launcher item is > created? Yes because that is where the panel needs to be located. > Either way, best to fix LauncherView so it isn't susceptible > to this. I think this could be addressed by making LauncherView cache > the last_visible_index and having GetIdealBoundsOfItemIcon check > against that rather the visible. > I see; so rephrasing, GetScreenBoundsOfItemIconForWindow should return the bounds the icon animates to even if the icon is not visible yet? This makes sense to me. > > -Scott > > >> > >> > >> -Scott > >> > >> On Tue, Apr 10, 2012 at 12:13 PM, <dslomov@chromium.org> wrote: > >> > Here is an attempt at tests (unfinished). I run into the issue below - > >> > suggestions are most welcome. > >> > > >> > > >> > > >> > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > >> > File ash/wm/panel_layout_manager_unittest.cc (right): > >> > > >> > > >> > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > >> > ash/wm/panel_layout_manager_unittest.cc:54: > >> > EXPECT_FALSE(icon_rect.IsEmpty()); > >> > This test fails. The reason for the failure is that, immediately after > >> > the item is added to the launcher, the icon for the item is not > >> > initially visible. It only becomes visible after the animation is > >> > performed (see LauncherView::LauncherItemAdded). What would be the > good > >> > way to either wait for animation to end in the test or disable > >> > animations for test? > >> > > >> > https://chromiumcodereview.appspot.com/9808026/ > > > > >
On Tue, Apr 10, 2012 at 4:32 PM, Dmitry Lomov <dslomov@chromium.org> wrote: > > > On Tue, Apr 10, 2012 at 4:09 PM, Scott Violet <sky@chromium.org> wrote: >> >> On Tue, Apr 10, 2012 at 1:50 PM, Dmitry Lomov <dslomov@chromium.org> >> wrote: >> > >> > >> > On Tue, Apr 10, 2012 at 1:38 PM, Scott Violet <sky@chromium.org> wrote: >> >> >> >> Will panels trigger code that needs to know the bounds of the launcher >> >> item on creation? >> > >> > >> > I am not sure I understand. Yes, creating the panel does trigger >> > launcher >> > item creation, see CreatePanelWindow in panel_layout_manager_unittest.cc >> >> But will real code need the target bounds when the launcher item is >> created? > > > Yes because that is where the panel needs to be located. > >> >> Either way, best to fix LauncherView so it isn't susceptible >> to this. I think this could be addressed by making LauncherView cache >> the last_visible_index and having GetIdealBoundsOfItemIcon check >> against that rather the visible. > > > I see; so rephrasing, GetScreenBoundsOfItemIconForWindow should return the > bounds the icon animates to even if the icon is not visible yet? > This makes sense to me. Yes. Sorry, couldn't help myself with the color there. -Scott >> >> >> -Scott >> >> >> >> >> >> >> -Scott >> >> >> >> On Tue, Apr 10, 2012 at 12:13 PM, <dslomov@chromium.org> wrote: >> >> > Here is an attempt at tests (unfinished). I run into the issue below >> >> > - >> >> > suggestions are most welcome. >> >> > >> >> > >> >> > >> >> > >> >> > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... >> >> > File ash/wm/panel_layout_manager_unittest.cc (right): >> >> > >> >> > >> >> > >> >> > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... >> >> > ash/wm/panel_layout_manager_unittest.cc:54: >> >> > EXPECT_FALSE(icon_rect.IsEmpty()); >> >> > This test fails. The reason for the failure is that, immediately >> >> > after >> >> > the item is added to the launcher, the icon for the item is not >> >> > initially visible. It only becomes visible after the animation is >> >> > performed (see LauncherView::LauncherItemAdded). What would be the >> >> > good >> >> > way to either wait for animation to end in the test or disable >> >> > animations for test? >> >> > >> >> > https://chromiumcodereview.appspot.com/9808026/ >> > >> > > >
On Tue, Apr 10, 2012 at 4:56 PM, Scott Violet <sky@chromium.org> wrote: > On Tue, Apr 10, 2012 at 4:32 PM, Dmitry Lomov <dslomov@chromium.org> > wrote: > > > > > > On Tue, Apr 10, 2012 at 4:09 PM, Scott Violet <sky@chromium.org> wrote: > >> > >> On Tue, Apr 10, 2012 at 1:50 PM, Dmitry Lomov <dslomov@chromium.org> > >> wrote: > >> > > >> > > >> > On Tue, Apr 10, 2012 at 1:38 PM, Scott Violet <sky@chromium.org> > wrote: > >> >> > >> >> Will panels trigger code that needs to know the bounds of the > launcher > >> >> item on creation? > >> > > >> > > >> > I am not sure I understand. Yes, creating the panel does trigger > >> > launcher > >> > item creation, see CreatePanelWindow in > panel_layout_manager_unittest.cc > >> > >> But will real code need the target bounds when the launcher item is > >> created? > > > > > > Yes because that is where the panel needs to be located. > > > >> > >> Either way, best to fix LauncherView so it isn't susceptible > >> to this. I think this could be addressed by making LauncherView cache > >> the last_visible_index and having GetIdealBoundsOfItemIcon check > >> against that rather the visible. > > > > > > I see; so rephrasing, GetScreenBoundsOfItemIconForWindow should return > the > > bounds the icon animates to even if the icon is not visible yet? > > This makes sense to me. > > Yes. Sorry, couldn't help myself with the color there. > I copy-pasted from Rietvield; where do you copy-paste your "yes"es and "no"es from? coloredresponses.com? > > > -Scott > > >> > >> > >> -Scott > >> > >> >> > >> >> > >> >> -Scott > >> >> > >> >> On Tue, Apr 10, 2012 at 12:13 PM, <dslomov@chromium.org> wrote: > >> >> > Here is an attempt at tests (unfinished). I run into the issue > below > >> >> > - > >> >> > suggestions are most welcome. > >> >> > > >> >> > > >> >> > > >> >> > > >> >> > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > >> >> > File ash/wm/panel_layout_manager_unittest.cc (right): > >> >> > > >> >> > > >> >> > > >> >> > > https://chromiumcodereview.appspot.com/9808026/diff/32001/ash/wm/panel_layout... > >> >> > ash/wm/panel_layout_manager_unittest.cc:54: > >> >> > EXPECT_FALSE(icon_rect.IsEmpty()); > >> >> > This test fails. The reason for the failure is that, immediately > >> >> > after > >> >> > the item is added to the launcher, the icon for the item is not > >> >> > initially visible. It only becomes visible after the animation is > >> >> > performed (see LauncherView::LauncherItemAdded). What would be the > >> >> > good > >> >> > way to either wait for animation to end in the test or disable > >> >> > animations for test? > >> >> > > >> >> > https://chromiumcodereview.appspot.com/9808026/ > >> > > >> > > > > > > >
PTAL. Now has tests (also includes test implementation of LauncherDelegate)
Can you also add assertions that LauncherIconObserver is notified when items are added/removed/moved? https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/launcher/launch... File ash/launcher/launcher_view.h (right): https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/launcher/launch... ash/launcher/launcher_view.h:177: // Last index of a launcher button that is visible (does not go into overflow) nit: end with '.'. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... File ash/test/test_launcher_delegate.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.cc:6: #include "ash/test/test_launcher_delegate.h" this should be your first include. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.cc:14: : model_(model) { nit: 4 space indent. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.cc:18: CreateNewWindow(); Any point in having this since CreateNewWindow does nothing? https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.cc:25: //aura::Window* window = watcher_->GetWindowByID(item.id); ? https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.cc:35: return string16(ASCIIToUTF16("")); return string16(); https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.cc:49: if (found == window_to_id_.end()) return 0; avoid single line ifs. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.cc:62: only one newline. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... File ash/test/test_launcher_delegate.h (right): https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.h:20: class TestLauncherDelegate : public LauncherDelegate { Add a description. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.h:24: virtual void CreateNewTab() OVERRIDE; Style for overriden methods is to prefix section with something like: // LauncherDelegate: then overriden methods with no newlines between each method. Typically we put these at the end of the section. In other words AddLauncherItem would be before these. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.h:39: private: nit: newline between 38/39. https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... ash/test/test_launcher_delegate.h:46: }; DISALLOW_COPY_AND_ASSIGN
Thanks for review, I'll fix all the problems. On 2012/04/11 15:47:48, sky wrote: > Can you also add assertions that LauncherIconObserver is notified when items are > added/removed/moved? Do you mean in a separate test for LauncherView? What would be a good test to do that? > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/launcher/launch... > File ash/launcher/launcher_view.h (right): > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/launcher/launch... > ash/launcher/launcher_view.h:177: // Last index of a launcher button that is > visible (does not go into overflow) > nit: end with '.'. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > File ash/test/test_launcher_delegate.cc (right): > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.cc:6: #include > "ash/test/test_launcher_delegate.h" > this should be your first include. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.cc:14: : model_(model) { > nit: 4 space indent. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.cc:18: CreateNewWindow(); > Any point in having this since CreateNewWindow does nothing? > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.cc:25: //aura::Window* window = > watcher_->GetWindowByID(item.id); > ? > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.cc:35: return string16(ASCIIToUTF16("")); > return string16(); > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.cc:49: if (found == window_to_id_.end()) return > 0; > avoid single line ifs. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.cc:62: > only one newline. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > File ash/test/test_launcher_delegate.h (right): > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.h:20: class TestLauncherDelegate : public > LauncherDelegate { > Add a description. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.h:24: virtual void CreateNewTab() OVERRIDE; > Style for overriden methods is to prefix section with something like: > // LauncherDelegate: > then overriden methods with no newlines between each method. > > Typically we put these at the end of the section. In other words AddLauncherItem > would be before these. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.h:39: private: > nit: newline between 38/39. > > https://chromiumcodereview.appspot.com/9808026/diff/38001/ash/test/test_launc... > ash/test/test_launcher_delegate.h:46: }; > DISALLOW_COPY_AND_ASSIGN
On Thu, Apr 12, 2012 at 10:55 AM, <dslomov@chromium.org> wrote: > Thanks for review, I'll fix all the problems. > > > On 2012/04/11 15:47:48, sky wrote: >> >> Can you also add assertions that LauncherIconObserver is notified when >> items > > are >> >> added/removed/moved? > > > Do you mean in a separate test for LauncherView? What would be a good test > to do > that? Yes. Create a launcher_view_unittest add/remove/move some items and make sure the observer is notified. -Scott
So I tried to add some LauncherView tests. Unfortunately anything but Add fails because of animations getting in the way. Ideas? https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... File ash/launcher/launcher_view.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged()); This change is needed for Add test to work. I am not sure I am happy about it. Tests for remove and move will not work because there are animations happening before ideal bounds are calculated
Make it so the time of the animation can be set for tests. That way you could set the time to 0. -Scott On Thu, Apr 12, 2012 at 11:14 PM, <dslomov@chromium.org> wrote: > So I tried to add some LauncherView tests. Unfortunately anything but Add > fails > because of animations getting in the way. > > Ideas? > > > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... > File ash/launcher/launcher_view.cc (right): > > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... > ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged()); > This change is needed for Add test to work. I am not sure I am happy > about it. Tests for remove and move will not work because there are > animations happening before ideal bounds are calculated > > https://chromiumcodereview.appspot.com/9808026/
Hmm ok this is getting hairy.
I tried doing this (see that patch below).
It looks like the code is really not ready to 0-duration animations.
Two problems:
1) SlideAnimation::Show has a bug where, if the duration is 0, it just
slides to end and never notifies the AnimationDelegate that it has ended.
2) Even if that bug was fixed, we will have problems with this pattern e.g.
in LauncherView::LauncherItemRemoved:
// The first animation fades out the view. When done we'll animate the
rest of
// the views to their target location.
bounds_animator_->AnimateViewTo(view, view->bounds());
bounds_animator_->SetAnimationDelegate(
view, new FadeOutAnimationDelegate(this, view), true);
In case of 0-duration, the animation will be over right after AnimateToView
returns; however SetAnimationDelegate asserts that animation should still
be running when it is called.
I feel this patch starts sucking in too much pieces. I suggest we keep
testing LauncherViewObserver for a separate issue.
diff --git a/ui/views/animation/bounds_animator.cc
b/ui/views/animation/bounds_animator.cc
index 2e4b0c1..9a658f9 100644
--- a/ui/views/animation/bounds_animator.cc
+++ b/ui/views/animation/bounds_animator.cc
@@ -7,6 +7,7 @@
#include "base/memory/scoped_ptr.h"
#include "ui/base/animation/animation_container.h"
#include "ui/base/animation/slide_animation.h"
+#include "ui/gfx/compositor/layer_animator.h"
#include "ui/views/view.h"
// Duration in milliseconds for animations.
@@ -140,7 +141,8 @@ void BoundsAnimator::Cancel() {
SlideAnimation* BoundsAnimator::CreateAnimation() {
SlideAnimation* animation = new SlideAnimation(this);
animation->SetContainer(container_.get());
- animation->SetSlideDuration(kAnimationDuration);
+ animation->SetSlideDuration(
+ ui::LayerAnimator::disable_animations_for_test() ? 0 :
kAnimationDuration);
animation->SetTweenType(Tween::EASE_OUT);
return animation;
}
On Fri, Apr 13, 2012 at 8:46 AM, Scott Violet <sky@chromium.org> wrote:
> Make it so the time of the animation can be set for tests. That way
> you could set the time to 0.
> -Scott
>
> On Thu, Apr 12, 2012 at 11:14 PM, <dslomov@chromium.org> wrote:
> > So I tried to add some LauncherView tests. Unfortunately anything but Add
> > fails
> > because of animations getting in the way.
> >
> > Ideas?
> >
> >
> >
>
https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch...
> > File ash/launcher/launcher_view.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch...
> > ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged());
> > This change is needed for Add test to work. I am not sure I am happy
> > about it. Tests for remove and move will not work because there are
> > animations happening before ideal bounds are calculated
> >
> > https://chromiumcodereview.appspot.com/9808026/
>
I'm fine with pushing launcherview test to a separate patch, but remove the test from this patch since it doesn't work. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... File ash/launcher/launcher_icon_observer.h (right): https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_icon_observer.h:1: remove mepty line. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... File ash/launcher/launcher_view.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged()); On 2012/04/13 06:14:14, Dmitry Lomov (chromium) wrote: > This change is needed for Add test to work. I am not sure I am happy about it. > Tests for remove and move will not work because there are animations happening > before ideal bounds are calculated This isn't the right place either. The notification should be sent from LauncherItemRemoved/Added/Moved and OnBoundsChanged. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... File ash/launcher/launcher_view_unittest.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_view_unittest.cc:3: // found in the LICENSE file. I this doesn't work, don't bother checking it in. Move it into a separate patch set. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_view_unittest.cc:25: class LauncherViewTest : public ash::test::AshTestBase { If you don't need to override anything, use a typedef. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_view_unittest.cc:53: private: nit: newline between 52/53. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_view_unittest.cc:65: params.bounds = gfx::Rect(0,0,200,200); nit: spaces after commas. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/test/test_launc... File ash/test/test_launcher_delegate.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/test/test_launc... ash/test/test_launcher_delegate.cc:22: if (id == 0) return; nit: no single newline. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/test/test_launc... ash/test/test_launcher_delegate.cc:60: if (found == window_to_id_.end()) return 0; no single line if. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/test/test_launc... File ash/test/test_launcher_delegate.h (right): https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/test/test_launc... ash/test/test_launcher_delegate.h:38: nit: no newlines between override lines. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/test/test_launc... ash/test/test_launcher_delegate.h:54: typedef std::map<aura::Window*, ash::LauncherID> WindowToID; typedefs should be before methods (style guide has exact ordering).
One comment below. I'll amend the patch per your review otherwise. https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... File ash/launcher/launcher_view.cc (right): https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged()); On 2012/04/13 17:32:15, sky wrote: > On 2012/04/13 06:14:14, Dmitry Lomov (chromium) wrote: > > This change is needed for Add test to work. I am not sure I am happy about it. > > Tests for remove and move will not work because there are animations happening > > before ideal bounds are calculated > > This isn't the right place either. The notification should be sent from > LauncherItemRemoved/Added/Moved and OnBoundsChanged. No not really - we need to send notifiactions *after* ideal bounds has been updated. In case of animations this may happen long after LauncherItemRemoved/Added/Moved and OnBoundsChanged had exited. I am ok with moving this back to after the animation ends.
PTAL (removed the ill-fated test :))
On Fri, Apr 13, 2012 at 10:41 AM, <dslomov@chromium.org> wrote: > One comment below. I'll amend the patch per your review otherwise. > > > > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... > File ash/launcher/launcher_view.cc (right): > > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... > ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged()); > On 2012/04/13 17:32:15, sky wrote: >> >> On 2012/04/13 06:14:14, Dmitry Lomov (chromium) wrote: >> > This change is needed for Add test to work. I am not sure I am happy > > about it. >> >> > Tests for remove and move will not work because there are animations > > happening >> >> > before ideal bounds are calculated > > >> This isn't the right place either. The notification should be sent > > from >> >> LauncherItemRemoved/Added/Moved and OnBoundsChanged. > > > No not really - we need to send notifiactions *after* ideal bounds has > been updated. In case of animations this may happen long after > LauncherItemRemoved/Added/Moved and OnBoundsChanged had exited. I am ok > with moving this back to after the animation ends. All of those update the ideal bounds immediately. It's the actual bounds that animate. -Scott
On Fri, Apr 13, 2012 at 12:46 PM, Scott Violet <sky@chromium.org> wrote: > On Fri, Apr 13, 2012 at 10:41 AM, <dslomov@chromium.org> wrote: > > One comment below. I'll amend the patch per your review otherwise. > > > > > > > > > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... > > File ash/launcher/launcher_view.cc (right): > > > > > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... > > ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged()); > > On 2012/04/13 17:32:15, sky wrote: > >> > >> On 2012/04/13 06:14:14, Dmitry Lomov (chromium) wrote: > >> > This change is needed for Add test to work. I am not sure I am happy > > > > about it. > >> > >> > Tests for remove and move will not work because there are animations > > > > happening > >> > >> > before ideal bounds are calculated > > > > > >> This isn't the right place either. The notification should be sent > > > > from > >> > >> LauncherItemRemoved/Added/Moved and OnBoundsChanged. > > > > > > No not really - we need to send notifiactions *after* ideal bounds has > > been updated. In case of animations this may happen long after > > LauncherItemRemoved/Added/Moved and OnBoundsChanged had exited. I am ok > > with moving this back to after the animation ends. > > All of those update the ideal bounds immediately. It's the actual > bounds that animate. > No, if you look at the code CalculateIdealBounds happen in AnimateToIdeaBounds (which in case of at least remove happens after fade-out animation) > > -Scott >
On Fri, Apr 13, 2012 at 12:53 PM, Dmitry Lomov <dslomov@google.com> wrote: > > > On Fri, Apr 13, 2012 at 12:46 PM, Scott Violet <sky@chromium.org> wrote: >> >> On Fri, Apr 13, 2012 at 10:41 AM, <dslomov@chromium.org> wrote: >> > One comment below. I'll amend the patch per your review otherwise. >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... >> > File ash/launcher/launcher_view.cc (right): >> > >> > >> > https://chromiumcodereview.appspot.com/9808026/diff/39017/ash/launcher/launch... >> > ash/launcher/launcher_view.cc:425: OnLauncherIconPositionsChanged()); >> > On 2012/04/13 17:32:15, sky wrote: >> >> >> >> On 2012/04/13 06:14:14, Dmitry Lomov (chromium) wrote: >> >> > This change is needed for Add test to work. I am not sure I am happy >> > >> > about it. >> >> >> >> > Tests for remove and move will not work because there are animations >> > >> > happening >> >> >> >> > before ideal bounds are calculated >> > >> > >> >> This isn't the right place either. The notification should be sent >> > >> > from >> >> >> >> LauncherItemRemoved/Added/Moved and OnBoundsChanged. >> > >> > >> > No not really - we need to send notifiactions *after* ideal bounds has >> > been updated. In case of animations this may happen long after >> > LauncherItemRemoved/Added/Moved and OnBoundsChanged had exited. I am ok >> > with moving this back to after the animation ends. >> >> All of those update the ideal bounds immediately. It's the actual >> bounds that animate. > > > No, if you look at the code CalculateIdealBounds happen in > AnimateToIdeaBounds (which in case of at least remove happens after fade-out > animation) It should be fine to add a call to CalculateIdealBounds in LauncherItemRemoved before you notify but after existing code. -Scott
Dmitry Lomov's last day in Google was today... Steven, did you mention today that you'll land this patch? Just want to confirm so it doesn't fall through. Let me know if you want me to pick it up.
Xiyuan needs a small portion of this to fix a bug. So he's incorporating that portion into http://codereview.chromium.org/10068027 .
Looks like we lost the description with the issue number somewhere along the way, but I thought that dcheng@ was going to pick this up?
On 2012/04/16 17:33:41, stevenjb (chromium) wrote: > Looks like we lost the description with the issue number somewhere along the > way, but I thought that dcheng@ was going to pick this up? Yes, I've talked with dimich locally. I'll have to create a new issue though, unless someone (maruel) can manually change the owner on this issue.
Probably better off creating a new issue anyway, given how long this one is already :) Also you will need to rebase it once Xiyuan's changes from http://codereview.chromium.**org/10068027<http://codereview.chromium.org/1006... are in. On Mon, Apr 16, 2012 at 10:52 AM, <dcheng@chromium.org> wrote: > On 2012/04/16 17:33:41, stevenjb (chromium) wrote: > >> Looks like we lost the description with the issue number somewhere along >> the >> way, but I thought that dcheng@ was going to pick this up? >> > > Yes, I've talked with dimich locally. I'll have to create a new issue > though, > unless someone (maruel) can manually change the owner on this issue. > > https://chromiumcodereview.**appspot.com/9808026/<https://chromiumcodereview.... > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
