views: Moves TextfieldController/TextRange into their own headers.

views: Moves TextfieldController/TextRange into their own headers.

BUG=None
TEST=existing unit_tests

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

[tfarina, 2011-03-05 21:22:02]

Hi Scott, could you review this to me?

[sky, 2011-03-07 23:40:11]

http://codereview.chromium.org/6628037/diff/1050/views/controls/textfield/tex...
File views/controls/textfield/text_range.h (right):

http://codereview.chromium.org/6628037/diff/1050/views/controls/textfield/tex...
views/controls/textfield/text_range.h:19: class TextRange {
All methods of this class are cheap. Inline it all.

[tfarina, 2011-03-08 00:02:43]

http://codereview.chromium.org/6628037/diff/1050/views/controls/textfield/tex...
File views/controls/textfield/text_range.h (right):

http://codereview.chromium.org/6628037/diff/1050/views/controls/textfield/tex...
views/controls/textfield/text_range.h:19: class TextRange {
On 2011/03/07 23:40:11, sky wrote:
> All methods of this class are cheap. Inline it all.

Ctors should not be in lined. I'd like to keep the others at the source file. It
makes this API nice and easy to read.

[sky, 2011-03-08 00:11:27]

http://codereview.chromium.org/6628037/diff/1050/views/controls/textfield/tex...
File views/controls/textfield/text_range.h (right):

http://codereview.chromium.org/6628037/diff/1050/views/controls/textfield/tex...
views/controls/textfield/text_range.h:19: class TextRange {
On 2011/03/08 00:02:46, tfarina wrote:
> On 2011/03/07 23:40:11, sky wrote:
> > All methods of this class are cheap. Inline it all.
> 
> Ctors should not be in lined. I'd like to keep the others at the source file.
It
> makes this API nice and easy to read.

See
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo...
.

[tfarina, 2011-03-08 18:46:01]

On 2011/03/08 00:11:27, sky wrote:
> > On 2011/03/07 23:40:11, sky wrote:
> > > All methods of this class are cheap. Inline it all.
> > 
> > Ctors should not be in lined. I'd like to keep the others at the source
file.
> It
> > makes this API nice and easy to read.
> 
> See
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo...
> .

Besides that, is there any optimization advantage? I'm against polluting the
header with the initialization of the data members. If I can avoid, I prefer to
keep the initialization in the source file.

[sky, 2011-03-08 22:05:59]

On Tue, Mar 8, 2011 at 10:46 AM,  <tfarina@chromium.org> wrote:
> On 2011/03/08 00:11:27, sky wrote:
>>
>> > On 2011/03/07 23:40:11, sky wrote:
>> > > All methods of this class are cheap. Inline it all.
>> >
>> > Ctors should not be in lined. I'd like to keep the others at the source
>
> file.
>>
>> It
>> > makes this API nice and easy to read.
>
>> See
>
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo...
>>
>> .
>
> Besides that, is there any optimization advantage? I'm against polluting the
> header with the initialization of the data members. If I can avoid, I prefer
> to
> keep the initialization in the source file.

The rule is intended for uses when you have more than trivial PODs.
TextRange is two PODs and all methods are dirt cheap. It be entirely
in the header.

  -Scott

[tfarina, 2011-03-08 23:36:07]

On 2011/03/08 22:05:59, sky wrote:
> On Tue, Mar 8, 2011 at 10:46 AM,  <mailto:tfarina@chromium.org> wrote:
> > On 2011/03/08 00:11:27, sky wrote:
> >>
> >> > On 2011/03/07 23:40:11, sky wrote:
> >> > > All methods of this class are cheap. Inline it all.
> >> >
> >> > Ctors should not be in lined. I'd like to keep the others at the source
> >
> > file.
> >>
> >> It
> >> > makes this API nice and easy to read.
> >
> >> See
> >
> >
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo...
> >>
> >> .
> >
> > Besides that, is there any optimization advantage? I'm against polluting the
> > header with the initialization of the data members. If I can avoid, I prefer
> > to
> > keep the initialization in the source file.
> 
> The rule is intended for uses when you have more than trivial PODs.
> TextRange is two PODs and all methods are dirt cheap. It be entirely
> in the header.
> 
Fixed. Please, could you take another look?

[sky, 2011-03-09 17:02:33]

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.h (right):

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.h:14: #include
"views/controls/textfield/textfield.h"
Can you nuke the textfield include now?

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/opt...
File chrome/browser/ui/views/options/cookies_view.cc (right):

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/opt...
chrome/browser/ui/views/options/cookies_view.cc:27: #include
"views/controls/textfield/textfield.h"
nuke one of these

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/opt...
chrome/browser/ui/views/options/cookies_view.cc:161: int
CookiesView::GetDialogButtons() const {
Did you intend to add these methods?

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/url...
File chrome/browser/ui/views/url_picker.h (left):

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/url...
chrome/browser/ui/views/url_picker.h:48: UrlPicker(UrlPickerDelegate* delegate,
It's find to leave everything on the same line when it fits.

http://codereview.chromium.org/6628037/diff/10063/views/controls/textfield/te...
File views/controls/textfield/text_range.h (right):

http://codereview.chromium.org/6628037/diff/10063/views/controls/textfield/te...
views/controls/textfield/text_range.h:28: : start_(range.start_),
end_(range.end_) {}
Once you wrap, each variable should be on its own line.

http://codereview.chromium.org/6628037/diff/10063/views/controls/textfield/te...
views/controls/textfield/text_range.h:64: void operator=(const TextRange&);
I can't think of a good reason why we should disallow this.

[tfarina, 2011-03-09 17:31:05]

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.h (right):

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.h:14: #include
"views/controls/textfield/textfield.h"
On 2011/03/09 17:02:33, sky wrote:
> Can you nuke the textfield include now?

No, we inherit from views::Textfield at line 114.

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/opt...
File chrome/browser/ui/views/options/cookies_view.cc (right):

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/opt...
chrome/browser/ui/views/options/cookies_view.cc:27: #include
"views/controls/textfield/textfield.h"
On 2011/03/09 17:02:33, sky wrote:
> nuke one of these

Done.

http://codereview.chromium.org/6628037/diff/10063/chrome/browser/ui/views/opt...
chrome/browser/ui/views/options/cookies_view.cc:161: int
CookiesView::GetDialogButtons() const {
On 2011/03/09 17:02:33, sky wrote:
> Did you intend to add these methods?

Yes.

http://codereview.chromium.org/6628037/diff/10063/views/controls/textfield/te...
File views/controls/textfield/text_range.h (right):

http://codereview.chromium.org/6628037/diff/10063/views/controls/textfield/te...
views/controls/textfield/text_range.h:28: : start_(range.start_),
end_(range.end_) {}
On 2011/03/09 17:02:33, sky wrote:
> Once you wrap, each variable should be on its own line.

Done.

http://codereview.chromium.org/6628037/diff/10063/views/controls/textfield/te...
views/controls/textfield/text_range.h:64: void operator=(const TextRange&);
On 2011/03/09 17:02:33, sky wrote:
> I can't think of a good reason why we should disallow this.

Removed.

[sky, 2011-03-09 23:28:02]

LGTGM