Add a DummySearchBoxView to the experimental app list start page.

Add a DummySearchBoxView to the experimental app list start page.

This CL adds a DummySearchBoxView to the experimental app list which
forwards its input to the AppListMainView search box in a similar way
to the NTP's fakebox.

BUG=349727
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278588

[tapted, 2014-06-18 02:46:06]

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:183:
search_box_view_->search_box()->SetText(new_contents);
will this trigger AppListMainView::QueryChanged(SearchBoxView* sender)? If yes,
the lines above and below are probably redundant, since QueryChanged calls
UpdateSearchBoxVisibility

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:27: // Layout constants
nit: full-stop at the end of comments. More below.

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:39: SkColorSetARGB(0x30, 0xD0, 0xD0, 0xD0);
If the thing behind the color is always solid, I think it's better just to
describe this in RGB and have an opaque alpha. (and we probably wouldn't want
things behind just showing through the border anyway)

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:51: explicit
DummySearchBoxView(SearchBoxViewDelegate* delegate,
nit: explicit not required

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:66: class DummySearchBoxBackground : public
views::Background {
this class needs a brief comment describing what Paint draws

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:149: search_box_view_->set_background(new
DummySearchBoxBackground());
maybe do this in the DummySearchBoxView constructor? (you'll need to reorder
some declarations, or de-inline the constructor)

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:240:
sender->search_box()->SetText(base::string16());
each SearchBoxView has a reference to the the same AppListModel* right? Won't
this cause the query stored in the model to get reset via
SearchBoxView::ContentsChanged() -> UpdateModel() ?

[calamity, 2014-06-18 06:49:40]

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:183:
search_box_view_->search_box()->SetText(new_contents);
On 2014/06/18 02:46:05, tapted wrote:
> will this trigger AppListMainView::QueryChanged(SearchBoxView* sender)? If
yes,
> the lines above and below are probably redundant, since QueryChanged calls
> UpdateSearchBoxVisibility

Unfortunately QueryChanged() isn't triggered by SetText() as part of Textfield's
design.

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:27: // Layout constants
On 2014/06/18 02:46:05, tapted wrote:
> nit: full-stop at the end of comments. More below.

Done.

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:39: SkColorSetARGB(0x30, 0xD0, 0xD0, 0xD0);
On 2014/06/18 02:46:06, tapted wrote:
> If the thing behind the color is always solid, I think it's better just to
> describe this in RGB and have an opaque alpha. (and we probably wouldn't want
> things behind just showing through the border anyway)

Done. Moved this to app_list_constants.h

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:51: explicit
DummySearchBoxView(SearchBoxViewDelegate* delegate,
On 2014/06/18 02:46:05, tapted wrote:
> nit: explicit not required

Done.

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:66: class DummySearchBoxBackground : public
views::Background {
On 2014/06/18 02:46:06, tapted wrote:
> this class needs a brief comment describing what Paint draws

Done.

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:149: search_box_view_->set_background(new
DummySearchBoxBackground());
On 2014/06/18 02:46:06, tapted wrote:
> maybe do this in the DummySearchBoxView constructor? (you'll need to reorder
> some declarations, or de-inline the constructor)

Done.

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:240:
sender->search_box()->SetText(base::string16());
On 2014/06/18 02:46:06, tapted wrote:
> each SearchBoxView has a reference to the the same AppListModel* right? Won't
> this cause the query stored in the model to get reset via
> SearchBoxView::ContentsChanged() -> UpdateModel() ?

Hmm. This doesn't _actually_ affect anything due to the way Textfield::SetText()
works (doesn't notify about changes) but it would be better to remove the link
to the model completely.

How about changing SearchBoxView::ModelChanged() to SetModel() so that clients
need to explicitly set the model rather than pulling it from the given view
delegate.

[calamity, 2014-06-18 07:08:30]

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/335343002/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:240:
sender->search_box()->SetText(base::string16());
On 2014/06/18 06:49:40, calamity wrote:
> On 2014/06/18 02:46:06, tapted wrote:
> > each SearchBoxView has a reference to the the same AppListModel* right?
Won't
> > this cause the query stored in the model to get reset via
> > SearchBoxView::ContentsChanged() -> UpdateModel() ?
> 
> Hmm. This doesn't _actually_ affect anything due to the way
Textfield::SetText()
> works (doesn't notify about changes) but it would be better to remove the link
> to the model completely.
> 
> How about changing SearchBoxView::ModelChanged() to SetModel() so that clients
> need to explicitly set the model rather than pulling it from the given view
> delegate.

Actually, it looks like this isn't exactly easy. The search box model also
contains information about what the speech button looks like, etc. I think we
can leave this as is for now and slowly move stuff that needs customisation into
the SearchBoxViewDelegate?

At the moment the dummy search box will have a menu on non-chromeos.

[calamity, 2014-06-19 06:43:17]

Added a test. Leaving the rest as is as discussed.

[tapted, 2014-06-19 07:29:25]

sweeet lgtm. Thanks for adding a test. Just some suggestions in the testing code

https://codereview.chromium.org/335343002/diff/40001/ui/app_list/app_list_con...
File ui/app_list/app_list_constants.cc (right):

https://codereview.chromium.org/335343002/diff/40001/ui/app_list/app_list_con...
ui/app_list/app_list_constants.cc:33: const SkColor kStartPageBorderColor =
SkColorSetARGB(0xFF, 0xEE, 0xEE, 0xEE);
nit: can just use SkColorSetRGB

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:486: 
// Check that the current search is using |search_text|.
EXPECT_EQ(search_text, delegate_->GetTestModel()->search_box()->text());

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:491:
main_view->search_box_view()->search_box()->InsertText(search_text);
perhaps a different search text? Then you can verify that the search in the
model changed.

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:496: // Check that the dummy search
box is cleared when reshowing the start page.
Should the dummy search box be already cleared here too? Since, focus has
changed to the real one already.

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:498:
contents_view->GetPageIndexForNamedPage(ContentsView::NAMED_PAGE_APPS));
did you mean to do this as well as the next line? Or, maybe rename
ShowContentsViewPage -> ShowContentsViewPageAndVerify if that's the intention.

[calamity, 2014-06-19 08:21:47]

https://codereview.chromium.org/335343002/diff/40001/ui/app_list/app_list_con...
File ui/app_list/app_list_constants.cc (right):

https://codereview.chromium.org/335343002/diff/40001/ui/app_list/app_list_con...
ui/app_list/app_list_constants.cc:33: const SkColor kStartPageBorderColor =
SkColorSetARGB(0xFF, 0xEE, 0xEE, 0xEE);
On 2014/06/19 07:29:24, tapted wrote:
> nit: can just use SkColorSetRGB

Done.

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:486: 
On 2014/06/19 07:29:25, tapted wrote:
> // Check that the current search is using |search_text|.
> EXPECT_EQ(search_text, delegate_->GetTestModel()->search_box()->text());

Done.

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:491:
main_view->search_box_view()->search_box()->InsertText(search_text);
On 2014/06/19 07:29:25, tapted wrote:
> perhaps a different search text? Then you can verify that the search in the
> model changed.

Done.

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:496: // Check that the dummy search
box is cleared when reshowing the start page.
On 2014/06/19 07:29:25, tapted wrote:
> Should the dummy search box be already cleared here too? Since, focus has
> changed to the real one already.

Done.

https://codereview.chromium.org/335343002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:498:
contents_view->GetPageIndexForNamedPage(ContentsView::NAMED_PAGE_APPS));
On 2014/06/19 07:29:24, tapted wrote:
> did you mean to do this as well as the next line? Or, maybe rename
> ShowContentsViewPage -> ShowContentsViewPageAndVerify if that's the intention.

In this case I wanted to reset the start page by showing the apps page and then
the start page. In any case that's a better name.

[calamity, 2014-06-19 08:21:52]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-19 08:23:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/335343002/100001

[commit-bot: I haz the power, 2014-06-19 16:59:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 16:59:59]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[calamity, 2014-06-20 00:48:17]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-20 00:50:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/335343002/100001

[commit-bot: I haz the power, 2014-06-20 04:29:31]

Change committed as 278588