Adds a FakeToolbarModel for use in testing.

Adds a FakeToolbarModel for use in testing.

The ToolbarModel's dependencies make it hard to include in unit tests. This simple fake is created with some default values which can be changed via the setters.

BUG=None


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

[sky, 2012-10-05 16:41:22]

If we're going to do this ToolbarModel should be pure virtual otherwise folks
are going to add random methods to ToolbarModel over time.

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
File chrome/browser/ui/toolbar/fake_toolbar_model.cc (right):

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:9:
FakeToolbarModel::FakeToolbarModel() : ToolbarModel(NULL),
ToolbarModel(NULL) should be on the next line.

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:10:
text_(ASCIIToUTF16("www.chromium.org")),
I think you should leave all the strings/urls empty.

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
File chrome/browser/ui/toolbar/fake_toolbar_model.h (right):

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
chrome/browser/ui/toolbar/fake_toolbar_model.h:19: void SetText(string16 text);
const string16&

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
chrome/browser/ui/toolbar/fake_toolbar_model.h:22: void SetURL(GURL url);
const GURL&

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
chrome/browser/ui/toolbar/fake_toolbar_model.h:34: void SetEVCertName(string16
ev_cert_name);
const string16&

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
chrome/browser/ui/toolbar/fake_toolbar_model.h:38: virtual bool
ShouldDisplayURL() const;
Keep all the virtual methods grouped together and use OVERRIDE.
For the setters you can inline them, eg set_url(const GURL& url)

https://codereview.chromium.org/11040055/diff/1/chrome/browser/ui/toolbar/fak...
chrome/browser/ui/toolbar/fake_toolbar_model.h:48: };
DISALLOW_...

[lliabraa, 2012-10-09 19:29:35]

I've moved the implementation to an Impl class and replaced all instantiations
of ToolbarModel with ToolbarModelImpl.

PTAL

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/fake_toolbar_model.cc (right):

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:9:
FakeToolbarModel::FakeToolbarModel() : ToolbarModel(NULL),
On 2012/10/05 16:41:22, sky wrote:
> ToolbarModel(NULL) should be on the next line.

Done.

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:10:
text_(ASCIIToUTF16("www.chromium.org")),
On 2012/10/05 16:41:22, sky wrote:
> I think you should leave all the strings/urls empty.

Done.

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/fake_toolbar_model.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/fake_toolbar_model.h:19: void SetText(string16 text);
On 2012/10/05 16:41:22, sky wrote:
> const string16&

Done.

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/fake_toolbar_model.h:22: void SetURL(GURL url);
On 2012/10/05 16:41:22, sky wrote:
> const GURL&

Done.

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/fake_toolbar_model.h:34: void SetEVCertName(string16
ev_cert_name);
On 2012/10/05 16:41:22, sky wrote:
> const string16&

Done.

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/fake_toolbar_model.h:38: virtual bool
ShouldDisplayURL() const;
On 2012/10/05 16:41:22, sky wrote:
> Keep all the virtual methods grouped together and use OVERRIDE.
> For the setters you can inline them, eg set_url(const GURL& url)

Done.

https://chromiumcodereview.appspot.com/11040055/diff/1/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/fake_toolbar_model.h:48: };
On 2012/10/05 16:41:22, sky wrote:
> DISALLOW_...

Done.

[sky, 2012-10-09 20:07:13]

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/fake_toolbar_model.cc (right):

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:7: #include
"base/utf_string_conversions.h"
Do you really need this?

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:11: text_(),
Remove text_, url_ and ev_cert_name_.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:17: should_display_url_(true) {}
You missed input_in_progress_

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/fake_toolbar_model.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:15: class FakeToolbarModel :
public ToolbarModel {
Name this TestToolbarModel.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:18: virtual ~FakeToolbarModel()
OVERRIDE;
no OVERRIDE here

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:30: void SetText(const string16&
text) { text_ = text; }
If you're going to inline all these (which is fine) name the in
unix_hacker_style, eg set_text and set_url.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:33: should_replace_url_ =
should_replace_url; }
When you wrap like this, wrap } to the next line.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/toolbar_model.h:72: virtual void
set_input_in_progress(bool value) = 0;
These need to be renamed now to SetInputInProgress and GetInputInProgress

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/toolbar_model.h:82:
DISALLOW_COPY_AND_ASSIGN(ToolbarModel);
You shouldn't need this.

