Use a placeholder logo image/label in SearchViewController.

Use a placeholder logo image/label in SearchViewController.
Note: Logos still appear as red rectangle placeholders.

Use ImageView and Label logos in SearchViewController.
These are scoped_ptrs owned by the client, to avoid accidental leaks.
Add SearchViewController::GetLogoView to get the proper view.

Use the placeholder IDR_GOOGLE_LOGO_LG image as appropriate.
Otherwise, fallback to a label with the short name (or empty for NULL DSE).
Remove unused FixedSizeLayoutManager.

BUG=140955
TEST=Instant Extended API still shows a red rectangle logo placeholder.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152699

[msw, 2012-08-15 21:46:51]

Hey David, please take a look; thanks!

[msw, 2012-08-15 22:04:40]

Hold up, your crrev.com/150849 removed the browser_context_ used here... I'll
let you know when it's ready for review.

[dhollowa, 2012-08-15 22:23:51]

https://chromiumcodereview.appspot.com/10854173/diff/8001/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.cc (right):

https://chromiumcodereview.appspot.com/10854173/diff/8001/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:26: #include
"ui/views/controls/label.h"
nit: sort

https://chromiumcodereview.appspot.com/10854173/diff/8001/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:458: return logo_image_ ?
logo_image_ : static_cast<views::View*>(logo_label_);
nit: I'd prefer if/else to avoid the cast.  Crap compilers...

[msw, 2012-08-15 22:32:53]

Addressed comments, etc. Please take another look, thanks!

https://chromiumcodereview.appspot.com/10854173/diff/8001/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.cc (right):

https://chromiumcodereview.appspot.com/10854173/diff/8001/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:26: #include
"ui/views/controls/label.h"
On 2012/08/15 22:23:51, dhollowa wrote:
> nit: sort

Done.

https://chromiumcodereview.appspot.com/10854173/diff/8001/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:458: return logo_image_ ?
logo_image_ : static_cast<views::View*>(logo_label_);
On 2012/08/15 22:23:51, dhollowa wrote:
> nit: I'd prefer if/else to avoid the cast.  Crap compilers...

Done.

[samarth, 2012-08-15 22:41:04]

https://chromiumcodereview.appspot.com/10854173/diff/7002/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.h (right):

https://chromiumcodereview.appspot.com/10854173/diff/7002/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.h:162: views::Label* logo_label_;
It's probably worth adding a comment here explaining the difference between the
two (unless you want to wait to do so until we actually implement them).

[dhollowa, 2012-08-15 22:48:35]

lgtm

https://chromiumcodereview.appspot.com/10854173/diff/7002/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.h (right):

