Bug 38158 – Fix gcc compiler warnings in chromium platform graphics code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Fix gcc compiler warnings in chromium platform graphics code

[James Robinson]

Created attachment 54360 [details]
Patch

[James Robinson]

Created attachment 54361 [details]
patch that will pass checkstyle

[James Robinson]

The warnings are from gcc 4.4.1-4ubuntu9, which is what is currently shipping on Karmic, with -Wall -Werror.

[James Robinson]

Comment on attachment 54361 [details]
patch that will pass checkstyle

Clearing flags on attachment: 54361

Committed r58340: <http://trac.webkit.org/changeset/58340>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Thanks for the review!

[James Robinson]

Reverted: http://trac.webkit.org/changeset/58404

[James Robinson]

This was reverted because this snippet:
  if (x < walker.width())
in FontLinux.cpp would evaluate to false if 'x' was less than 'walker.width()' or if 'x' was negative, because walker.width() returned an unsigned value.  C's type promotion rules would promote a negative 'x' to a large positive integer.  I've changed walker.width() to return a signed value and changed this statement to:
  if (x >= 0 && x < walker.width())

[James Robinson]

Created attachment 55074 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55074 [details]
Patch

Again, removing the unused locals, and replacing NULL/0 and fixing parens (all completely uncontrovercial, mindless changes) would be easier to do first.

The signed/unsigned stuff requires some thinking to review correctly. :(

[James Robinson]

Created attachment 55311 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55311 [details]
Patch

 +      if (!((format == GraphicsContext3D::RGBA && type == GraphicsContext3D::UNSIGNED_BYTE) || (format == m_implementationColorReadFormat && type == m_implementationColorReadType))) {
As far as I can tell this is correct.  Certainly reads nicer.  I would probably have even used local variables if I had written this originally. :)  GCC warnings FTW.

WebCore/platform/graphics/chromium/FontLinux.cpp:608
 +          if (x >= 0 && x < walker.width()) {
I'm curious how, if at all, this changes behavior.  Is this testable?

Please add at least a ChangeLog comment explaining why the x >= does not change behavior and does not require testing.  Otherwise this looks good.

[James Robinson]

Committed r58916: <http://trac.webkit.org/changeset/58916>

[James Robinson]

Thanks for the review.  Landed with an explanation of the x >= 0 change in the ChangeLog and a citation of the relevant test.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58916 might have broken Qt Windows 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58912
http://trac.webkit.org/changeset/58913
http://trac.webkit.org/changeset/58914
http://trac.webkit.org/changeset/58915
http://trac.webkit.org/changeset/58916
http://trac.webkit.org/changeset/58917
http://trac.webkit.org/changeset/58918