Bug 122146 – Grammar markers are not updated when switching between 1x and 2x

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Myles C. Maxfield]

editing/spelling/grammar-markers.html pixel test fails on non-retina mac pro. However, both WK1 and WK2 Minibrowser + Safari render the dots correctly.

[Myles C. Maxfield]

editing/spelling/inline_spelling_markers.html as well

[Myles C. Maxfield]

Created attachment 213066 [details]
Patch

[Myles C. Maxfield]

This appears to only be the case when running all of editing/spelling. When running only the single test (editing/spelling/grammar-markers.html) the markers appear to be rendered properly. I'll see if I can get a better reduction.

[Myles C. Maxfield]

If the same runner runs both editing/spelling/grammar-markers.html and editing/spelling/grammar-markers-hidpi.html, the grammar dots are drawn incorrectly. It looks like the 2x resources are being used instead of the 1x resources

[Myles C. Maxfield]

The following command is showing the problem:

printf "file:///Volumes/Data/home/mmaxfield/src/Safari/OpenSource/LayoutTests/editing/spelling/grammar-markers-hidpi.html\nfile:///Volumes/Data/home/mmaxfield/src/Safari/OpenSource/LayoutTests/editing/spelling/grammar-markers.html\n" | DYLD_FRAMEWORK_PATH=/Volumes/Data/home/mmaxfield/Build/Debug ~/Build/Debug/DumpRenderTree --pixel-tests - > /tmp/out.png

[Myles C. Maxfield]

"static NSColor *grammarPatternColor = [makePatternColor(@"NSGrammarDot", @"GrammarDot", [NSColor greenColor], usingDotForGrammar) retain];"

makePatternColor is called as a static initializer, so it only gets called once. If the driver happens to run this first in a 2x context, we will get the 2x dot, and keep it indefinitely

[Ryosuke Niwa]

Comment on attachment 213066 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Marking the tests as ImageOnlyFailure for now

Since these tests are not ran as pixel tests by default, I'd prefer fixing the bug as opposed to adding a new test expectation.

[Alexey Proskuryakov]

Comment on attachment 213066 [details]
Patch

Good catch and analysis!

I agree with Ryosuke, this doesn't affect bots or normal test running, and there are way too many pixel test failures to start marking them in TextExpectations individually. Or is there a specific workflow that would be enhanced if we land this patch?

[Alexey Proskuryakov]

Do we have the same issue with spelling markers?

[Myles C. Maxfield]

I agree with rniwa and ap. The patch was uploaded before I discovered the root cause of the issue.

Yes, this occurs with spelling dots as well.

How do we feel about adding functions to GraphicsContext? :D

[Myles C. Maxfield]

The fact that this variable is static is a red herring - in ToT, the dots change when running editing/spelling/grammar-markers.html followed by editing/spelling/grammar-markers-hidpi.html (but not when running them in reverse order). This means that the logic that determines which image to draw is not done inside of ImageNamed:, but is rather done (probably) at draw time. However, removing the "static" keyword from the line above causes the correct dots to be drawn in both cases.

The difference between the two tests is a call to testRunner.setBackingScaleFactor(). Even if I make editing/spelling/grammar-markers.html call this function with an argument of 1, the dots still stay at 2x (when run after editing/spelling/grammar-markers-hidpi.html).

In addition, the problem does not seem to present itself when dragging WK1 minibrowser between a 1x and 2x screen. The dots redraw correctly in this case.

[Myles C. Maxfield]

In Safari, if the dots are created when the window is on a 1x display, then dragged to a 2x display, the dots are not redrawn using the higher-resolution assets. Instead, it appears like the lower-res dots are just blown up to the larger size

[Myles C. Maxfield]

Created attachment 213280 [details]
Patch

[Jon Lee]

Comment on attachment 213280 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:118
> +                patternColor = [NSColor redColor];

For clarity, I think these three lines might be better written within a single if…else statement.
if (spellingPatternImage) {
  usingDot = true;
  patternColor = [NSColor …];
} else {
  usingDot = false;
  patternColor = [NSColor redColor];
}

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:129
> +                patternColor = [NSColor greenColor];

Ditto here.

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:143
> +                patternColor = [NSColor blueColor];

Ditto here.

[Myles C. Maxfield]

Created attachment 213293 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 213293 [details]
Patch

Rejecting attachment 213293 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Jon Lee]

Comment on attachment 213293 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:86
> +static NSImage* makePattern(NSString* firstChoiceName, NSString* secondChoiceName)

While we're at it, let's fix the style here. Any Obj-C objects should be styled with a space between the class name and the asterisk.

So:
static NSImage *makePattern(NSString *firstChoiceName, NSString *secondChoiceName)

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:113
> +            usingDot = false;

