Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
186483004:
Add a skeleton Start Page to the experimental app list. (Closed)
|
Created:
6 years, 9 months ago by calamity Modified:
6 years, 7 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ares_change_experimental_animation Visibility:
Public. |
DescriptionAdd a skeleton Start Page to the experimental app list.
This CL adds a basic version of a start page to the experimental app list.
This page contains a WebView that displays the Google Logo and a grey bar
which are currently only placeholders.
This CL also modifies the StartPage WebUI to have the google logo.
BUG=349727
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269752
Patch Set 1 : #
Total comments: 23
Patch Set 2 : rebase #Patch Set 3 : rework #
Total comments: 14
Patch Set 4 : rebase with suggestions #Patch Set 5 : rebase again #
Total comments: 21
Patch Set 6 : address commnets #
Total comments: 6
Patch Set 7 : address comments #
Total comments: 14
Patch Set 8 : address comments, get rid of bad merge #Patch Set 9 : address comments #
Total comments: 10
Patch Set 10 : fix nits, change how we clear the search box #Patch Set 11 : fix initialization order #Messages
Total messages: 21 (0 generated)
This needs some tests :). There might be some infrastructure you can leverage from the tests for the other start page. Also, some screenshots on the bug. https://codereview.chromium.org/186483004/diff/60001/chrome/browser/resources... File chrome/browser/resources/app_list/start_page.html (right): https://codereview.chromium.org/186483004/diff/60001/chrome/browser/resources... chrome/browser/resources/app_list/start_page.html:17: <img id="logo" src="chrome://app-list/images/2x/google_logo.png"> is this the typical way to do it? I think to properly handle HiDPI you need something like (in CSS) #logo { background-image: -webkit-image-set( url('chrome://app-list/IDR_APP_LIST_SEARCH_PAGE_LOGO') 1x, url('chrome://app-list/IDR_APP_LIST_SEARCH_PAGE_LOGO@2x') 2x); } Then in the .cc use stuff like ui::ScaleFactor scale_factor; std::string uncached_path; webui::ParsePathAndScale(url, &path, &scale_factor); int resource_id = /* map from string -> resource_id */ ResourceBundle::GetSharedInstance() .LoadDataResourceBytesForScale(resource_id, scale_factor)); But most things seem to just lean on chrome://theme for this already (i.e. instead of chrome://app-list). Maybe try that? Otherwise you'd need to roll a new handler with the above, similar to what HandleHotwordFilesResourceFilter is doing in start_page_ui.cc already. Also, where is the image (`git add`)? And are there implications re:branding? I don't think we can distribute a google logo with Chromium. Can you instead hook into the instant-extended API? Or, for an initial placeholder perhaps see, e.g., what's done for IDR_APP_LIST_GOOGLE_LOGO_VOICE_SEARCH https://codereview.chromium.org/186483004/diff/60001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/186483004/diff/60001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.cc:148: app_list::switches::IsExperimentalAppListEnabled()) ? contents_.get() Do you still need to handle the "old" start page flag here? (or is it deprecated? can the flags be folded in to each other?). Reading more of the patch.. I actually think we need a separate function -- StartPageService::GetSearchPageContents -- and it should not be passed to the AppsGridView constructor https://codereview.chromium.org/186483004/diff/60001/ui/app_list/app_list_swi... File ui/app_list/app_list_switches.h (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/app_list_swi... ui/app_list/app_list_switches.h:17: APP_LIST_EXPORT extern const char kShowAppListStartPage[]; nit: we should sort these - I think that's the general rule for foo_switches files. Can you fix the whole list? But then I can't decide whether the IsFooEnabled() functions should be sorted.. (they probably don't belong in a foo_switches.h file). Appending is probably fine for them. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/app_li... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/app_li... ui/app_list/views/app_list_view.cc:12: #include "ui/app_list/app_list_switches.h" Remove app_list_view.cc from the diff? These changes don't seem needed https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:331: if (start_page_contents && app_list::switches::IsStartPageWebUIEnabled()) { With StartPageService::GetSearchPageContents(..) this change shouldn't be needed https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:43: SHOW_START_PAGE, Maybe go with SHOW_SEARCH_PAGE? I don't think we should mix the meanings of the current search page unless we are ready to replace it completely. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:91: AppListMainView* app_list_main_view_; // Owned by the views hierarchy. In keeping with the other GetFooView(..) functions, this could perhaps be (a member function) AppListMainView* ContentsView::GetMainView() { return static_cast<AppListMainView*>(parent()); } This would also be a closer match to what's done for the folder view in apps_grid_view.cc. But I think either is OK (both have merits, so I'll let you decide). https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:92: StartPageView* start_page_view_; // Owned by the views hierarchy. This doesn't seem to be used/initialized - remove? https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... ui/app_list/views/start_page_view.h:24: // A view that contains buttons to switch the displayed view in the given This comment is not right. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... ui/app_list/views/start_page_view.h:26: class StartPageView : public views::View, I'd go with SearchPageView (or something other than 'StartPageView' -- since the start page is something else already.
https://codereview.chromium.org/186483004/diff/60001/chrome/browser/resources... File chrome/browser/resources/app_list/start_page.html (right): https://codereview.chromium.org/186483004/diff/60001/chrome/browser/resources... chrome/browser/resources/app_list/start_page.html:17: <img id="logo" src="chrome://app-list/images/2x/google_logo.png"> On 2014/03/10 02:41:54, tapted wrote: > is this the typical way to do it? I think to properly handle HiDPI you need > something like (in CSS) > > #logo { > background-image: -webkit-image-set( > url('chrome://app-list/IDR_APP_LIST_SEARCH_PAGE_LOGO') 1x, > url('chrome://app-list/IDR_APP_LIST_SEARCH_PAGE_LOGO@2x') 2x); > } > > Then in the .cc use stuff like > > ui::ScaleFactor scale_factor; > std::string uncached_path; > webui::ParsePathAndScale(url, &path, &scale_factor); > int resource_id = /* map from string -> resource_id */ > ResourceBundle::GetSharedInstance() > .LoadDataResourceBytesForScale(resource_id, scale_factor)); > > But most things seem to just lean on chrome://theme for this already (i.e. > instead of chrome://app-list). Maybe try that? Otherwise you'd need to roll a > new handler with the above, similar to what HandleHotwordFilesResourceFilter is > doing in start_page_ui.cc already. > It seems to be the way that the local ntp does it. (see chrome-search://local-ntp/local-ntp.html) I don't think that has any issues on the Pixel. > Also, where is the image (`git add`)? And are there implications re:branding? I > don't think we can distribute a google logo with Chromium. Can you instead hook > into the instant-extended API? > This image is already in the chromium codebase for the local ntp (chrome/browser/resources/local_ntp/images/2x). I'm not entirely sure how to do the Instant stuff at the moment and it's definitely out of scope for this CL. > Or, for an initial placeholder perhaps see, e.g., what's done for > IDR_APP_LIST_GOOGLE_LOGO_VOICE_SEARCH https://codereview.chromium.org/186483004/diff/60001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/186483004/diff/60001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.cc:148: app_list::switches::IsExperimentalAppListEnabled()) ? contents_.get() On 2014/03/10 02:41:54, tapted wrote: > Do you still need to handle the "old" start page flag here? (or is it > deprecated? can the flags be folded in to each other?). > > Reading more of the patch.. I actually think we need a separate function -- > StartPageService::GetSearchPageContents -- and it should not be passed to the > AppsGridView constructor xiyuan@ said that he's fine with changing the start page webui and having it only be used for the experimental app launcher. However, we're not sure what the responsibilities the webui will have yet (at the moment, the only thing I'm using it for is a web surface to have a logo, hopefully allowing us to hook into the instant extended stuff). I think we can remove the other flag. Adding xiyuan to confirm. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/app_list_swi... File ui/app_list/app_list_switches.h (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/app_list_swi... ui/app_list/app_list_switches.h:17: APP_LIST_EXPORT extern const char kShowAppListStartPage[]; On 2014/03/10 02:41:54, tapted wrote: > nit: we should sort these - I think that's the general rule for foo_switches > files. Can you fix the whole list? > > But then I can't decide whether the IsFooEnabled() functions should be sorted.. > (they probably don't belong in a foo_switches.h file). Appending is probably > fine for them. Sorted these somewhere else. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/app_li... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/app_li... ui/app_list/views/app_list_view.cc:12: #include "ui/app_list/app_list_switches.h" On 2014/03/10 02:41:54, tapted wrote: > Remove app_list_view.cc from the diff? These changes don't seem needed Done. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/apps_g... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/apps_g... ui/app_list/views/apps_grid_view.cc:331: if (start_page_contents && app_list::switches::IsStartPageWebUIEnabled()) { On 2014/03/10 02:41:54, tapted wrote: > With StartPageService::GetSearchPageContents(..) this change shouldn't be needed Holding off on this in case we can just remove the web contents from the AppsGridView entirely. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:43: SHOW_START_PAGE, On 2014/03/10 02:41:54, tapted wrote: > Maybe go with SHOW_SEARCH_PAGE? I don't think we should mix the meanings of the > current search page unless we are ready to replace it completely. See below. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:91: AppListMainView* app_list_main_view_; // Owned by the views hierarchy. On 2014/03/10 02:41:54, tapted wrote: > In keeping with the other GetFooView(..) functions, this could perhaps be (a > member function) > > AppListMainView* ContentsView::GetMainView() { > return static_cast<AppListMainView*>(parent()); > } > > This would also be a closer match to what's done for the folder view in > apps_grid_view.cc. But I think either is OK (both have merits, so I'll let you > decide). Done. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:92: StartPageView* start_page_view_; // Owned by the views hierarchy. On 2014/03/10 02:41:54, tapted wrote: > This doesn't seem to be used/initialized - remove? Done. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... ui/app_list/views/start_page_view.h:24: // A view that contains buttons to switch the displayed view in the given On 2014/03/10 02:41:54, tapted wrote: > This comment is not right. Done. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... ui/app_list/views/start_page_view.h:26: class StartPageView : public views::View, On 2014/03/10 02:41:54, tapted wrote: > I'd go with SearchPageView (or something other than 'StartPageView' -- since the > start page is something else already. I asked xiyuan@ and he said that the start page webui isn't being developed anymore, and is a good thing to use for the experimental app list. I think we can stick with StartPageView and eventually remove the start_page_view from AppsGridView. Otherwise we'll end up with an equally confusing start_page_web_contents inside a SearchPageView. Also, just FYI, the speech contents isn't at all used to display the speech UI, it's only there to talk to the speech plugin.
https://codereview.chromium.org/186483004/diff/60001/chrome/browser/resources... File chrome/browser/resources/app_list/start_page.html (right): https://codereview.chromium.org/186483004/diff/60001/chrome/browser/resources... chrome/browser/resources/app_list/start_page.html:17: <img id="logo" src="chrome://app-list/images/2x/google_logo.png"> On 2014/03/14 07:50:02, calamity wrote: > On 2014/03/10 02:41:54, tapted wrote: > > is this the typical way to do it? I think to properly handle HiDPI you need > > something like (in CSS) > > > > #logo { > > background-image: -webkit-image-set( > > url('chrome://app-list/IDR_APP_LIST_SEARCH_PAGE_LOGO') 1x, > > url('chrome://app-list/IDR_APP_LIST_SEARCH_PAGE_LOGO@2x') 2x); > > } > > > > Then in the .cc use stuff like > > > > ui::ScaleFactor scale_factor; > > std::string uncached_path; > > webui::ParsePathAndScale(url, &path, &scale_factor); > > int resource_id = /* map from string -> resource_id */ > > ResourceBundle::GetSharedInstance() > > .LoadDataResourceBytesForScale(resource_id, scale_factor)); > > > > But most things seem to just lean on chrome://theme for this already (i.e. > > instead of chrome://app-list). Maybe try that? Otherwise you'd need to roll a > > new handler with the above, similar to what HandleHotwordFilesResourceFilter > is > > doing in start_page_ui.cc already. > > > It seems to be the way that the local ntp does it. (see > chrome-search://local-ntp/local-ntp.html) I don't think that has any issues on > the Pixel. > > Also, where is the image (`git add`)? And are there implications re:branding? > I > > don't think we can distribute a google logo with Chromium. Can you instead > hook > > into the instant-extended API? > > > > This image is already in the chromium codebase for the local ntp > (chrome/browser/resources/local_ntp/images/2x). I'm not entirely sure how to do > the Instant stuff at the moment and it's definitely out of scope for this CL. > > Or, for an initial placeholder perhaps see, e.g., what's done for > > IDR_APP_LIST_GOOGLE_LOGO_VOICE_SEARCH > I agree with Trent that we should follow the -webkit-image-set approach and probably should use chrome:://theme. The local-ntp way is more like a shortcut way. Also, we should use background-image of a div instead of a <img> whe we can. The local-ntp page is providing logo in that way. The benefits of using background-image is that we have one less DOM node and the image is not draggable or selectable, does not have a default broken placeholder etc. It is preferable than using a <img>. https://codereview.chromium.org/186483004/diff/60001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/186483004/diff/60001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.cc:148: app_list::switches::IsExperimentalAppListEnabled()) ? contents_.get() On 2014/03/14 07:50:02, calamity wrote: > On 2014/03/10 02:41:54, tapted wrote: > > Do you still need to handle the "old" start page flag here? (or is it > > deprecated? can the flags be folded in to each other?). > > > > Reading more of the patch.. I actually think we need a separate function -- > > StartPageService::GetSearchPageContents -- and it should not be passed to the > > AppsGridView constructor > > xiyuan@ said that he's fine with changing the start page webui and having it > only be used for the experimental app launcher. However, we're not sure what the > responsibilities the webui will have yet (at the moment, the only thing I'm > using it for is a web surface to have a logo, hopefully allowing us to hook into > the instant extended stuff). > > I think we can remove the other flag. Adding xiyuan to confirm. The last I talked to Alex, my impression is that we would use it for the new launcher 2.0 UI (assuming that is what experimental launcher is about). So it is actual the right thing to change/reuse the start page content. One thing we need to be careful is the voice recognition code, as it is also hosted in the start page contents and is actually being used and released. https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/60001/ui/app_list/views/start_... ui/app_list/views/start_page_view.h:26: class StartPageView : public views::View, On 2014/03/14 07:50:02, calamity wrote: > On 2014/03/10 02:41:54, tapted wrote: > > I'd go with SearchPageView (or something other than 'StartPageView' -- since > the > > start page is something else already. > > I asked xiyuan@ and he said that the start page webui isn't being developed > anymore, and is a good thing to use for the experimental app list. > > I think we can stick with StartPageView and eventually remove the > start_page_view from AppsGridView. Otherwise we'll end up with an equally > confusing start_page_web_contents inside a SearchPageView. > > Also, just FYI, the speech contents isn't at all used to display the speech UI, > it's only there to talk to the speech plugin. StartPage UI is never finished (my fault) and think launcher 2.0 UI would replace it. Getting it out of grid view would be good and made my two bugs obsolete (http://crbug.com/333911 and http://crbug.com/329561). So ping me when this happens. And yes, the speech UI is in views and not part of the start page. The start page is only a means to host the hotword plugin and provide speech recognition functionality. https://codereview.chromium.org/186483004/diff/140001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/186483004/diff/140001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1598: SINGLE_VALUE_TYPE(app_list::switches::kShowAppListStartPage) Do we want to just replace this flag with the experimental app list flag? https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/start_page_service.cc:149: app_list::switches::IsExperimentalAppListEnabled()) A || A is strange. We should just keep one. https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/start_page_service.cc:158: content::WebContents* StartPageService::GetSpeechRecognitionContents() { Unintended copy-n-paste? https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/start_page_service_factory.cc (right): https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/start_page_service_factory.cc:26: return NULL; nit: fix the wrong indent since you touched here. :) https://codereview.chromium.org/186483004/diff/140001/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/186483004/diff/140001/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:29: const char kShowAppListStartPage[] = "show-app-list-start-page"; Can we get rid of this and use just kEnableExperimentalAppList? Start page is dead and replaced by the new UI. It should be okay to drop it. https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:73: AppListMainView* GetMainView(); Seems not used. And casting parent is not safe, if we need this, I would prefer to keep a copy of AppListMainView passed in ctor and use that. https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:28: explicit StartPageView(AppListMainView* app_list_main_view, nit: explicit is not needed.
Finally got back on this. Rebased this on top of the local NTP change. https://codereview.chromium.org/186483004/diff/140001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/186483004/diff/140001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1598: SINGLE_VALUE_TYPE(app_list::switches::kShowAppListStartPage) On 2014/03/14 17:00:54, xiyuan wrote: > Do we want to just replace this flag with the experimental app list flag? Done. https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/start_page_service.cc:149: app_list::switches::IsExperimentalAppListEnabled()) On 2014/03/14 17:00:54, xiyuan wrote: > A || A is strange. We should just keep one. Done. https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/start_page_service.cc:158: content::WebContents* StartPageService::GetSpeechRecognitionContents() { On 2014/03/14 17:00:54, xiyuan wrote: > Unintended copy-n-paste? Done. https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/start_page_service_factory.cc (right): https://codereview.chromium.org/186483004/diff/140001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/start_page_service_factory.cc:26: return NULL; On 2014/03/14 17:00:54, xiyuan wrote: > nit: fix the wrong indent since you touched here. :) Done. https://codereview.chromium.org/186483004/diff/140001/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/186483004/diff/140001/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:29: const char kShowAppListStartPage[] = "show-app-list-start-page"; On 2014/03/14 17:00:54, xiyuan wrote: > Can we get rid of this and use just kEnableExperimentalAppList? Start page is > dead and replaced by the new UI. It should be okay to drop it. Done. https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:73: AppListMainView* GetMainView(); On 2014/03/14 17:00:54, xiyuan wrote: > Seems not used. And casting parent is not safe, if we need this, I would prefer > to keep a copy of AppListMainView passed in ctor and use that. Done. https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/140001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:28: explicit StartPageView(AppListMainView* app_list_main_view, On 2014/03/14 17:00:54, xiyuan wrote: > nit: explicit is not needed. Done.
there might be a better way to do the layout.. I'll defer to xiyuan on the views stuff :) https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:98: if (apps_container_view_->app_list_folder_view() oops - just realised this came in from a rebase, but I commented anyway. My question was: does this work if the folder UI is disabled via the flag? (e.g. is app_list_folder_view() guaranteed non-null?) https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:91: AppListMainView* app_list_main_view_; nit: add ownership comment - e.g. // Parent view, owns this. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:29: class FakeBarButton : public views::CustomButton { needs a class comment. Also FakeFoo sounds odd outside of a test. If the comment would be along the lines of ~this is a placeholder until we get assets - maybe better to just to pick an appropriately sized constant image from ui_resources.grd https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:92: rect.y(), this looks out of place - I think only the width/height of GetContentsBounds matter -- x/y should always be zero unless borders are involved. Maybe just a literal 0 here? https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:94: rect.height() / 2); Why /2 here? (doesn't seem to match the comment - to fill) https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:17: class BoundsAnimator; nit: not used?
https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:98: if (apps_container_view_->app_list_folder_view() On 2014/05/07 08:19:17, tapted wrote: > oops - just realised this came in from a rebase, but I commented anyway. My > question was: does this work if the folder UI is disabled via the flag? (e.g. is > app_list_folder_view() guaranteed non-null?) My impression is that folder UI views are all created but not functioning when folder UI is disabled (i.e. no folder is shown in root grid and no way to create folder). It should be a noop when folder UI is disabled. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:128: app_list_main_view_->search_box_view()->SetVisible(show_state_ != I prefer to add a method to AppListMainView and keep it there. e.g. void AppListMainView::OnContentsViewShowStateChanged() { search_box_view()->SetVisible( contents_view_->show_state() != ContentsView::SHOW_START_PAGE); } https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:70: instant_container_->SetLayoutManager(new views::BoxLayout( Why do we need |instant_container_|? Since app list is fixed size, we can use constant padding. And BoxLayout's spacing args and/or adding EmptyBorder to StartPageView are probably sufficient. If setting them right, we can get rid of Layout. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:79: instant_container_->AddChildView(new FakeBarButton(this)); Looks like we are using FakeBarButton as the placeholder searchbox that triggers the real searchbox? If so, why do it this way instead of doing it in webui? What if we need later to add some contents below the bar? https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:92: rect.y(), On 2014/05/07 08:19:17, tapted wrote: > this looks out of place - I think only the width/height of GetContentsBounds > matter -- x/y should always be zero unless borders are involved. Maybe just a > literal 0 here? x/y might not be zero when a view has padding around it's content area, i.e. GetInsets() returns non-empty Insets. rect.y() is good here. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:101: app_list_main_view_->search_box_view()->RequestFocus(); This feels strange that when user clicks on place A, focus goes to place B. Another concern is whether user is able to restore the UI and get the real search-box hidden again?
https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:98: if (apps_container_view_->app_list_folder_view() On 2014/05/07 17:32:16, xiyuan wrote: > On 2014/05/07 08:19:17, tapted wrote: > > oops - just realised this came in from a rebase, but I commented anyway. My > > question was: does this work if the folder UI is disabled via the flag? (e.g. > is > > app_list_folder_view() guaranteed non-null?) > > My impression is that folder UI views are all created but not functioning when > folder UI is disabled (i.e. no folder is shown in root grid and no way to create > folder). It should be a noop when folder UI is disabled. Yeah, seems to be that way. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:128: app_list_main_view_->search_box_view()->SetVisible(show_state_ != On 2014/05/07 17:32:16, xiyuan wrote: > I prefer to add a method to AppListMainView and keep it there. > > e.g. > void AppListMainView::OnContentsViewShowStateChanged() { > search_box_view()->SetVisible( > contents_view_->show_state() != ContentsView::SHOW_START_PAGE); > } Done. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:91: AppListMainView* app_list_main_view_; On 2014/05/07 08:19:17, tapted wrote: > nit: add ownership comment - e.g. // Parent view, owns this. Done. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:29: class FakeBarButton : public views::CustomButton { On 2014/05/07 08:19:17, tapted wrote: > needs a class comment. Also FakeFoo sounds odd outside of a test. If the comment > would be along the lines of ~this is a placeholder until we get assets - maybe > better to just to pick an appropriately sized constant image from > ui_resources.grd Ok. This was named after the NTP's UI element which has been referred to has the fakebar. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:70: instant_container_->SetLayoutManager(new views::BoxLayout( On 2014/05/07 17:32:16, xiyuan wrote: > Why do we need |instant_container_|? Since app list is fixed size, we can use > constant padding. And BoxLayout's spacing args and/or adding EmptyBorder to > StartPageView are probably sufficient. If setting them right, we can get rid of > Layout. This is mostly in preparation for adding the app tiles below the halfway point and the search results in the top half. We will need to replace the instant container with the search results when the search box button is clicked while keeping the tiles visible. Having the instant_container is a useful way to group the instant elements. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:79: instant_container_->AddChildView(new FakeBarButton(this)); On 2014/05/07 17:32:16, xiyuan wrote: > Looks like we are using FakeBarButton as the placeholder searchbox that triggers > the real searchbox? If so, why do it this way instead of doing it in webui? What > if we need later to add some contents below the bar? We will be adding app tiles below the bar. It's unclear whether we want to do this via views or the webui. The current approach seems to be to use views where possible since webui is often slow. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:94: rect.height() / 2); On 2014/05/07 08:19:17, tapted wrote: > Why /2 here? (doesn't seem to match the comment - to fill) Deleted this whole thing. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:101: app_list_main_view_->search_box_view()->RequestFocus(); On 2014/05/07 17:32:16, xiyuan wrote: > This feels strange that when user clicks on place A, focus goes to place B. This is the behavior specified by the mocks. The mocks have a nice animation for the bar which masks the focus shift. > > Another concern is whether user is able to restore the UI and get the real > search-box hidden again? Not at present. The view will need to change for the search box's behavior but I think that's beyond the scope of this CL. https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/180001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:17: class BoundsAnimator; On 2014/05/07 08:19:17, tapted wrote: > nit: not used? Done.
https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/app_l... File ui/app_list/views/app_list_main_view.cc (right): https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/app_l... ui/app_list/views/app_list_main_view.cc:25: #include "ui/app_list/views/contents_view.h" nit: no longer needed https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:128: app_list_main_view_->search_box_view()->SetVisible(show_state_ != call app_list_main_view_->OnContentsViewShowStateChanged(show_state_)? https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:70: items_container_(new views::View) { is this used? (stray?)
https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/app_l... File ui/app_list/views/app_list_main_view.cc (right): https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/app_l... ui/app_list/views/app_list_main_view.cc:25: #include "ui/app_list/views/contents_view.h" On 2014/05/08 08:19:50, tapted wrote: > nit: no longer needed Done. https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:128: app_list_main_view_->search_box_view()->SetVisible(show_state_ != On 2014/05/08 08:19:50, tapted wrote: > call app_list_main_view_->OnContentsViewShowStateChanged(show_state_)? Done. https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/220001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:70: items_container_(new views::View) { On 2014/05/08 08:19:50, tapted wrote: > is this used? (stray?) Done.
https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:121: AddChildView(items_container_); I think your branches are still a little whacky.
https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:18: #include "ui/app_list/views/search_box_view.h" nit: do we still need this? https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:127: // Don't show the search box for the start page. nit: update comment. It's more like "Notify parent AppListMainView about show state change". https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:128: app_list_main_view_->OnContentsViewShowStateChanged(show_state_); nit: Slightly prefer to add a show_state() accessor than passing it in here. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:91: AppListMainView* app_list_main_view_; // Owned by the views hierarchy. Think tapted@'s suggesiont is better. "// Parent view, owns this." It is more useful to point out |app_list_main_view_| is parent of ContentsView. |apps_container_view_| above is a child view. Using similar comments would mislead reader to think |app_list_main_view_| is also a child. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:141: app_list_main_view_->search_box_view()->RequestFocus(); Similar to Contents view, prefer to do these in a method of AppListMainView, e.g. OnStartPageSearchPressed or something and manipulate search box there. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:35: views::View* instant_container_; // Owned by views hierarchy. Make the comments in line 34 and here different and point out |app_list_main_view_| is the parent. It is confusing in the current format.
Refreshed the whole branch >_> Hopefully the extraneous changes are gone. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:18: #include "ui/app_list/views/search_box_view.h" On 2014/05/08 16:17:48, xiyuan wrote: > nit: do we still need this? Done. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:127: // Don't show the search box for the start page. On 2014/05/08 16:17:48, xiyuan wrote: > nit: update comment. It's more like "Notify parent AppListMainView about show > state change". Done. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:128: app_list_main_view_->OnContentsViewShowStateChanged(show_state_); On 2014/05/08 16:17:48, xiyuan wrote: > nit: Slightly prefer to add a show_state() accessor than passing it in here. Done. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:91: AppListMainView* app_list_main_view_; // Owned by the views hierarchy. On 2014/05/08 16:17:48, xiyuan wrote: > Think tapted@'s suggesiont is better. "// Parent view, owns this." > > It is more useful to point out |app_list_main_view_| is parent of ContentsView. > |apps_container_view_| above is a child view. Using similar comments would > mislead reader to think |app_list_main_view_| is also a child. Done. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:121: AddChildView(items_container_); On 2014/05/08 10:15:22, tapted wrote: > I think your branches are still a little whacky. Done. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:141: app_list_main_view_->search_box_view()->RequestFocus(); On 2014/05/08 16:17:48, xiyuan wrote: > Similar to Contents view, prefer to do these in a method of AppListMainView, > e.g. OnStartPageSearchPressed or something and manipulate search box there. Done. https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/240001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:35: views::View* instant_container_; // Owned by views hierarchy. On 2014/05/08 16:17:48, xiyuan wrote: > Make the comments in line 34 and here different and point out > |app_list_main_view_| is the parent. It is confusing in the current format. Done.
LGTM with nits https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/app_l... File ui/app_list/views/app_list_main_view.h (left): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/app_l... ui/app_list/views/app_list_main_view.h:29: class ContentsView; nit: think we can restore this and get rid of contents_view.h include. https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:70: ShowState show_state() { return show_state_; } nit: make it a const method: i.e. ShowState show_state() const {...} https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:10: #include "ui/app_list/views/search_box_view.h" nit: not needed any more. https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:72: SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0)); I assume BoxLayout here is for adding more child views in the future. https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:21: class StartPageView : public views::View, public views::ButtonListener { nit: does the style guid mandate one parent class per line?
lgtm!
https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/app_l... File ui/app_list/views/app_list_main_view.h (left): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/app_l... ui/app_list/views/app_list_main_view.h:29: class ContentsView; On 2014/05/09 07:59:46, xiyuan wrote: > nit: think we can restore this and get rid of contents_view.h include. Done. https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:70: ShowState show_state() { return show_state_; } On 2014/05/09 07:59:46, xiyuan wrote: > nit: make it a const method: > i.e. ShowState show_state() const {...} Done. https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:10: #include "ui/app_list/views/search_box_view.h" On 2014/05/09 07:59:46, xiyuan wrote: > nit: not needed any more. Done. https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:72: SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0)); On 2014/05/09 07:59:46, xiyuan wrote: > I assume BoxLayout here is for adding more child views in the future. Yep. https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/186483004/diff/280001/ui/app_list/views/start... ui/app_list/views/start_page_view.h:21: class StartPageView : public views::View, public views::ButtonListener { On 2014/05/09 07:59:46, xiyuan wrote: > nit: does the style guid mandate one parent class per line? This is what git cl format wants.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/186483004/320001
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/186483004/340001
Message was sent while issue was closed.
Change committed as 269752 |
