[webkit-reviews] review requested: [Bug 187632] Dark Mode: document markers are difficult to see : [Attachment 344968] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has asked	for review:
Bug 187632: Dark Mode: document markers are difficult to see
https://bugs.webkit.org/show_bug.cgi?id=187632

Attachment 344968: Patch

https://bugs.webkit.org/attachment.cgi?id=344968&action=review




--- Comment #6 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 344968
  --> https://bugs.webkit.org/attachment.cgi?id=344968
Patch

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

> Source/WebCore/rendering/RenderTheme.h:256
> +    virtual void drawLineForDocumentMarker(const RenderText&,
GraphicsContext&, const FloatPoint&, float, DocumentMarkerLineStyle);

Please give names to the parameters whose roles are not obvious.

> Source/WebCore/rendering/RenderThemeMac.h:171
> +    void drawLineForDocumentMarker(const RenderText&, GraphicsContext&,
const FloatPoint&, float, DocumentMarkerLineStyle) final;

Likewise.

> Source/WebCore/rendering/RenderThemeMac.mm:2803
> +    auto patternWidth = cMisspellingLinePatternWidth;

Why not just use cMisspellingLinePatternWidth below?

> Source/WebCore/rendering/RenderThemeMac.mm:2804
> +    auto patternHeight = cMisspellingLineThickness;

Ditto.

> Source/WebCore/rendering/RenderThemeMac.mm:2807
> +    // Make sure to draw only complete dots allowing slightly more
considering that the pattern ends with a transparent pixel.

Not sure this comment still makes sense.