https://chromiumcodereview.appspot.com/10854173/diff/7002/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.h:43: explicit
SearchViewController(content::BrowserContext* browser_context,
nit: no longer need |explicit|.

[msw, 2012-08-15 23:15:55]

Comments addressed; landing.

https://chromiumcodereview.appspot.com/10854173/diff/7002/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.h (right):

https://chromiumcodereview.appspot.com/10854173/diff/7002/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.h:43: explicit
SearchViewController(content::BrowserContext* browser_context,
On 2012/08/15 22:48:35, dhollowa wrote:
> nit: no longer need |explicit|.

Done.

https://chromiumcodereview.appspot.com/10854173/diff/7002/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.h:162: views::Label* logo_label_;
On 2012/08/15 22:41:04, samarth wrote:
> It's probably worth adding a comment here explaining the difference between
the
> two (unless you want to wait to do so until we actually implement them).

Done.

[commit-bot: I haz the power, 2012-08-15 23:16:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10854173/7004

[commit-bot: I haz the power, 2012-08-15 23:16:19]

Presubmit check for 10854173-7004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

Presubmit checks took 1.1s to calculate.

[msw, 2012-08-15 23:18:54]

+Peter for OWNERS; please take a look, thanks!

[Peter Kasting, 2012-08-16 03:52:41]

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.cc (right):

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:409: const TemplateURL*
default_template_url =
This may be NULL.  You must NULL-check before dereferencing below.

(That is, unless you otherwise guarantee that you never even reach here when
there is no default search provider.  That may be a good idea as it's not clear
to me what you're supposed to do here if this is NULL.  If you do this, DCHECK +
comment here.)

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:415: if
(default_template_url->prepopulate_id() == SEARCH_ENGINE_GOOGLE) {
prepopulate_id() is not a SearchEngineType.  You need to use
TemplateURLPrepopulateData::GetEngineType().

(It is a total coincidence that this happens to work as written.)

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:416: logo_image_ = new
views::ImageView();
Given how you're creating these, it might be better to make the member variables
scoped_ptrs and call set_owned_by_client(true).  This ensures these will be
cleaned up properly regardless of if/how they get added to a view hierarchy. 
This might turn out to be important if for example you end up writing the code
such that if the user changes their DSP, they could somehow end up running
through and creating both a logo and a label.

Really I just get nervous when we create a view on the heap and don't in the
same function guarantee that it's owned by a hierarchy.

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:460: if (logo_image_)
Nit: Shorter:

  return logo_image_ ? logo_image_ : logo_label;

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.h (right):

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.h:162: // The default provider's
name label, used if an image is unavailable.
Nit: Might be clearer to write as:

  // The default provider's logo.  May be NULL.
  views::ImageView* default_provider_logo_;
  // The default provider's name.  Used as a fallback when |logo_image_| is
  // NULL.
  views::Label* default_provider_name_;

[msw, 2012-08-16 08:29:08]

Comments addressed; please take another look, thanks!

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.cc (right):

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:409: const TemplateURL*
default_template_url =
On 2012/08/16 03:52:41, Peter Kasting wrote:
> This may be NULL.  You must NULL-check before dereferencing below.
> 
> (That is, unless you otherwise guarantee that you never even reach here when
> there is no default search provider.  That may be a good idea as it's not
clear
> to me what you're supposed to do here if this is NULL.  If you do this, DCHECK
+
> comment here.)

Done. Checking and having a blank Label with no DSE seems fine for now.

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:415: if
(default_template_url->prepopulate_id() == SEARCH_ENGINE_GOOGLE) {
On 2012/08/16 03:52:41, Peter Kasting wrote:
> prepopulate_id() is not a SearchEngineType.  You need to use
> TemplateURLPrepopulateData::GetEngineType().
> 
> (It is a total coincidence that this happens to work as written.)

Done. I'll fix the basis for this code (in SearchProvider) at:
http://codereview.chromium.org/10832338/

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:416: logo_image_ = new
views::ImageView();
On 2012/08/16 03:52:41, Peter Kasting wrote:
> Given how you're creating these, it might be better to make the member
variables
> scoped_ptrs and call set_owned_by_client(true).  This ensures these will be
> cleaned up properly regardless of if/how they get added to a view hierarchy. 
> This might turn out to be important if for example you end up writing the code
> such that if the user changes their DSP, they could somehow end up running
> through and creating both a logo and a label.
> 
> Really I just get nervous when we create a view on the heap and don't in the
> same function guarantee that it's owned by a hierarchy.

Done.

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.cc:460: if (logo_image_)
On 2012/08/16 03:52:41, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return logo_image_ ? logo_image_ : logo_label;

That'd require a cast (Label/ImageView), which David asked that I avoid.

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
File chrome/browser/ui/views/search_view_controller.h (right):

https://chromiumcodereview.appspot.com/10854173/diff/7004/chrome/browser/ui/v...
chrome/browser/ui/views/search_view_controller.h:162: // The default provider's
name label, used if an image is unavailable.
On 2012/08/16 03:52:41, Peter Kasting wrote:
> Nit: Might be clearer to write as:
> 
>   // The default provider's logo.  May be NULL.
>   views::ImageView* default_provider_logo_;
>   // The default provider's name.  Used as a fallback when |logo_image_| is
>   // NULL.
>   views::Label* default_provider_name_;

Done.

[msw, 2012-08-17 19:44:42]

ping

[msw, 2012-08-20 22:24:05]

Peter: ping; lmk if I should bug someone else.

[msw, 2012-08-21 21:40:47]

+ben for review; please take a look, thanks!

[Ben Goodger (Google), 2012-08-21 21:49:49]

lgtm

[commit-bot: I haz the power, 2012-08-21 22:11:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10854173/4013

[commit-bot: I haz the power, 2012-08-22 00:52:59]

Change committed as 152699