[lliabraa, 2012-10-10 19:39:33]

Thanks for the review - this is ready for another look.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/fake_toolbar_model.cc (right):

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:7: #include
"base/utf_string_conversions.h"
On 2012/10/09 20:07:13, sky wrote:
> Do you really need this?

Nope...removed.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:11: text_(),
On 2012/10/09 20:07:13, sky wrote:
> Remove text_, url_ and ev_cert_name_.

Done.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.cc:17: should_display_url_(true) {}
On 2012/10/09 20:07:13, sky wrote:
> You missed input_in_progress_

Done.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/fake_toolbar_model.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:15: class FakeToolbarModel :
public ToolbarModel {
On 2012/10/09 20:07:13, sky wrote:
> Name this TestToolbarModel.

Done.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:18: virtual ~FakeToolbarModel()
OVERRIDE;
On 2012/10/09 20:07:13, sky wrote:
> no OVERRIDE here

Done.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:30: void SetText(const string16&
text) { text_ = text; }
On 2012/10/09 20:07:13, sky wrote:
> If you're going to inline all these (which is fine) name the in
> unix_hacker_style, eg set_text and set_url.

Done.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/fake_toolbar_model.h:33: should_replace_url_ =
should_replace_url; }
On 2012/10/09 20:07:13, sky wrote:
> When you wrap like this, wrap } to the next line.

Done.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/toolbar_model.h:72: virtual void
set_input_in_progress(bool value) = 0;
On 2012/10/09 20:07:13, sky wrote:
> These need to be renamed now to SetInputInProgress and GetInputInProgress

Done.

https://chromiumcodereview.appspot.com/11040055/diff/8003/chrome/browser/ui/t...
chrome/browser/ui/toolbar/toolbar_model.h:82:
DISALLOW_COPY_AND_ASSIGN(ToolbarModel);
On 2012/10/09 20:07:13, sky wrote:
> You shouldn't need this.

Removed.

[sky, 2012-10-10 21:17:36]

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model.h:76: static string16
GetEVCertName(const net::X509Certificate& cert);
Is there a reason to leave this here?

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model_impl.h:35: // Returns the text for the
current page's URL. This will have been formatted
Don't duplicate all the comments here. Just remove all the comments and prefix
the section with:
// Overriden from ToolbarModel.

[lliabraa, 2012-10-11 13:03:00]

qq

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model.h:76: static string16
GetEVCertName(const net::X509Certificate& cert);
On 2012/10/10 21:17:36, sky wrote:
> Is there a reason to leave this here?

I didn't realize this was only used by ToolbarModelImpl; I've moved it to
toolbar_model_impl.cc.

Can I inline the ~ToolbarModel() {} and get rid of toolbar_model.cc?

[sky, 2012-10-11 17:18:24]

On Thu, Oct 11, 2012 at 6:03 AM,  <lliabraa@chromium.org> wrote:
> qq
>
>
>
>
https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
> File chrome/browser/ui/toolbar/toolbar_model.h (right):
>
>
https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
> chrome/browser/ui/toolbar/toolbar_model.h:76: static string16
> GetEVCertName(const net::X509Certificate& cert);
> On 2012/10/10 21:17:36, sky wrote:
>>
>> Is there a reason to leave this here?
>
>
> I didn't realize this was only used by ToolbarModelImpl; I've moved it
> to toolbar_model_impl.cc.
>
> Can I inline the ~ToolbarModel() {} and get rid of toolbar_model.cc?

Yes please.

[lliabraa, 2012-10-11 18:14:59]

PTAL

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://chromiumcodereview.appspot.com/11040055/diff/21002/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model_impl.h:35: // Returns the text for the
current page's URL. This will have been formatted
On 2012/10/10 21:17:36, sky wrote:
> Don't duplicate all the comments here. Just remove all the comments and prefix
> the section with:
> // Overriden from ToolbarModel.

Done.

[sky, 2012-10-11 19:20:44]

LGTM

[commit-bot: I haz the power, 2012-10-11 19:29:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lliabraa@chromium.org/11040055/19006

[commit-bot: I haz the power, 2012-10-11 21:32:43]

Change committed as 161407