Bug 25573 - misspelling underline cleanup
: misspelling underline cleanup
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-05-05 11:57 PST by
Modified: 2010-10-22 11:22 PST (History)


Attachments
Patch WITHOUT layout test rebaseline (13.59 KB, patch)
2009-05-05 11:58 PST, John Grabowski
no flags Review Patch | Details | Formatted Diff | Diff
patch WITH layout test rebaseline (174.07 KB, patch)
2009-05-05 11:59 PST, John Grabowski
mitz: review-
Review Patch | Details | Formatted Diff | Diff
before/after screenshots (23.91 KB, application/x-gzip)
2009-05-06 11:32 PST, John Grabowski
no flags Details
feedback applied (13.68 KB, patch)
2009-05-06 13:24 PST, John Grabowski
no flags Review Patch | 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 PST, John Grabowski
no flags Review Patch | 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 PST, John Grabowski
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-05-05 11:57:20 PST
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 From 2009-05-05 11:58:09 PST -------
Created an attachment (id=30027) [details]
Patch WITHOUT layout test rebaseline
------- Comment #2 From 2009-05-05 11:59:06 PST -------
Created an attachment (id=30029) [details]
patch WITH layout test rebaseline

Rebaseline not strictly needed (test passes but checksum fails), but added for completeness.
------- Comment #3 From 2009-05-05 13:11:10 PST -------
This is currently the top crasher for Mac Chromium.
------- Comment #4 From 2009-05-06 11:32:47 PST -------
Created an attachment (id=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 From 2009-05-06 11:42:12 PST -------
Mitz, can you look at this?
------- Comment #6 From 2009-05-06 11:48:31 PST -------
(From update of attachment 30029 [details])
> 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 From 2009-05-06 13:24:15 PST -------
Created an attachment (id=30061) [details]
feedback applied

Thanks for the feedback; all comments applied.
------- Comment #8 From 2009-05-07 14:46:07 PST -------
With the style change is this acceptable?
------- Comment #9 From 2009-05-07 15:10:50 PST -------
(From update of attachment 30061 [details])
> +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 From 2009-05-07 15:28:23 PST -------
Created an attachment (id=30116) [details]
more feedback applied to patch.  This one DOES have new layout tests included.
------- Comment #11 From 2009-05-07 15:30:18 PST -------
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 From 2009-05-07 18:02:15 PST -------
(From update of attachment 30116 [details])
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 From 2009-05-07 18:08:55 PST -------
Created an attachment (id=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 From 2009-05-07 18:15:20 PST -------
Assign to levin for landing.
------- Comment #15 From 2009-05-07 22:18:00 PST -------
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 From 2009-05-07 22:19:09 PST -------
(From update of attachment 30122 [details])
Clearing the ?. The patch has been committed and this one is the same as the previous with just a mime type change.
------- Comment #17 From 2009-05-07 23:03:33 PST -------
Had to roll this change back see:
http://trac.webkit.org/changeset/43389
------- Comment #18 From 2009-05-07 23:04:18 PST -------
(From update of attachment 30061 [details])
Clearing r+ to avoid having this show up in the commit queue.
------- Comment #19 From 2009-05-07 23:04:31 PST -------
(From update of attachment 30116 [details])
Clearing r+ to avoid having this show up in the commit queue.
------- Comment #20 From 2009-10-01 11:10:08 PST -------
Is there anything left to do here, can we mark this bug as fixed?
------- Comment #21 From 2010-10-21 20:52:33 PST -------
(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?