Bug 25573 - misspelling underline cleanup
Summary: misspelling underline cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: John Grabowski
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-05 11:57 PDT by John Grabowski
Modified: 2010-10-22 11:22 PDT (History)
7 users (show)

See Also:


Attachments
Patch WITHOUT layout test rebaseline (13.59 KB, patch)
2009-05-05 11:58 PDT, John Grabowski
no flags Details | Formatted Diff | Diff
patch WITH layout test rebaseline (174.07 KB, patch)
2009-05-05 11:59 PDT, John Grabowski
mitz: review-
Details | Formatted Diff | Diff
before/after screenshots (23.91 KB, application/x-gzip)
2009-05-06 11:32 PDT, John Grabowski
no flags Details
feedback applied (13.68 KB, patch)
2009-05-06 13:24 PDT, John Grabowski
no flags Details | Formatted Diff | Diff
more feedback applied to patch. This one DOES have new layout tests included. (174.22 KB, patch)
2009-05-07 15:28 PDT, John Grabowski
no flags Details | Formatted Diff | Diff
same as above but with svn mime-type set for layout test result pngs (156.16 KB, patch)
2009-05-07 18:08 PDT, John Grabowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Grabowski 2009-05-05 11:57:20 PDT
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.
Comment 1 John Grabowski 2009-05-05 11:58:09 PDT
Created attachment 30027 [details]
Patch WITHOUT layout test rebaseline
Comment 2 John Grabowski 2009-05-05 11:59:06 PDT
Created attachment 30029 [details]
patch WITH layout test rebaseline

Rebaseline not strictly needed (test passes but checksum fails), but added for completeness.
Comment 3 John Grabowski 2009-05-05 13:11:10 PDT
This is currently the top crasher for Mac Chromium.
Comment 4 John Grabowski 2009-05-06 11:32:47 PDT
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.
Comment 5 Dimitri Glazkov (Google) 2009-05-06 11:42:12 PDT
Mitz, can you look at this?
Comment 6 mitz 2009-05-06 11:48:31 PDT
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.
Comment 7 John Grabowski 2009-05-06 13:24:15 PDT
Created attachment 30061 [details]
feedback applied

Thanks for the feedback; all comments applied.
Comment 8 Jon@Chromium 2009-05-07 14:46:07 PDT
With the style change is this acceptable?
Comment 9 Simon Fraser (smfr) 2009-05-07 15:10:50 PDT
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.
Comment 10 John Grabowski 2009-05-07 15:28:23 PDT
Created attachment 30116 [details]
more feedback applied to patch.  This one DOES have new layout tests included.
Comment 11 John Grabowski 2009-05-07 15:30:18 PDT
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 12 Simon Fraser (smfr) 2009-05-07 18:02:15 PDT
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.
Comment 13 John Grabowski 2009-05-07 18:08:55 PDT
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
Comment 14 David Levin 2009-05-07 18:15:20 PDT
Assign to levin for landing.

Comment 15 David Levin 2009-05-07 22:18:00 PDT
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 16 David Levin 2009-05-07 22:19:09 PDT
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.
Comment 17 David Levin 2009-05-07 23:03:33 PDT
Had to roll this change back see:
http://trac.webkit.org/changeset/43389
Comment 18 David Levin 2009-05-07 23:04:18 PDT
Comment on attachment 30061 [details]
feedback applied

Clearing r+ to avoid having this show up in the commit queue.
Comment 19 David Levin 2009-05-07 23:04:31 PDT
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.
Comment 20 Jeremy Moskovich 2009-10-01 11:10:08 PDT
Is there anything left to do here, can we mark this bug as fixed?
Comment 21 Hajime Morrita 2010-10-21 20:52:33 PDT
(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?