Fix some gcc 4.4 issues when compiling with toolkit_views.

Fix some gcc 4.4 issues when compiling with toolkit_views.

Most of these squash harmless compiler warnings but the custom_button.cc
change fixes a real problem with the accelerator key bitmask calculation
(+ preceeds << in terms of operator precedence)

The change to tab_strip.cc is ugly but it fixes the following error:

chrome/browser/views/tabs/tab_strip.cc: In member function
‘void TabStrip::StartRemoveTabAnimation(int, TabContents*)’:
chrome/browser/views/tabs/tab_strip.cc:201: error: assuming
signed overflow does not occur when assuming that
(X - c) > X is always false

As an added bonus, this makes the Linux shared build of toolkit_views
work too.

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

[Craig, 2009-09-04 17:23:47]

Any takers to review this please?

Suggestions are welcome for less ugly ways to fix the tab_strip.cc issue!

Thank you.

[tony, 2009-09-04 21:02:36]

http://codereview.chromium.org/199025/diff/1/3
File chrome/browser/views/dialog_stubs_gtk.cc (right):

http://codereview.chromium.org/199025/diff/1/3#newcode49
Line 49: #if !defined(TOOLKIT_VIEWS)
I don't think we compile this if !TOOLKIT_VIEWS.  Maybe I'm misunderstanding why
this is needed?

http://codereview.chromium.org/199025/diff/1/4
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/199025/diff/1/4#newcode202
Line 202: if (adding_tab && start_unselected_width_ < standard_tab_width) {
Bah, can we just add -Wnotype-limits ?

http://codereview.chromium.org/199025/diff/1/5
File chrome/chrome.gyp (right):

http://codereview.chromium.org/199025/diff/1/5#newcode2529
Line 2529: ['exclude', '^browser/extensions/extension_creator.cc'],
Can you comment as to why these are excluded?  If they're in the regular Linux
shared build, do they cause a problem?

[Craig, 2009-09-05 08:13:52]

On 2009/09/04 21:02:36, tony wrote:
> http://codereview.chromium.org/199025/diff/1/3
> File chrome/browser/views/dialog_stubs_gtk.cc (right):
> 
> http://codereview.chromium.org/199025/diff/1/3#newcode49
> Line 49: #if !defined(TOOLKIT_VIEWS)
> I don't think we compile this if !TOOLKIT_VIEWS.  Maybe I'm misunderstanding
why
> this is needed?

Toolkit_views includes dialog_stubs_gtk.cc explicitly (line 2407, chrome.gyp)
and I get a range of linker issues if I drop it. There is another definition for
CreateFindBar in find_bar_win.cc which is also part of the views build which
creates linker errors so I dropped the stub to resolve the issue.

> http://codereview.chromium.org/199025/diff/1/4
> File chrome/browser/views/tabs/tab_strip.cc (right):
> 
> http://codereview.chromium.org/199025/diff/1/4#newcode202
> Line 202: if (adding_tab && start_unselected_width_ < standard_tab_width) {
> Bah, can we just add -Wnotype-limits ?

I've added a gcc pragma now to tab_strip.cc ... I don't want to add a warning
suppression in gyp or it will apply to a much larger range of files and it might
thus hide actual problems introduced in the code. The warning in this case is
strict-overflow. I'm somewhat concerned how older compilers will behave with
this ... is there a toolkit views trybot somewhere that uses an older compiler?

> http://codereview.chromium.org/199025/diff/1/5
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/199025/diff/1/5#newcode2529
> Line 2529: ['exclude', '^browser/extensions/extension_creator.cc'],
> Can you comment as to why these are excluded?  If they're in the regular Linux
> shared build, do they cause a problem?

Both are excluded in the regular shared build too. Extension creator relies on
some RSA stuff that isn't ported so we get linker errors and the pack extension
stuff tries to use extension creator so it has to be excluded too for now.

Thank you!

[tony, 2009-09-05 18:25:39]

LGTM with the below changes applied.

http://codereview.chromium.org/199025/diff/3003/1003
File chrome/browser/views/dialog_stubs_gtk.cc (right):

http://codereview.chromium.org/199025/diff/3003/1003#newcode50
Line 50: FindBar* CreateFindBar(BrowserView* browser_view) {
> Toolkit_views includes dialog_stubs_gtk.cc explicitly (line 2407, chrome.gyp)
> and I get a range of linker issues if I drop it. There is another definition
for
> CreateFindBar in find_bar_win.cc which is also part of the views build which
> creates linker errors so I dropped the stub to resolve the issue.

I think you can just remove the stub since it's in find_bar_win.cc and
find_bar_win.cc is compiled on win and linux.  It's always the case that if this
file is compiled, TOOLKIT_VIEWS=1.

http://codereview.chromium.org/199025/diff/3003/1004
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/199025/diff/3003/1004#newcode39
Line 39: #if defined(OS_LINUX)
Nit: COMPILER_GCC (also in build_config.h)

http://codereview.chromium.org/199025/diff/3003/1005
File chrome/chrome.gyp (right):

http://codereview.chromium.org/199025/diff/3003/1005#newcode2545
Line 2545: ['exclude', '^browser/extensions/extension_creator.cc'],
That's fine, can you actually add the comment here.  The other stuff is all gtk
specific, so it's not obvious just from looking at this.

[Craig, 2009-09-06 16:06:13]

On 2009/09/05 18:25:39, tony wrote:
> LGTM with the below changes applied.

Thank you.
 
> http://codereview.chromium.org/199025/diff/3003/1003
> File chrome/browser/views/dialog_stubs_gtk.cc (right):
> 
> http://codereview.chromium.org/199025/diff/3003/1003#newcode50
> Line 50: FindBar* CreateFindBar(BrowserView* browser_view) {
> > Toolkit_views includes dialog_stubs_gtk.cc explicitly (line 2407,
chrome.gyp)
> > and I get a range of linker issues if I drop it. There is another definition
> for
> > CreateFindBar in find_bar_win.cc which is also part of the views build which
> > creates linker errors so I dropped the stub to resolve the issue.
> 
> I think you can just remove the stub since it's in find_bar_win.cc and
> find_bar_win.cc is compiled on win and linux.  It's always the case that if
this
> file is compiled, TOOLKIT_VIEWS=1.

Ah, yes! ... for some odd reason I kept thinking it was part of the non-views
build but it isn't. Duh. Fixed. Thanks!

> http://codereview.chromium.org/199025/diff/3003/1004
> File chrome/browser/views/tabs/tab_strip.cc (right):
> 
> http://codereview.chromium.org/199025/diff/3003/1004#newcode39
> Line 39: #if defined(OS_LINUX)
> Nit: COMPILER_GCC (also in build_config.h)

Fixed.
 
> http://codereview.chromium.org/199025/diff/3003/1005
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/199025/diff/3003/1005#newcode2545
> Line 2545: ['exclude', '^browser/extensions/extension_creator.cc'],
> That's fine, can you actually add the comment here.  The other stuff is all
gtk
> specific, so it's not obvious just from looking at this.

I've added some comments now.