Bug 25573 – misspelling underline cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[John Grabowski]

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.

[John Grabowski]

Created attachment 30027 [details]
Patch WITHOUT layout test rebaseline

[John Grabowski]

Created attachment 30029 [details]
patch WITH layout test rebaseline

Rebaseline not strictly needed (test passes but checksum fails), but added for completeness.

[John Grabowski]

This is currently the top crasher for Mac Chromium.

[John Grabowski]

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.

[Dimitri Glazkov (Google)]

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 @@
> [phone]  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.

[John Grabowski]

Created attachment 30061 [details]
feedback applied

Thanks for the feedback; all comments applied.

[Jon@Chromium]

With the style change is this acceptable?

[Simon Fraser (smfr)]

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.

[John Grabowski]

Created attachment 30116 [details]
more feedback applied to patch.  This one DOES have new layout tests included.

[John Grabowski]

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.

[Simon Fraser (smfr)]

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.

[John Grabowski]

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

[David Levin]

Assign to levin for landing.

[David Levin]

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.

[David Levin]

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.

[David Levin]

Had to roll this change back see:
http://trac.webkit.org/changeset/43389

[David Levin]

Comment on attachment 30061 [details]
feedback applied

Clearing r+ to avoid having this show up in the commit queue.

[David Levin]

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.

[Jeremy Moskovich]

Is there anything left to do here, can we mark this bug as fixed?

[Hajime Morrita]

(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?