Bug 122365 - Include misspelling dot gap width when centering misspelling dots
Summary: Include misspelling dot gap width when centering misspelling dots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-10-04 17:33 PDT by Myles C. Maxfield
Modified: 2013-10-23 16:49 PDT (History)
5 users (show)

See Also:


Attachments
Patch (93.68 KB, patch)
2013-10-22 14:18 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (23.06 KB, patch)
2013-10-22 16:05 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (23.89 KB, patch)
2013-10-23 13:10 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (44.49 KB, patch)
2013-10-23 15:51 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2013-10-04 17:33:48 PDT
Include misspelling dot gap width when centering misspelling dots
Comment 1 Myles C. Maxfield 2013-10-22 14:18:06 PDT
Created attachment 214881 [details]
Patch
Comment 2 Myles C. Maxfield 2013-10-22 16:05:52 PDT
Created attachment 214897 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2013-10-23 10:43:42 PDT
<rdar://problem/15299813>
Comment 4 Jon Lee 2013-10-23 11:34:02 PDT
Comment on attachment 214897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214897&action=review

> Source/WebCore/ChangeLog:11
> +        (WebCore::GraphicsContext::drawLineForDocumentMarker):

More explanation here please. Why is there a new (widthMod > 0) check? What is the new code doing to the offset? Why is it a floor()?

> LayoutTests/editing/spelling/centering-misspelling-dots-expected.txt:58
> +caret: position 14 of child 0 {#text} of child 3 {DIV} of body

Is having expected text useful for this test?
Comment 5 Myles C. Maxfield 2013-10-23 13:08:14 PDT
Comment on attachment 214897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214897&action=review

>> Source/WebCore/ChangeLog:11
>> +        (WebCore::GraphicsContext::drawLineForDocumentMarker):
> 
> More explanation here please. Why is there a new (widthMod > 0) check? What is the new code doing to the offset? Why is it a floor()?

Done.

>> LayoutTests/editing/spelling/centering-misspelling-dots-expected.txt:58
>> +caret: position 14 of child 0 {#text} of child 3 {DIV} of body
> 
> Is having expected text useful for this test?

I don't think it's possible to run a pixel test without expected text. When I try, it says that the test failed due to missing results.
Comment 6 Myles C. Maxfield 2013-10-23 13:10:10 PDT
Created attachment 214985 [details]
Patch
Comment 7 Jon Lee 2013-10-23 15:08:21 PDT
Comment on attachment 214985 [details]
Patch

Provisional r=me.
Comment 8 Simon Fraser (smfr) 2013-10-23 15:09:56 PDT
Comment on attachment 214985 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214985&action=review

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:-170
> -    // NOTE: Code here used to shift the underline to the left and increase the width
> -    // to make sure everything gets underlined, but that results in drawing out of
> -    // bounds (e.g. when at the edge of a view) and could make it appear that the
> -    // space between adjacent misspelled words was underlined.

I would like to see your testcase testing "the space between misspelled words".
Comment 9 Myles C. Maxfield 2013-10-23 15:51:29 PDT
Created attachment 215004 [details]
Patch
Comment 10 WebKit Commit Bot 2013-10-23 16:49:20 PDT
Comment on attachment 215004 [details]
Patch

Clearing flags on attachment: 215004

Committed r157900: <http://trac.webkit.org/changeset/157900>
Comment 11 WebKit Commit Bot 2013-10-23 16:49:23 PDT
All reviewed patches have been landed.  Closing bug.