[webkit-reviews] review denied: [Bug 23291] -webkit-box-shadow doesn't work on Qt-WebKit : [Attachment 38878] Patch proposal that addresses Eric's concerns (see comment)

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Carol Szabo <carol.szabo at nokia.com>'s
request for review:
Bug 23291: -webkit-box-shadow doesn't work on Qt-WebKit
https://bugs.webkit.org/show_bug.cgi?id=23291

Attachment 38878: Patch proposal that addresses Eric's concerns (see comment)
https://bugs.webkit.org/attachment.cgi?id=38878&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
The ChangeLog is now out of date (it doesn't mention the new functions)

 621 static void inline DrawShadowPathFilled(GraphicsContext* self, QPainter*
p, const QPainterPath *path)

self is a reserved word for Obj-C so using it here might be confusing.	I would
have called that "context" instead.

Also, DrawShadowPathFilled does not match WebKit naming style:
http://webkit.org/coding/coding-style.html

I would expect the check-webkit-style script to catch the naming issue, but
maybe not?

Also the indent is wrong.  4spaces, not 2.

This definitely looks better.  I mean to fully share the shadow code we'd have
to come up with some object level abstraction with virtual functions for the
"draw" operation, and storage for the various parameters the draw() would need.
  This improved version is definitely less code duplication than before. 
Really it would be best if QPainter just knew how to do this.

r- for the style issues.