This line is now unnecessary.

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:118
> +            } else {

I think the logic I initially proposed here isn't quite right. It's possible that [NSColor colorWithPatternImage] returns nil, so we want to fall back to using a color.

Instead of the else I think you want
if (patternColor) { ...

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:127
> +            usingDot = false;

This is now unnecessary.

[Myles C. Maxfield]

Comment on attachment 213293 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:86
>> +static NSImage* makePattern(NSString* firstChoiceName, NSString* secondChoiceName)
> 
> While we're at it, let's fix the style here. Any Obj-C objects should be styled with a space between the class name and the asterisk.
> 
> So:
> static NSImage *makePattern(NSString *firstChoiceName, NSString *secondChoiceName)

Done.

>> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:113
>> +            usingDot = false;
> 
> This line is now unnecessary.

Embarrassing....

Done.

>> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:118
>> +            } else {
> 
> I think the logic I initially proposed here isn't quite right. It's possible that [NSColor colorWithPatternImage] returns nil, so we want to fall back to using a color.
> 
> Instead of the else I think you want
> if (patternColor) { ...

Done.

>> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:127
>> +            usingDot = false;
> 
> This is now unnecessary.

Done.

[Myles C. Maxfield]

Created attachment 213377 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 213377 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        NSColor inspects the current graphics context to determine which
> +        resolution to use. Recreate the NSColor whenever we need to,
> +        but still cache the NSImage which is the heavier player here

You should explain what was causing the bug and how you fixed it.

> Source/WebCore/ChangeLog:12
> +        No new tests (OOPS!).

This line needs to be removed.

[Darin Adler]

Comment on attachment 213377 [details]
Patch

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

I am concerned that creating a new NSColor object every time we call drawLineForDocumentMarker will have a measurable performance impact.

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:143
>              // Constants for spelling pattern color.

Comment is wrong. Says "spelling", but also points out how pointless these comments are. I suggest removing all three. They say the same thing as the code beneath them.

[Myles C. Maxfield]

Darin: Would a better approach be to add a static function to GraphicsContext which resets the NSImage / NSColor, and to call it from Page::setDeviceScaleFactor()?

[Myles C. Maxfield]

What is the best way to see if this patch represents a performance regression? (@Darin)?

[Myles C. Maxfield]

Created attachment 213808 [details]
Patch

[Myles C. Maxfield]

Patch posted until I can figure out if this is a performance regression

[Darin Adler]

Comment on attachment 213808 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:108
> -    NSColor *patternColor;
> +    NSColor *patternColor = 0;

You won't need an assert if you take my advice below.

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:117
> +            }
> +            if (!patternColor) {

How about just using else here?

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:130
> +            }
> +            if (!patternColor) {

And here.

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:146
> +            }
> +            if (!patternColor) {

And here.

[Myles C. Maxfield]

Comment on attachment 213808 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:108
>> +    NSColor *patternColor = 0;
> 
> You won't need an assert if you take my advice below.

Do you mean the assert on line 93?

>> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:117
>> +            if (!patternColor) {
> 
> How about just using else here?

jonlee says: "It's possible that [NSColor colorWithPatternImage] returns nil, so we want to fall back to using a color."

[Myles C. Maxfield]

After timing the drawLineForDocumentMarker() line in InlineTextBox 500 times both with and without this patch, the average time the line takes (underlining the word "asdf") is 0.621% slower. Does that make this patch unacceptable?

[Myles C. Maxfield]

Oops, that was in a debug build. Trying release build now.

[Myles C. Maxfield]

In a release build, a full page of misspelled words appears to be 11.9% faster with this patch than without it (on my macbook pro)

[Myles C. Maxfield]

Created attachment 214283 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 214283 [details]
Patch

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

Have you measured the perf cost of re-creating the NSColor every time?

> Source/WebCore/ChangeLog:16
> +        want to recreate the NSColor whenever the device pixel ratio chagnes.

chagnes

[Myles C. Maxfield]

Previous metrics I reported didn't take tiling into account. Please disregard them.

[Myles C. Maxfield]

Created attachment 214532 [details]
Patch

[Myles C. Maxfield]

This is kind of a shot-in-the-dark, but I'm hoping we can work through making this acceptable.

[Myles C. Maxfield]

Any love?

[Dean Jackson]

Comment on attachment 214532 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:641
> +    // Unnecessary, since our document markers don't use resources

Nit: period needed at end of comment.

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:108
> +    // Be lazy.

I'm not sure what this means.

> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:187
> +    // Unnecessary, since our document markers don't use resources

Nit: period.

[Myles C. Maxfield]

Comment on attachment 214532 [details]
Patch

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

>> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:641
>> +    // Unnecessary, since our document markers don't use resources
> 
> Nit: period needed at end of comment.

Done.

>> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:108
>> +    // Be lazy.
> 
> I'm not sure what this means.

Done.

>> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:187
>> +    // Unnecessary, since our document markers don't use resources
> 
> Nit: period.

Done.

[Myles C. Maxfield]

Created attachment 214776 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 214776 [details]
Patch

Clearing flags on attachment: 214776

Committed r157739: <http://trac.webkit.org/changeset/157739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.