Summary: | misspelling underline cleanup | ||
---|---|---|---|
Product: | WebKit | Reporter: | John Grabowski <jrg> |
Component: | WebCore Misc. | Assignee: | John Grabowski <jrg> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | jon, jrg, levin, mitz, morrita, oliver, playmobil |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Mac | ||
OS: | OS X 10.5 | ||
Attachments: |
Description
John Grabowski
2009-05-05 11:57:20 PDT
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? |