Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26277006:
Created ash::FirstRunHelper. (Closed)
|
Created:
7 years, 2 months ago by dzhioev (left Google) Modified:
7 years, 2 months ago CC:
chromium-reviews, kalyank, sadrul, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreated ash::FirstRunHelper.
FirstRunHelper is an interface providing API for manipulating and retreiving
information about Shell elements. It will be used by new first-run tutorial.
BUG=269286
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229175
Patch Set 1 #
Total comments: 12
Patch Set 2 : Comments addressed. #
Total comments: 6
Patch Set 3 : Renamed class. Changed method signature. #Patch Set 4 : Fixed error in gyp-file. #Patch Set 5 : Rebased. #
Messages
Total messages: 28 (0 generated)
Hi, please review this CL. It's not final version of FirstRunDelegate, bunch of new methods will be added soon.
See comment! https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate_impl.cc (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.cc:44: if (app_list::AppListView* view = shell_->GetAppListView()) { Is there a particular reason why you are not using GetAppListWindow and then window->GetBoundsInScreen()? Is the window too far off from the view?
https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate_impl.cc (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.cc:44: if (app_list::AppListView* view = shell_->GetAppListView()) { On 2013/10/10 20:02:34, Mr4D wrote: > Is there a particular reason why you are not using GetAppListWindow and then > window->GetBoundsInScreen()? Is the window too far off from the view? Because AppList window is much bigger than AppListView (orange border on screenshot http://goo.gl/73OTEV ).
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/26277006/1
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Adding oshima@ for owner's review.
It looks odd to have the class for overlay UI in ash. This is more like app, not the part of ash, isn't it? And you should be able to get this info without being inside ash. Let's discuss options offline. https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate.h (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate.h:10: #include "ui/gfx/rect.h" this can be forward decl. https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate_impl.cc (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.cc:44: if (app_list::AppListView* view = shell_->GetAppListView()) { On 2013/10/10 20:51:11, dzhioev wrote: > On 2013/10/10 20:02:34, Mr4D wrote: > > Is there a particular reason why you are not using GetAppListWindow and then > > window->GetBoundsInScreen()? Is the window too far off from the view? > Because AppList window is much bigger than AppListView (orange border on > screenshot http://goo.gl/73OTEV ). I didn't quite understand as there are many orange borders. From the mock, we should be using the window border? https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate_impl.h (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. ditto https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.h:26: Shell* shell_; Can we just use GetInstance()? Advantage is that it will return NULL after shutdown.
> It looks odd to have the class for overlay UI in ash. > This is more like app, not the part of ash, isn't it? > And you should be able to get this info without being inside ash. > > Let's discuss options offline. We can't discuss it offline because I am in Moscow. I'll write you message in Hangouts. https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate.h (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/10/10 22:47:18, oshima wrote: > no (c) Done. https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate.h:10: #include "ui/gfx/rect.h" On 2013/10/10 22:47:18, oshima wrote: > this can be forward decl. Done. https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate_impl.cc (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.cc:44: if (app_list::AppListView* view = shell_->GetAppListView()) { On 2013/10/10 22:47:18, oshima wrote: > On 2013/10/10 20:51:11, dzhioev wrote: > > On 2013/10/10 20:02:34, Mr4D wrote: > > > Is there a particular reason why you are not using GetAppListWindow and then > > > window->GetBoundsInScreen()? Is the window too far off from the view? > > Because AppList window is much bigger than AppListView (orange border on > > screenshot http://goo.gl/73OTEV ). > > I didn't quite understand as there are many orange borders. > From the mock, we should be using the window border? I've marked AppList window with red (same link as above). For some reason window has wide transparent border. That's how it looks when I try to use window's size instead of view's size: http://goo.gl/eJuzt2 https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate_impl.h (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.h:26: Shell* shell_; On 2013/10/10 22:47:18, oshima wrote: > Can we just use GetInstance()? Advantage is that it will return NULL > after shutdown. OK.
by offline, I mean off the CL. Please feel free to ping me on chat anytime. https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... File ash/first_run/first_run_delegate_impl.cc (right): https://codereview.chromium.org/26277006/diff/1/ash/first_run/first_run_deleg... ash/first_run/first_run_delegate_impl.cc:44: if (app_list::AppListView* view = shell_->GetAppListView()) { On 2013/10/11 18:03:52, dzhioev wrote: > On 2013/10/10 22:47:18, oshima wrote: > > On 2013/10/10 20:51:11, dzhioev wrote: > > > On 2013/10/10 20:02:34, Mr4D wrote: > > > > Is there a particular reason why you are not using GetAppListWindow and > then > > > > window->GetBoundsInScreen()? Is the window too far off from the view? > > > Because AppList window is much bigger than AppListView (orange border on > > > screenshot http://goo.gl/73OTEV ). > > > > I didn't quite understand as there are many orange borders. > > From the mock, we should be using the window border? > I've marked AppList window with red (same link as above). For some reason window > has wide transparent border. > That's how it looks when I try to use window's size instead of view's size: > http://goo.gl/eJuzt2 > Ok, this seems to be because bubbble window renders shadow by itself. We should move to ash shadow at some point but this is ok for the time being.
ash/first_run is ok at least for now. Once we have WM api, we may want to change to use it, but that's separate issue. See other comments below. https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... File ash/first_run/first_run_delegate.h (right): https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { XxxDelegate is for interface to delegate the part of implementation. This is more like information retrieval, so it shouldn't be a delegate. Also what is the benefit of defining a interface for this? Will there be test impl? https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... ash/first_run/first_run_delegate.h:30: virtual gfx::Rect GetLauncherBounds() = 0; what if the launcher is hidden?
https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... File ash/first_run/first_run_delegate.h (right): https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { On 2013/10/11 20:58:44, oshima wrote: > XxxDelegate is for interface to delegate the part of implementation. > This is more like information retrieval, so it shouldn't be a delegate. > > Also what is the benefit of defining a interface for this? > Will there be test impl? It's not only for information retrieval, but also for interacting with shell. Have you some proper name in mind? I'm going to implement stub for testing UI independently of Ash. https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... ash/first_run/first_run_delegate.h:30: virtual gfx::Rect GetLauncherBounds() = 0; On 2013/10/11 20:58:44, oshima wrote: > what if the launcher is hidden? It's not decided yet how to handle this case. Maybe we will unhide shelf temporarily during tutorial.
https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... File ash/first_run/first_run_delegate.h (right): https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { On 2013/10/12 00:02:50, dzhioev wrote: > On 2013/10/11 20:58:44, oshima wrote: > > XxxDelegate is for interface to delegate the part of implementation. > > This is more like information retrieval, so it shouldn't be a delegate. > > > > Also what is the benefit of defining a interface for this? > > Will there be test impl? > > It's not only for information retrieval, but also for interacting with shell. > Have you some proper name in mind? It's still not delegate. Controller/Manager would be more appropriate. By the way, have you considered using a snapshot of the screen to implement this feature, instead of using real, active app_list, launcher widgets? That way, you can avoid dealing with dynamic aspect of ash (not only unhiding, or opening app_list, but launcher bounds may change while syncing, a notification may popup during tutorial etc) > > I'm going to implement stub for testing UI independently of Ash. What kind of scenario you can't test by just using ash impl? https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... ash/first_run/first_run_delegate.h:30: virtual gfx::Rect GetLauncherBounds() = 0; On 2013/10/12 00:02:50, dzhioev wrote: > On 2013/10/11 20:58:44, oshima wrote: > > what if the launcher is hidden? > It's not decided yet how to handle this case. Maybe we will unhide shelf > temporarily during tutorial. This issue sounds same as app_list. Shouldn't this use the same patter as app_list? If we can automatically unhide, then can't we do the same for app list?
On 2013/10/16 16:32:05, oshima wrote: > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > File ash/first_run/first_run_delegate.h (right): > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { > On 2013/10/12 00:02:50, dzhioev wrote: > > On 2013/10/11 20:58:44, oshima wrote: > > > XxxDelegate is for interface to delegate the part of implementation. > > > This is more like information retrieval, so it shouldn't be a delegate. > > > > > > Also what is the benefit of defining a interface for this? > > > Will there be test impl? > > > > It's not only for information retrieval, but also for interacting with shell. > > Have you some proper name in mind? > > It's still not delegate. Controller/Manager would be more appropriate. > > By the way, have you considered using a snapshot of the screen to > implement this feature, instead of using real, active app_list, launcher > widgets? > That way, you can avoid dealing with dynamic aspect of ash > (not only unhiding, or opening app_list, but launcher bounds may change while > syncing, a notification may popup during tutorial etc) > > > > > I'm going to implement stub for testing UI independently of Ash. > > What kind of scenario you can't test by just using ash impl? > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > ash/first_run/first_run_delegate.h:30: virtual gfx::Rect GetLauncherBounds() = > 0; > On 2013/10/12 00:02:50, dzhioev wrote: > > On 2013/10/11 20:58:44, oshima wrote: > > > what if the launcher is hidden? > > It's not decided yet how to handle this case. Maybe we will unhide shelf > > temporarily during tutorial. > > This issue sounds same as app_list. Shouldn't this use the > same patter as app_list? If we can automatically unhide, > then can't we do the same for app list?
Sorry, previous message was sent accidentally. On 2013/10/16 16:32:05, oshima wrote: > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > File ash/first_run/first_run_delegate.h (right): > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { > On 2013/10/12 00:02:50, dzhioev wrote: > > On 2013/10/11 20:58:44, oshima wrote: > > > XxxDelegate is for interface to delegate the part of implementation. > > > This is more like information retrieval, so it shouldn't be a delegate. > > > > > > Also what is the benefit of defining a interface for this? > > > Will there be test impl? > > > > It's not only for information retrieval, but also for interacting with shell. > > Have you some proper name in mind? > > It's still not delegate. Controller/Manager would be more appropriate. > Unfortunately I already have chromeos::FirsrRunController -- class that initiates tutorial and manages tutorial's flow. How do you like FirstRunHelper? > By the way, have you considered using a snapshot of the screen to > implement this feature, instead of using real, active app_list, launcher > widgets? > That way, you can avoid dealing with dynamic aspect of ash > (not only unhiding, or opening app_list, but launcher bounds may change while > syncing, a notification may popup during tutorial etc) > You are proposing to make snapshots before release and put them to resources, right? I think it's not suitable solution. Couple of reasons: * Enormous number of screenshots: we need screenshots for every language X every step of tutorial. * Hard to support: all screenshots should be remade every time we change something in Shelf. > > > > I'm going to implement stub for testing UI independently of Ash. > > What kind of scenario you can't test by just using ash impl? > I want to write lightweight unit tests for chromeos::FirstRunController without using UI at all. > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > ash/first_run/first_run_delegate.h:30: virtual gfx::Rect GetLauncherBounds() = > 0; > On 2013/10/12 00:02:50, dzhioev wrote: > > On 2013/10/11 20:58:44, oshima wrote: > > > what if the launcher is hidden? > > It's not decided yet how to handle this case. Maybe we will unhide shelf > > temporarily during tutorial. > > This issue sounds same as app_list. Shouldn't this use the > same patter as app_list? If we can automatically unhide, > then can't we do the same for app list? Sorry, I can't understand what pattern are you talking about. Can you please clarify?
On 2013/10/16 18:41:50, dzhioev wrote: > Sorry, previous message was sent accidentally. > > On 2013/10/16 16:32:05, oshima wrote: > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > File ash/first_run/first_run_delegate.h (right): > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { > > On 2013/10/12 00:02:50, dzhioev wrote: > > > On 2013/10/11 20:58:44, oshima wrote: > > > > XxxDelegate is for interface to delegate the part of implementation. > > > > This is more like information retrieval, so it shouldn't be a delegate. > > > > > > > > Also what is the benefit of defining a interface for this? > > > > Will there be test impl? > > > > > > It's not only for information retrieval, but also for interacting with > shell. > > > Have you some proper name in mind? > > > > It's still not delegate. Controller/Manager would be more appropriate. > > > > Unfortunately I already have chromeos::FirsrRunController -- class that > initiates > tutorial and manages tutorial's flow. How do you like FirstRunHelper? SGTM > > > By the way, have you considered using a snapshot of the screen to > > implement this feature, instead of using real, active app_list, launcher > > widgets? > > That way, you can avoid dealing with dynamic aspect of ash > > (not only unhiding, or opening app_list, but launcher bounds may change while > > syncing, a notification may popup during tutorial etc) > > > > You are proposing to make snapshots before release and put them to resources, > right? No, take a snapshot of desktop at runtime. This way, you can use latest and personalize destkop image without worrying about dynamic aspect of the desktop (such as keeping app_list open, avoid showing popup/notification, etc) > I think it's not suitable solution. Couple of reasons: > * Enormous number of screenshots: we need screenshots for every language X every > step of tutorial. > * Hard to support: all screenshots should be remade every time we change > something in Shelf. > > > > > > > I'm going to implement stub for testing UI independently of Ash. > > > > What kind of scenario you can't test by just using ash impl? > > > > I want to write lightweight unit tests for chromeos::FirstRunController without > using UI at all. You mean no without ash shell? What kind of test do you have in your mind? (It's not clear to me what'd be meaningful test without UI as this is UI feature) > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > ash/first_run/first_run_delegate.h:30: virtual gfx::Rect GetLauncherBounds() = > > 0; > > On 2013/10/12 00:02:50, dzhioev wrote: > > > On 2013/10/11 20:58:44, oshima wrote: > > > > what if the launcher is hidden? > > > It's not decided yet how to handle this case. Maybe we will unhide shelf > > > temporarily during tutorial. > > > > This issue sounds same as app_list. Shouldn't this use the > > same patter as app_list? If we can automatically unhide, > > then can't we do the same for app list? > Sorry, I can't understand what pattern are you talking about. Can you please > clarify? The difference between bool GetAppListBounds(gfx::Rect&result) and gfx::Rect GetLauncherBounds(); Given that both UI can be hidden and we have to show it when it's hidden, I'd expect both returns gfx::Rect if it'll automatically unhide/show them, or return bool to indicates it's shown or not.
On 2013/10/16 19:34:08, oshima wrote: > On 2013/10/16 18:41:50, dzhioev wrote: > > Sorry, previous message was sent accidentally. > > > > On 2013/10/16 16:32:05, oshima wrote: > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > File ash/first_run/first_run_delegate.h (right): > > > > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { > > > On 2013/10/12 00:02:50, dzhioev wrote: > > > > On 2013/10/11 20:58:44, oshima wrote: > > > > > XxxDelegate is for interface to delegate the part of implementation. > > > > > This is more like information retrieval, so it shouldn't be a delegate. > > > > > > > > > > Also what is the benefit of defining a interface for this? > > > > > Will there be test impl? > > > > > > > > It's not only for information retrieval, but also for interacting with > > shell. > > > > Have you some proper name in mind? > > > > > > It's still not delegate. Controller/Manager would be more appropriate. > > > > > > > Unfortunately I already have chromeos::FirsrRunController -- class that > > initiates > > tutorial and manages tutorial's flow. How do you like FirstRunHelper? > > SGTM > Renamed. > > > > > By the way, have you considered using a snapshot of the screen to > > > implement this feature, instead of using real, active app_list, launcher > > > widgets? > > > That way, you can avoid dealing with dynamic aspect of ash > > > (not only unhiding, or opening app_list, but launcher bounds may change > while > > > syncing, a notification may popup during tutorial etc) > > > > > > > You are proposing to make snapshots before release and put them to resources, > > right? > > No, take a snapshot of desktop at runtime. This way, you can use latest and > personalize destkop image without worrying about dynamic aspect of the desktop > (such as keeping app_list open, avoid showing popup/notification, etc) > But before taking screenshot we have to ensure that shelf/app_list/tray is opened, all notifications are hidden, there are no windows on desktop, etc -- exactly same things that we need for "live" implementation. > > I think it's not suitable solution. Couple of reasons: > > * Enormous number of screenshots: we need screenshots for every language X > every > > step of tutorial. > > * Hard to support: all screenshots should be remade every time we change > > something in Shelf. > > > > > > > > > > I'm going to implement stub for testing UI independently of Ash. > > > > > > What kind of scenario you can't test by just using ash impl? > > > > > > > I want to write lightweight unit tests for chromeos::FirstRunController > without > > using UI at all. > > You mean no without ash shell? What kind of test do you have in your mind? > (It's not clear to me what'd be meaningful test without UI as this is UI > feature) > I want to write tests that covers such things as: * Steps order is correct. * Coordinates of WebUI elements set correctly according to coordinates returned by FirstRunHelper. * Manipulations with Shell occur in a right time. Another reason to have interface is that FirstRunHelperImpl will implement interfaces that are internal to Ash (OverlayEventFilter::Delegate for example, maybe some observers). > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > ash/first_run/first_run_delegate.h:30: virtual gfx::Rect GetLauncherBounds() > = > > > 0; > > > On 2013/10/12 00:02:50, dzhioev wrote: > > > > On 2013/10/11 20:58:44, oshima wrote: > > > > > what if the launcher is hidden? > > > > It's not decided yet how to handle this case. Maybe we will unhide shelf > > > > temporarily during tutorial. > > > > > > This issue sounds same as app_list. Shouldn't this use the > > > same patter as app_list? If we can automatically unhide, > > > then can't we do the same for app list? > > Sorry, I can't understand what pattern are you talking about. Can you please > > clarify? > > The difference between > > bool GetAppListBounds(gfx::Rect&result) > > and > > gfx::Rect GetLauncherBounds(); > > > Given that both UI can be hidden and we have to show it when it's hidden, > I'd expect both returns gfx::Rect if it'll automatically unhide/show them, > or return bool to indicates it's shown or not. It makes no sense to retrieve bounds of closed app list, so I've changed signature of GetAppListBounds and added CHECK inside body. Final implementation will take care that app list is opened after OpenAppList call.
On 2013/10/16 23:34:51, dzhioev wrote: > On 2013/10/16 19:34:08, oshima wrote: > > On 2013/10/16 18:41:50, dzhioev wrote: > > > Sorry, previous message was sent accidentally. > > > > > > On 2013/10/16 16:32:05, oshima wrote: > > > > > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > > File ash/first_run/first_run_delegate.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > > ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate { > > > > On 2013/10/12 00:02:50, dzhioev wrote: > > > > > On 2013/10/11 20:58:44, oshima wrote: > > > > > > XxxDelegate is for interface to delegate the part of implementation. > > > > > > This is more like information retrieval, so it shouldn't be a > delegate. > > > > > > > > > > > > Also what is the benefit of defining a interface for this? > > > > > > Will there be test impl? > > > > > > > > > > It's not only for information retrieval, but also for interacting with > > > shell. > > > > > Have you some proper name in mind? > > > > > > > > It's still not delegate. Controller/Manager would be more appropriate. > > > > > > > > > > Unfortunately I already have chromeos::FirsrRunController -- class that > > > initiates > > > tutorial and manages tutorial's flow. How do you like FirstRunHelper? > > > > SGTM > > > Renamed. > > > > > > > By the way, have you considered using a snapshot of the screen to > > > > implement this feature, instead of using real, active app_list, launcher > > > > widgets? > > > > That way, you can avoid dealing with dynamic aspect of ash > > > > (not only unhiding, or opening app_list, but launcher bounds may change > > while > > > > syncing, a notification may popup during tutorial etc) > > > > > > > > > > You are proposing to make snapshots before release and put them to > resources, > > > right? > > > > No, take a snapshot of desktop at runtime. This way, you can use latest and > > personalize destkop image without worrying about dynamic aspect of the desktop > > (such as keeping app_list open, avoid showing popup/notification, etc) > > > > But before taking screenshot we have to ensure that shelf/app_list/tray is > opened, > all notifications are hidden, there are no windows on desktop, etc -- exactly > same things that > we need for "live" implementation. Maybe you misunderstood me. I'm not proposing alternative approach. I'm suggesting a potential improvement so that you don't have to worry about dynamic aspect of "live" impl. Once you capture the image of current desktop, you can hide everything and use these images in your app. > > > I think it's not suitable solution. Couple of reasons: > > > * Enormous number of screenshots: we need screenshots for every language X > > every > > > step of tutorial. > > > * Hard to support: all screenshots should be remade every time we change > > > something in Shelf. > > > > > > > > > > > > > I'm going to implement stub for testing UI independently of Ash. > > > > > > > > What kind of scenario you can't test by just using ash impl? > > > > > > > > > > I want to write lightweight unit tests for chromeos::FirstRunController > > without > > > using UI at all. > > > > You mean no without ash shell? What kind of test do you have in your mind? > > (It's not clear to me what'd be meaningful test without UI as this is UI > > feature) > > > I want to write tests that covers such things as: > * Steps order is correct. > * Coordinates of WebUI elements set correctly according to coordinates returned > by FirstRunHelper. > * Manipulations with Shell occur in a right time. Sounds like you need ash/UI elements? In any case, we can tell if we need it when you add tests. LGTM for now. > Another reason to have interface is that FirstRunHelperImpl will implement > interfaces that are internal > to Ash (OverlayEventFilter::Delegate for example, maybe some observers). > > > > > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > > ash/first_run/first_run_delegate.h:30: virtual gfx::Rect > GetLauncherBounds() > > = > > > > 0; > > > > On 2013/10/12 00:02:50, dzhioev wrote: > > > > > On 2013/10/11 20:58:44, oshima wrote: > > > > > > what if the launcher is hidden? > > > > > It's not decided yet how to handle this case. Maybe we will unhide shelf > > > > > temporarily during tutorial. > > > > > > > > This issue sounds same as app_list. Shouldn't this use the > > > > same patter as app_list? If we can automatically unhide, > > > > then can't we do the same for app list? > > > Sorry, I can't understand what pattern are you talking about. Can you please > > > clarify? > > > > The difference between > > > > bool GetAppListBounds(gfx::Rect&result) > > > > and > > > > gfx::Rect GetLauncherBounds(); > > > > > > Given that both UI can be hidden and we have to show it when it's hidden, > > I'd expect both returns gfx::Rect if it'll automatically unhide/show them, > > or return bool to indicates it's shown or not. > It makes no sense to retrieve bounds of closed app list, so I've changed > signature of GetAppListBounds > and added CHECK inside body. > Final implementation will take care that app list > is opened after OpenAppList call. ok thanks.
On 2013/10/16 23:52:25, oshima wrote: > On 2013/10/16 23:34:51, dzhioev wrote: > > On 2013/10/16 19:34:08, oshima wrote: > > > On 2013/10/16 18:41:50, dzhioev wrote: > > > > Sorry, previous message was sent accidentally. > > > > > > > > On 2013/10/16 16:32:05, oshima wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > > > File ash/first_run/first_run_delegate.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > > > ash/first_run/first_run_delegate.h:20: class ASH_EXPORT FirstRunDelegate > { > > > > > On 2013/10/12 00:02:50, dzhioev wrote: > > > > > > On 2013/10/11 20:58:44, oshima wrote: > > > > > > > XxxDelegate is for interface to delegate the part of implementation. > > > > > > > This is more like information retrieval, so it shouldn't be a > > delegate. > > > > > > > > > > > > > > Also what is the benefit of defining a interface for this? > > > > > > > Will there be test impl? > > > > > > > > > > > > It's not only for information retrieval, but also for interacting with > > > > shell. > > > > > > Have you some proper name in mind? > > > > > > > > > > It's still not delegate. Controller/Manager would be more appropriate. > > > > > > > > > > > > > Unfortunately I already have chromeos::FirsrRunController -- class that > > > > initiates > > > > tutorial and manages tutorial's flow. How do you like FirstRunHelper? > > > > > > SGTM > > > > > Renamed. > > > > > > > > > By the way, have you considered using a snapshot of the screen to > > > > > implement this feature, instead of using real, active app_list, launcher > > > > > widgets? > > > > > That way, you can avoid dealing with dynamic aspect of ash > > > > > (not only unhiding, or opening app_list, but launcher bounds may change > > > while > > > > > syncing, a notification may popup during tutorial etc) > > > > > > > > > > > > > You are proposing to make snapshots before release and put them to > > resources, > > > > right? > > > > > > No, take a snapshot of desktop at runtime. This way, you can use latest and > > > personalize destkop image without worrying about dynamic aspect of the > desktop > > > (such as keeping app_list open, avoid showing popup/notification, etc) > > > > > > > But before taking screenshot we have to ensure that shelf/app_list/tray is > > opened, > > all notifications are hidden, there are no windows on desktop, etc -- exactly > > same things that > > we need for "live" implementation. > > Maybe you misunderstood me. I'm not proposing alternative approach. I'm > suggesting > a potential improvement so that you don't have to worry about dynamic aspect of > "live" impl. > Once you capture the image of current desktop, you can hide everything and use > these images in your app. > Thanks. I took this into account. > > > > I think it's not suitable solution. Couple of reasons: > > > > * Enormous number of screenshots: we need screenshots for every language X > > > every > > > > step of tutorial. > > > > * Hard to support: all screenshots should be remade every time we change > > > > something in Shelf. > > > > > > > > > > > > > > > > I'm going to implement stub for testing UI independently of Ash. > > > > > > > > > > What kind of scenario you can't test by just using ash impl? > > > > > > > > > > > > > I want to write lightweight unit tests for chromeos::FirstRunController > > > without > > > > using UI at all. > > > > > > You mean no without ash shell? What kind of test do you have in your mind? > > > (It's not clear to me what'd be meaningful test without UI as this is UI > > > feature) > > > > > I want to write tests that covers such things as: > > * Steps order is correct. > > * Coordinates of WebUI elements set correctly according to coordinates > returned > > by FirstRunHelper. > > * Manipulations with Shell occur in a right time. > > Sounds like you need ash/UI elements? In any case, we can tell if we need it > when you add tests. LGTM for now. > > > Another reason to have interface is that FirstRunHelperImpl will implement > > interfaces that are internal > > to Ash (OverlayEventFilter::Delegate for example, maybe some observers). > > > > > > > > > > > > > > > https://codereview.chromium.org/26277006/diff/17001/ash/first_run/first_run_d... > > > > > ash/first_run/first_run_delegate.h:30: virtual gfx::Rect > > GetLauncherBounds() > > > = > > > > > 0; > > > > > On 2013/10/12 00:02:50, dzhioev wrote: > > > > > > On 2013/10/11 20:58:44, oshima wrote: > > > > > > > what if the launcher is hidden? > > > > > > It's not decided yet how to handle this case. Maybe we will unhide > shelf > > > > > > temporarily during tutorial. > > > > > > > > > > This issue sounds same as app_list. Shouldn't this use the > > > > > same patter as app_list? If we can automatically unhide, > > > > > then can't we do the same for app list? > > > > Sorry, I can't understand what pattern are you talking about. Can you > please > > > > clarify? > > > > > > The difference between > > > > > > bool GetAppListBounds(gfx::Rect&result) > > > > > > and > > > > > > gfx::Rect GetLauncherBounds(); > > > > > > > > > Given that both UI can be hidden and we have to show it when it's hidden, > > > I'd expect both returns gfx::Rect if it'll automatically unhide/show them, > > > or return bool to indicates it's shown or not. > > It makes no sense to retrieve bounds of closed app list, so I've changed > > signature of GetAppListBounds > > and added CHECK inside body. > > Final implementation will take care that app list > > is opened after OpenAppList call. > > ok thanks. Thanks. And Happy Birthday!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/26277006/28001
Retried try job too often on linux_chromeos for step(s) ash_unittests, browser_tests, interactive_ui_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/26277006/55001
Retried try job too often on win_x64_rel for step(s) base_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/26277006/75001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/26277006/75001
Message was sent while issue was closed.
Change committed as 229175 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
