FBTF: Remove unneeded headers from base/ (part 7)

FBTF: Remove unneeded headers from base/ (part 7)

BUG=none
TEST=none

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

[Lei Zhang, 2010-08-20 16:18:02]

hopefully patchset 3 builds.

[viettrungluu, 2010-08-20 16:46:42]

I don't think we should move implementations of simple accessors/mutators to the
.cc just because of a DCHECK(); it makes things less clear and harder to
maintain.

http://codereview.chromium.org/3176026/diff/64004/65021
File chrome/browser/autocomplete/autocomplete.h (left):

http://codereview.chromium.org/3176026/diff/64004/65021#oldcode652
chrome/browser/autocomplete/autocomplete.h:652: const AutocompleteMatch&
match_at(size_t index) const {
I kind of don't like this not being inlined, since everything else is. Having
the DCHECK() in the header is kind of annoying though (and thus having to
include logging.h), I guess, but I think we can swallow that cost.

http://codereview.chromium.org/3176026/diff/64004/65034
File chrome/browser/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/3176026/diff/64004/65034#newcode104
chrome/browser/cocoa/constrained_window_mac.mm:104: void
ConstrainedWindowMacDelegateCustomSheet::init(NSWindow* sheet,
Could you insert this in the same order as it is in the header?

http://codereview.chromium.org/3176026/diff/64004/65071
File chrome/browser/renderer_host/render_sandbox_host_linux.h (left):

http://codereview.chromium.org/3176026/diff/64004/65071#oldcode22
chrome/browser/renderer_host/render_sandbox_host_linux.h:22: int
GetRendererSocket() const {
The same comments I made previously apply here.

http://codereview.chromium.org/3176026/diff/64004/65084
File chrome/browser/tab_contents/render_view_host_manager.h (left):

http://codereview.chromium.org/3176026/diff/64004/65084#oldcode149
chrome/browser/tab_contents/render_view_host_manager.h:149: void
set_interstitial_page(InterstitialPage* interstitial_page) {
Ditto.

http://codereview.chromium.org/3176026/diff/64004/65099
File chrome/default_plugin/plugin_install_job_monitor.h (left):

http://codereview.chromium.org/3176026/diff/64004/65099#oldcode36
chrome/default_plugin/plugin_install_job_monitor.h:36:
DCHECK(::IsWindow(plugin_window));
"

http://codereview.chromium.org/3176026/diff/64004/65118
File chrome_frame/np_event_listener.h (left):

http://codereview.chromium.org/3176026/diff/64004/65118#oldcode64
chrome_frame/np_event_listener.h:64: ref_count_--;
Errrr, this is a template.

http://codereview.chromium.org/3176026/diff/64004/65128
File gfx/window_impl.h (left):

http://codereview.chromium.org/3176026/diff/64004/65128#oldcode68
gfx/window_impl.h:68: // We dynamically generate the class name, so don't
register it globally!
"

http://codereview.chromium.org/3176026/diff/64004/65130
File gpu/command_buffer/service/buffer_manager.h (left):

http://codereview.chromium.org/3176026/diff/64004/65130#oldcode101
gpu/command_buffer/service/buffer_manager.h:101: DCHECK_EQ(target_, 0u);  // you
can only set this once.
"

http://codereview.chromium.org/3176026/diff/64004/65134
File gpu/command_buffer/service/texture_manager.cc (right):

http://codereview.chromium.org/3176026/diff/64004/65134#newcode483
gpu/command_buffer/service/texture_manager.cc:483: void
TextureManager::TextureInfo::SetTarget(GLenum target, GLint max_levels) {
Please don't just append to the end.

http://codereview.chromium.org/3176026/diff/64004/65162
File net/url_request/url_request.h (left):

http://codereview.chromium.org/3176026/diff/64004/65162#oldcode546
net/url_request/url_request.h:546: void set_priority(net::RequestPriority
priority) {
"

http://codereview.chromium.org/3176026/diff/64004/65164
File printing/image.h (left):

http://codereview.chromium.org/3176026/diff/64004/65164#oldcode58
printing/image.h:58: DCHECK(x >= 0 && x < size_.width());
I wonder what this is used for; its inlining may be performance-important.

http://codereview.chromium.org/3176026/diff/64004/65180
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/3176026/diff/64004/65180#newcode763
views/controls/tree/tree_view.cc:763: NodeDetails*
TreeView::GetNodeDetails(TreeModelNode* node) {
Please don't just append stuff to the end.

http://codereview.chromium.org/3176026/diff/64004/65189
File webkit/tools/test_shell/test_shell_devtools_callargs.h (left):

http://codereview.chromium.org/3176026/diff/64004/65189#oldcode25
webkit/tools/test_shell/test_shell_devtools_callargs.h:25: --calls_count_;
" (since the corresponding ++ ones are in the header)

[Elliot Glaysher, 2010-08-20 17:14:55]

I disagree. I think we should move accessors that require logging out
of the interface file.

On Fri, Aug 20, 2010 at 9:46 AM,  <viettrungluu@chromium.org> wrote:
> http://codereview.chromium.org/3176026/show

[viettrungluu, 2010-08-20 17:21:14]

They don't *require* logging. The alternative should be to consider deleting the
DCHECK(), not moving it out.

On 2010/08/20 17:14:55, Elliot Glaysher wrote:
> I disagree. I think we should move accessors that require logging out
> of the interface file.
> 
> On Fri, Aug 20, 2010 at 9:46 AM,  <mailto:viettrungluu@chromium.org> wrote:
> > http://codereview.chromium.org/3176026/show
>

[Lei Zhang, 2010-08-20 17:24:28]

I don't think we should be deleting DCHECKs.

Once you add a DCHECK, it stops being a trivial accessor. Yes, it's one line,
but it does a lot more work.

On 2010/08/20 17:21:14, viettrungluu wrote:
> They don't *require* logging. The alternative should be to consider deleting
the
> DCHECK(), not moving it out.
> 
> On 2010/08/20 17:14:55, Elliot Glaysher wrote:
> > I disagree. I think we should move accessors that require logging out
> > of the interface file.
> > 
> > On Fri, Aug 20, 2010 at 9:46 AM,  <mailto:viettrungluu@chromium.org> wrote:
> > > http://codereview.chromium.org/3176026/show
> >

[viettrungluu, 2010-08-20 17:29:37]

There's a high maintenance cost to having code in two places, and it's really
ugly to do so.

Moreover, you're penalizing the release build.

On 2010/08/20 17:24:28, Lei Zhang wrote:
> I don't think we should be deleting DCHECKs.
> 
> Once you add a DCHECK, it stops being a trivial accessor. Yes, it's one line,
> but it does a lot more work.
> 
> On 2010/08/20 17:21:14, viettrungluu wrote:
> > They don't *require* logging. The alternative should be to consider deleting
> the
> > DCHECK(), not moving it out.
> > 
> > On 2010/08/20 17:14:55, Elliot Glaysher wrote:
> > > I disagree. I think we should move accessors that require logging out
> > > of the interface file.
> > > 
> > > On Fri, Aug 20, 2010 at 9:46 AM,  <mailto:viettrungluu@chromium.org>
wrote:
> > > > http://codereview.chromium.org/3176026/show
> > >

[rvargas (doing something else), 2010-08-20 19:18:41]

For as long as I remember we have required that code that DCHECKS go to the cc
file and not the header (to keep logging out of headers), although they keep
creeping in.

On 2010/08/20 17:29:37, viettrungluu wrote:
> There's a high maintenance cost to having code in two places, and it's really
> ugly to do so.
> 
> Moreover, you're penalizing the release build.
> 
> On 2010/08/20 17:24:28, Lei Zhang wrote:
> > I don't think we should be deleting DCHECKs.
> > 
> > Once you add a DCHECK, it stops being a trivial accessor. Yes, it's one
line,
> > but it does a lot more work.
> > 
> > On 2010/08/20 17:21:14, viettrungluu wrote:
> > > They don't *require* logging. The alternative should be to consider
deleting
> > the
> > > DCHECK(), not moving it out.
> > > 
> > > On 2010/08/20 17:14:55, Elliot Glaysher wrote:
> > > > I disagree. I think we should move accessors that require logging out
> > > > of the interface file.
> > > > 
> > > > On Fri, Aug 20, 2010 at 9:46 AM,  <mailto:viettrungluu@chromium.org>
> wrote:
> > > > > http://codereview.chromium.org/3176026/show
> > > >

[viettrungluu, 2010-08-20 20:09:18]

Sure, but instead of just moving (and uglifying code, we need to compare the
cost of:
- the #include "base/logging.h"
- not having the DCHECK() (e.g., if it'll crash anyway, does the DCHECK() buy
you anything? what are the chances of the DCHECK() ever actually helping us?)
- code with an ugly split across multiple locations (with a slight performance
penalty -- I'm worried about performance so much 99% of the time, but I think we
should seriously consider anything which has us making code uglier)

On 2010/08/20 19:18:41, rvargas wrote:
> For as long as I remember we have required that code that DCHECKS go to the cc
> file and not the header (to keep logging out of headers), although they keep
> creeping in.
> 
> On 2010/08/20 17:29:37, viettrungluu wrote:
> > There's a high maintenance cost to having code in two places, and it's
really
> > ugly to do so.
> > 
> > Moreover, you're penalizing the release build.
> > 
> > On 2010/08/20 17:24:28, Lei Zhang wrote:
> > > I don't think we should be deleting DCHECKs.
> > > 
> > > Once you add a DCHECK, it stops being a trivial accessor. Yes, it's one
> line,
> > > but it does a lot more work.
> > > 
> > > On 2010/08/20 17:21:14, viettrungluu wrote:
> > > > They don't *require* logging. The alternative should be to consider
> deleting
> > > the
> > > > DCHECK(), not moving it out.
> > > > 
> > > > On 2010/08/20 17:14:55, Elliot Glaysher wrote:
> > > > > I disagree. I think we should move accessors that require logging out
> > > > > of the interface file.
> > > > > 
> > > > > On Fri, Aug 20, 2010 at 9:46 AM,  <mailto:viettrungluu@chromium.org>
> > wrote:
> > > > > > http://codereview.chromium.org/3176026/show
> > > > >

[Lei Zhang, 2010-08-20 20:36:05]

It feels like this conversation should go to chromium-dev rather than here. I'll
revert the changes to the accessors in this CL to maintain the status quo.

On 2010/08/20 20:09:18, viettrungluu wrote:
> Sure, but instead of just moving (and uglifying code, we need to compare the
> cost of:
> - the #include "base/logging.h"
> - not having the DCHECK() (e.g., if it'll crash anyway, does the DCHECK() buy
> you anything? what are the chances of the DCHECK() ever actually helping us?)
> - code with an ugly split across multiple locations (with a slight performance
> penalty -- I'm worried about performance so much 99% of the time, but I think
we
> should seriously consider anything which has us making code uglier)
> 
> On 2010/08/20 19:18:41, rvargas wrote:
> > For as long as I remember we have required that code that DCHECKS go to the
cc
> > file and not the header (to keep logging out of headers), although they keep
> > creeping in.
> > 
> > On 2010/08/20 17:29:37, viettrungluu wrote:
> > > There's a high maintenance cost to having code in two places, and it's
> really
> > > ugly to do so.
> > > 
> > > Moreover, you're penalizing the release build.
> > > 
> > > On 2010/08/20 17:24:28, Lei Zhang wrote:
> > > > I don't think we should be deleting DCHECKs.
> > > > 
> > > > Once you add a DCHECK, it stops being a trivial accessor. Yes, it's one
> > line,
> > > > but it does a lot more work.
> > > > 
> > > > On 2010/08/20 17:21:14, viettrungluu wrote:
> > > > > They don't *require* logging. The alternative should be to consider
> > deleting
> > > > the
> > > > > DCHECK(), not moving it out.
> > > > > 
> > > > > On 2010/08/20 17:14:55, Elliot Glaysher wrote:
> > > > > > I disagree. I think we should move accessors that require logging
out
> > > > > > of the interface file.
> > > > > > 
> > > > > > On Fri, Aug 20, 2010 at 9:46 AM,  <mailto:viettrungluu@chromium.org>
> > > wrote:
> > > > > > > http://codereview.chromium.org/3176026/show
> > > > > >

[Lei Zhang, 2010-08-25 20:19:14]

I still feel moving accessors out of headers is the right thing to do. I've put
them in the right order in the .cc files.

http://codereview.chromium.org/3176026/diff/64004/65118
File chrome_frame/np_event_listener.h (left):

http://codereview.chromium.org/3176026/diff/64004/65118#oldcode64
chrome_frame/np_event_listener.h:64: ref_count_--;
On 2010/08/20 16:46:43, viettrungluu wrote:
> Errrr, this is a template.

Whoops, reverted.

http://codereview.chromium.org/3176026/diff/64004/65134
File gpu/command_buffer/service/texture_manager.cc (right):

http://codereview.chromium.org/3176026/diff/64004/65134#newcode483
gpu/command_buffer/service/texture_manager.cc:483: void
TextureManager::TextureInfo::SetTarget(GLenum target, GLint max_levels) {
On 2010/08/20 16:46:43, viettrungluu wrote:
> Please don't just append to the end.

Done.

http://codereview.chromium.org/3176026/diff/64004/65180
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/3176026/diff/64004/65180#newcode763
views/controls/tree/tree_view.cc:763: NodeDetails*
TreeView::GetNodeDetails(TreeModelNode* node) {
On 2010/08/20 16:46:43, viettrungluu wrote:
> Please don't just append stuff to the end.

Done.

http://codereview.chromium.org/3176026/diff/64004/65189
File webkit/tools/test_shell/test_shell_devtools_callargs.h (left):

http://codereview.chromium.org/3176026/diff/64004/65189#oldcode25
webkit/tools/test_shell/test_shell_devtools_callargs.h:25: --calls_count_;
On 2010/08/20 16:46:43, viettrungluu wrote:
> " (since the corresponding ++ ones are in the header)

Moved the constructor here too.

[viettrungluu, 2010-08-25 23:12:21]

You may be right. Just a couple of (sadly, probably annoying) requests:
- if you move an accessor out of the header, consider moving other trivial
methods out too;
- also consider deleting the DCHECK() ;)
- and, most annoyingly, could you split up the change? right now it's rather
hard (well, tedious) to find the places where you did semi-nontrivial stuff

(You can have an immediate rubberstamp LGTM on the trivial stuff....)

On 2010/08/25 20:19:14, Lei Zhang wrote:
> I still feel moving accessors out of headers is the right thing to do. I've
put
> them in the right order in the .cc files.
> 
> http://codereview.chromium.org/3176026/diff/64004/65118
> File chrome_frame/np_event_listener.h (left):
> 
> http://codereview.chromium.org/3176026/diff/64004/65118#oldcode64
> chrome_frame/np_event_listener.h:64: ref_count_--;
> On 2010/08/20 16:46:43, viettrungluu wrote:
> > Errrr, this is a template.
> 
> Whoops, reverted.
> 
> http://codereview.chromium.org/3176026/diff/64004/65134
> File gpu/command_buffer/service/texture_manager.cc (right):
> 
> http://codereview.chromium.org/3176026/diff/64004/65134#newcode483
> gpu/command_buffer/service/texture_manager.cc:483: void
> TextureManager::TextureInfo::SetTarget(GLenum target, GLint max_levels) {
> On 2010/08/20 16:46:43, viettrungluu wrote:
> > Please don't just append to the end.
> 
> Done.
> 
> http://codereview.chromium.org/3176026/diff/64004/65180
> File views/controls/tree/tree_view.cc (right):
> 
> http://codereview.chromium.org/3176026/diff/64004/65180#newcode763
> views/controls/tree/tree_view.cc:763: NodeDetails*
> TreeView::GetNodeDetails(TreeModelNode* node) {
> On 2010/08/20 16:46:43, viettrungluu wrote:
> > Please don't just append stuff to the end.
> 
> Done.
> 
> http://codereview.chromium.org/3176026/diff/64004/65189
> File webkit/tools/test_shell/test_shell_devtools_callargs.h (left):
> 
> http://codereview.chromium.org/3176026/diff/64004/65189#oldcode25
> webkit/tools/test_shell/test_shell_devtools_callargs.h:25: --calls_count_;
> On 2010/08/20 16:46:43, viettrungluu wrote:
> > " (since the corresponding ++ ones are in the header)
> 
> Moved the constructor here too.

[brettw, 2010-08-26 00:11:11]

We've had a pretty strict rule that non-inline functions should not get
variable_style_naming(). I think it's best to preserve that.

[Lei Zhang, 2010-08-26 22:00:18]

On 2010/08/26 00:11:11, brettw wrote:
> We've had a pretty strict rule that non-inline functions should not get
> variable_style_naming(). I think it's best to preserve that.

Thanks. I'll keep that in mind. I've split the CL in two. This half is mostly
rubberstampable.

[viettrungluu, 2010-08-26 23:36:15]

LGTM, maybe also moving the copy constructor.

(Argh. I must remember to stick around after I click the "Publish All My Drafts"
button, lest it get swallowed by a rietveld error.)

http://codereview.chromium.org/3176026/diff/52013/42032
File chrome_frame/crash_reporting/veh_test.h (right):

http://codereview.chromium.org/3176026/diff/52013/42032#newcode80
chrome_frame/crash_reporting/veh_test.h:80: #endif  //
CHROME_FRAME_CRASH_REPORTING_VEH_TEST_H_
Lint claims that you're missing a newline here (and in a few other files). Is
this true, or is it the same old bug? (I think the bug occurs when last line
ends with a '//' comment.)

/me should fix lint.

http://codereview.chromium.org/3176026/diff/52013/42096
File webkit/tools/test_shell/test_shell_devtools_callargs.h (right):

http://codereview.chromium.org/3176026/diff/52013/42096#newcode16
webkit/tools/test_shell/test_shell_devtools_callargs.h:16:
TestShellDevToolsCallArgs(const TestShellDevToolsCallArgs& args)
Should you also move this one out?

[Lei Zhang, 2010-08-27 21:34:29]

http://codereview.chromium.org/3176026/diff/52013/42032
File chrome_frame/crash_reporting/veh_test.h (right):

http://codereview.chromium.org/3176026/diff/52013/42032#newcode80
chrome_frame/crash_reporting/veh_test.h:80: #endif  //
CHROME_FRAME_CRASH_REPORTING_VEH_TEST_H_
On 2010/08/26 23:36:16, viettrungluu wrote:
> Lint claims that you're missing a newline here (and in a few other files). Is
> this true, or is it the same old bug? (I think the bug occurs when last line
> ends with a '//' comment.)
> 
> /me should fix lint.

same old bug I imagine.

http://codereview.chromium.org/3176026/diff/52013/42096
File webkit/tools/test_shell/test_shell_devtools_callargs.h (right):

http://codereview.chromium.org/3176026/diff/52013/42096#newcode16
webkit/tools/test_shell/test_shell_devtools_callargs.h:16:
TestShellDevToolsCallArgs(const TestShellDevToolsCallArgs& args)
On 2010/08/26 23:36:16, viettrungluu wrote:
> Should you also move this one out?

Done.