Bug 99417 – [chromium/mac] Make spelling indicator HighDPI

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

[chromium/mac] Make spelling indicator HighDPI

[Nico Weber]

Created attachment 168856 [details]
Patch

[Rik Cabanier]

Attachment 168856 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:643:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nico Weber]

Note that this patch is much simpler than it looks. It does the following things:

1. Undo the forking of the function code done in the patch linked in the changelog.
2. Just add the else block that contains the comment "            // Pattern: a b c c b a"

This probably needs new baselines too (i.e. this isn't the final patch, but the code should be final); I'll look into that tomorrow. See http://crbug.com/156022 for where the color values come from.

[Nico Weber]

Created attachment 168979 [details]
Patch

[WebKit Review Bot]

Attachment 168979 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:643:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nico Weber]

Created attachment 168982 [details]
Patch

[WebKit Review Bot]

Attachment 168982 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:643:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Comment on attachment 168982 [details]
Patch

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

Fix the ChangeLog, satisfy the bots, the rest is at your discretion.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:604
> +    ASSERT(deviceScaleFactor > 0);

Maybe this could be ASSERT(deviceScaleFactor == 1 || deviceScaleFactor == 2) (see below).

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:640
> +        } else {

Maybe:

} else if (deviceScaleFactor == 2) {
 ... 
} else {
  ASSERT_NOT_REACHED();
}

That way, if we ever try to support some future DPI, we can find the problems quickly.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:663
> +            // Pattern: a b c c b a
> +            //          d e f f e d
> +            //          g h j j h g
> +            //          k l m m l k
> +            //          n o p p o n
> +            //          q r s s r q
> +            for (int x = 0; x < colPixels; ++x) {
> +                uint32_t* row = misspellBitmap[index]->getAddr32(0, x);
> +                row[0] = colors[index][x * 3];
> +                row[1] = colors[index][x * 3 + 1];
> +                row[2] = colors[index][x * 3 + 2];
> +                row[3] = colors[index][x * 3 + 2];
> +                row[4] = colors[index][x * 3 + 1];
> +                row[5] = colors[index][x * 3];
> +                row[6] = transparentColor;
> +                row[7] = transparentColor;

Nit:  this looks like

a b c c b a
d e f f e d
g h j j h g
g h j j h g
d e f f e d
a b c c b a

to me.

[Nico Weber]

Created attachment 168986 [details]
Patch

[Nico Weber]

(In reply to comment #8)
> (From update of attachment 168982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168982&action=review
> 
> Fix the ChangeLog,

Done.

> satisfy the bots

I think the style bot is wrong here; the pixels would look strange with a 4 indent (not aligned with the previous line)

> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:604
> > +    ASSERT(deviceScaleFactor > 0);
> 
> Maybe this could be ASSERT(deviceScaleFactor == 1 || deviceScaleFactor == 2) (see below).

Done.

> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:640
> > +        } else {
> 
> Maybe:
> 
> } else if (deviceScaleFactor == 2) {
>  ... 
> } else {
>   ASSERT_NOT_REACHED();
> }
> 
> That way, if we ever try to support some future DPI, we can find the problems quickly.

Done. (also in the non-mac code)

> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:663
> > +            // Pattern: a b c c b a
> > +            //          d e f f e d
> > +            //          g h j j h g
> > +            //          k l m m l k
> > +            //          n o p p o n
> > +            //          q r s s r q
> > +            for (int x = 0; x < colPixels; ++x) {
> > +                uint32_t* row = misspellBitmap[index]->getAddr32(0, x);
> > +                row[0] = colors[index][x * 3];
> > +                row[1] = colors[index][x * 3 + 1];
> > +                row[2] = colors[index][x * 3 + 2];
> > +                row[3] = colors[index][x * 3 + 2];
> > +                row[4] = colors[index][x * 3 + 1];
> > +                row[5] = colors[index][x * 3];
> > +                row[6] = transparentColor;
> > +                row[7] = transparentColor;
> 
> Nit:  this looks like
> 
> a b c c b a
> d e f f e d
> g h j j h g
> g h j j h g
> d e f f e d
> a b c c b a
> 
> to me.

No, it's not x-symmetrical.

[WebKit Review Bot]

Attachment 168986 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:643:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Comment on attachment 168982 [details]
Patch

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

>>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:663
>>> +                row[7] = transparentColor;
>> 
>> Nit:  this looks like
>> 
>> a b c c b a
>> d e f f e d
>> g h j j h g
>> g h j j h g
>> d e f f e d
>> a b c c b a
>> 
>> to me.
> 
> No, it's not x-symmetrical.

Yeah, I misunderstood.  Sorry for the noise.

[Stephen White]

Comment on attachment 168986 [details]
Patch

We agreed style-bot can be ignored.  Ignore other bots at your peril.  :)  r=me

[WebKit Review Bot]

Comment on attachment 168986 [details]
Patch

Clearing flags on attachment: 168986

Committed r131483: <http://trac.webkit.org/changeset/131483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.