Remove wstring from TableModel....

Remove wstring from TableModel.

BUG=23581
TEST=no visible changes; all tests pass


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

[viettrungluu, 2010-12-29 23:41:17]

Nice. LGTM with just a few nits.

http://codereview.chromium.org/6044007/diff/1/app/table_model.h
File app/table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/app/table_model.h#newcode9
app/table_model.h:9: #include <string>
No longer needed?

http://codereview.chromium.org/6044007/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.h:9: #include <string>
No longer needed, but should probably include base/string16.h.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/dom_ui/options/a...
File chrome/browser/dom_ui/options/add_startup_page_handler.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/dom_ui/options/a...
chrome/browser/dom_ui/options/add_startup_page_handler.cc:79:
dict->SetString("displayURL", url_table_model_->GetText(
Micro-nit: I think this would look nicer if you broke the line after
|"displayURL",| and lined things up on the following line?

http://codereview.chromium.org/6044007/diff/1/chrome/browser/notifications/no...
File chrome/browser/notifications/notification_exceptions_table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/notifications/no...
chrome/browser/notifications/notification_exceptions_table_model.h:9: #include
<string>
I don't think it's needed.

(I don't think you need to include string16.h either. In case you are (or anyone
else is) wondering, my rule of thumb is to not include string16.h (for example)
if string16 is only used in a way which implies that string16.h must already be
included by one of our includes. In this case, we have only an override of a
method returning string16, so the superclass's header must have included
string16.h.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url_table_model.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url_table_model.cc:7: #include <string>
These two #includes are presumably unneeded, since the class definition already
required that they be included.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url_table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url_table_model.h:60: const std::string&
url);
Probably <string> should be included as a drive-by fix.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url_table_model.h:101:
std::vector<ModelEntry*> entries_;
And <vector> too, I guess.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/options...
File chrome/browser/ui/views/options/passwords_page_view.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/options...
chrome/browser/ui/views/options/passwords_page_view.cc:80: WideToUTF16(url));
Should this be WideToUTF16Hack?

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/task_ma...
File chrome/browser/ui/views/task_manager_view.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/task_ma...
chrome/browser/ui/views/task_manager_view.cc:60: int RowCount();
Are these OVERRIDEs?

http://codereview.chromium.org/6044007/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_helper_unittest.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_helper_unittest.cc:11:
TEST(ContentSettingsHelperTest, OriginToWString) {
Rename this test?

I also wonder why there isn't a test for OriginToString() (which could be the
same, minus the ASCIIToUTF16()s).

[Avi (use Gerrit), 2010-12-30 00:04:14]

http://codereview.chromium.org/6044007/diff/1/app/table_model.h
File app/table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/app/table_model.h#newcode9
app/table_model.h:9: #include <string>
On 2010/12/29 23:41:17, viettrungluu wrote:
> No longer needed?

Done.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.h:9: #include <string>
On 2010/12/29 23:41:17, viettrungluu wrote:
> No longer needed, but should probably include base/string16.h.

No, overrides TableModel which includes.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/dom_ui/options/a...
File chrome/browser/dom_ui/options/add_startup_page_handler.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/dom_ui/options/a...
chrome/browser/dom_ui/options/add_startup_page_handler.cc:79:
dict->SetString("displayURL", url_table_model_->GetText(
On 2010/12/29 23:41:17, viettrungluu wrote:
> Micro-nit: I think this would look nicer if you broke the line after
> |"displayURL",| and lined things up on the following line?

Done.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/notifications/no...
File chrome/browser/notifications/notification_exceptions_table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/notifications/no...
chrome/browser/notifications/notification_exceptions_table_model.h:9: #include
<string>
On 2010/12/29 23:41:17, viettrungluu wrote:
> I don't think it's needed.
> 
> (I don't think you need to include string16.h either. In case you are (or
anyone
> else is) wondering, my rule of thumb is to not include string16.h (for
example)
> if string16 is only used in a way which implies that string16.h must already
be
> included by one of our includes. In this case, we have only an override of a
> method returning string16, so the superclass's header must have included
> string16.h.

Done.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url_table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url_table_model.h:60: const std::string&
url);
On 2010/12/29 23:41:17, viettrungluu wrote:
> Probably <string> should be included as a drive-by fix.

Done.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url_table_model.h:101:
std::vector<ModelEntry*> entries_;
On 2010/12/29 23:41:17, viettrungluu wrote:
> And <vector> too, I guess.

Done.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/options...
File chrome/browser/ui/views/options/passwords_page_view.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/options...
chrome/browser/ui/views/options/passwords_page_view.cc:80: WideToUTF16(url));
On 2010/12/29 23:41:17, viettrungluu wrote:
> Should this be WideToUTF16Hack?

Sure. In general the distinction between Hack and not hack is gray.

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/task_ma...
File chrome/browser/ui/views/task_manager_view.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/ui/views/task_ma...
chrome/browser/ui/views/task_manager_view.cc:60: int RowCount();
On 2010/12/29 23:41:17, viettrungluu wrote:
> Are these OVERRIDEs?

Done.

http://codereview.chromium.org/6044007/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_helper_unittest.cc (right):

http://codereview.chromium.org/6044007/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_helper_unittest.cc:11:
TEST(ContentSettingsHelperTest, OriginToWString) {
On 2010/12/29 23:41:17, viettrungluu wrote:
> Rename this test?
> 
> I also wonder why there isn't a test for OriginToString() (which could be the
> same, minus the ASCIIToUTF16()s).

Done. Re test possibly because the 16 version is implemented in terms of the 8
so the 8 gets tested. Yeah, I know, black box...

[viettrungluu, 2010-12-30 00:35:13]

http://codereview.chromium.org/6044007/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.h (right):

http://codereview.chromium.org/6044007/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.h:9: #include <string>
On 2010/12/30 00:04:15, Avi wrote:
> On 2010/12/29 23:41:17, viettrungluu wrote:
> > No longer needed, but should probably include base/string16.h.
> 
> No, overrides TableModel which includes.

I would include it because of the standalone use (for FormattedURL()), but our
rules about inclusions are vague so it's up to you.