Chromium has special restructions for WebKit use. The current WebCore::GraphicsContext::drawLineForMisspellingOrBadGrammar for Mac does not work. In this code there are a few comments line ""FIXME: More of this should use CoreGraphics instead of AppKit." drawLineForMisspellingOrBadGrammar for non-Mac CG is fine for Chromium. Fixing the FIXMEs by switching to the common routine for Mac makes Chromium happy and WebKit cleaner.
Created attachment 30027 [details] Patch WITHOUT layout test rebaseline
Created attachment 30029 [details] patch WITH layout test rebaseline Rebaseline not strictly needed (test passes but checksum fails), but added for completeness.
This is currently the top crasher for Mac Chromium.
Created attachment 30057 [details] before/after screenshots "before" pictures captured from a vanilla safari. "after" pictures captured from a run of WebKitTools/Scripts/run-safari with the change patched in. (I used DYLD_PRINT_LIBRARIES to confirm it was in fact using my build of WebKit). Large images taken with WebKit scaling (e.g. Cmd-+), not with post-capture processing.
Mitz, can you look at this?
Comment on attachment 30029 [details] patch WITH layout test rebaseline > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 43246) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,17 @@ > +2009-05-05 John Grabowski <set EMAIL_ADDRESS environment variable> You should put your email address there. > + Unify use of CG-common routine for drawLineForMisspellingOrBadGrammar. > + Cleanup for WebKit, but required for Chromium happiness. There are tabs up there. They need to be changed into spaces so that the patch can be checked in. > +static const Color& spellingPatternColor() { > + static const Color spellingColor(255, 0, 0); > + return spellingColor; > +} > + > +static const Color& grammarPatternColor() { > + static const Color grammarColor(0, 128, 0); > + return grammarColor; > +} WebKit coding style is that the brace at the beginning of a function body goes on its own line. I think you should use DEFINE_STATIC_LOCAL for the colors.
Created attachment 30061 [details] feedback applied Thanks for the feedback; all comments applied.
With the style change is this acceptable?
Comment on attachment 30061 [details] feedback applied > +void GraphicsContext::drawLineForMisspellingOrBadGrammar(const IntPoint& point, int width, bool grammar) > +{ > + if (paintingDisabled()) > + return; > + > + // These are the same for misspelling or bad grammar > + const int patternHeight = 3; // 3 rows > + ASSERT(cMisspellingLineThickness == patternHeight); > + const int patternWidth = 4; // 4 pixels > + ASSERT(patternWidth == cMisspellingLinePatternWidth); I know this was copied from the Windows code, but why: i) not just use the constants, rather than hardcoded values with assertions that they equal the constants ii) why do the two assertions use different styles? r=me if you clean that up. I don't see an issue with just using the constants directly.
Created attachment 30116 [details] more feedback applied to patch. This one DOES have new layout tests included.
Variables and assertions removed; constants are used directly. (I don't know why that was done). Also, - made some comments into complete sentences - added bug URL in ChangeLogs - Added a space where needed; "//Top line" --> "// Top Line" Thanks Simon.
Comment on attachment 30116 [details] more feedback applied to patch. This one DOES have new layout tests included. r=me Those PNG images in the diff that show binary spew need to have their MIME type set in SVN so that diff knows they are binary.
Created attachment 30122 [details] same as above but with svn mime-type set for layout test result pngs svn propset svn:mime-type image/png LayoutTests/platform/mac/editing/spelling/*.png
Assign to levin for landing.
http://trac.webkit.org/changeset/43385 Unfortunately, I was unable to pick up the change for the png file which had the svn mime-type change (because git doesn't provide a way to set this) but the change should work without this and the file just kept the same mime-type as before.
Comment on attachment 30122 [details] same as above but with svn mime-type set for layout test result pngs Clearing the ?. The patch has been committed and this one is the same as the previous with just a mime type change.
Had to roll this change back see: http://trac.webkit.org/changeset/43389
Comment on attachment 30061 [details] feedback applied Clearing r+ to avoid having this show up in the commit queue.
Comment on attachment 30116 [details] more feedback applied to patch. This one DOES have new layout tests included. Clearing r+ to avoid having this show up in the commit queue.
Is there anything left to do here, can we mark this bug as fixed?
(In reply to comment #20) > Is there anything left to do here, can we mark this bug as fixed? jrg: ping. Could you close this?