[webkit-reviews] review denied: [Bug 10648] All NSGraphicsContext usage should be removed from WebCore : [Attachment 10446] Gets us damn close.

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 10648: All NSGraphicsContext usage should be removed from WebCore
http://bugzilla.opendarwin.org/show_bug.cgi?id=10648

Attachment 10446: Gets us damn close.
http://bugzilla.opendarwin.org/attachment.cgi?id=10446&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This patch is great! Thanks for tackling this.

+#if MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_4
+    // Tiger versions of Safari don't properly flip the NSGraphicsContext
before calling WebCore

That #if looks wrong. It's saying "if < 10.4" which is true for Panther, not
for Tiger.

WebCoreNSGraphicsContextBridge retains but does not release
m_savedNSGraphicsContext!

WebCoreNSGraphicsContextBridge is a great idea for a class, but I don't think
it should have "bridge" in its name. And since it's in the WebCore namespace I
don't think its name should begin with WebCore. I think we should call the
class CurrentGraphicsContextSetter. It would be pretty clear even without the
NS since only AppKit has a current context, not CG or WebCore. I also think
it's not a good idea to call the local variable "hack". I suggest "setter".
Also, the class should inherit from Noncopyable. I think the class might need
to be in a header, too. There will probably be more places we want to use it. I
also don't think the constructor and destructor need to be inlined.

I don't think that every use of
WebCoreNSGraphicsContextBridge/CurrentGraphicsContextSetter needs a FIXME. The
comment should just say something like "Set up the NSGraphicsContext, since
this function uses it" or something to that effect. There's no need to say that
it "ignores the GraphicsContext in the PaintInfo" any more, since this will no
longer be true. And no need to say FIXME. In fact, I really don't think we need
a comment at all, but if we do have one it should be short and sweet.

I know I suggested putting the printing boolean in as a separate variable, but
you should instead consider making it part of the TextStyle class; it would
keep the API simpler and I think it would work really well. I also think it
should be called something more like "use printer font" than "is printing".

I think the FIXME in GraphicsContext::drawLineForMisspelling is incorrect. That
code could be changed to use CGPattern and need not be platform-independent. Or
the code could be changed to create  a suitable NSGraphicsContext as
GraphicsContext::setCompositeOperation or WebCoreNSGraphicsContextBridge. So
please don't check that comment in as is.

I don't think the new comment in -[WebCoreFrameBridge drawRect:] is
particularly helpful. It's calling a function that takes a GraphicsContext, so
yes, it needs to make a GraphicsContext. But I don't think that needs to be
called out. I'm also not that fond of the comment you added to
FileButtonMac.mm, but that file is about to be deleted so there's no real point
debating it! I suggest you just leave both of those files as-is.

Why the code to handle 0 for graphicsContext in ATSULayoutParameters? The old
code didn't seem to allow 0.

+    ATSUAttributeValuePtr values[] = { (context ? &context : 0),
&lineLayoutOptions, &rtl, &overrideSpecifier };

I'm specifically concerned about the above line. Is it really OK to pass 0 for
kATSUCGContextTag?

Also in Font::displayComplexText, there's this line of code:

    [nsColor(graphicsContext->pen().color()) set];

Since -[NSColor set] works on the current NSGraphicsContext, what are we doing
to ensure that's set correctly?

For this comment:

    // IMO this is confusing, setting the fill based on the pen color. - ecs
 
Instead you should say something more like "text needs to respect the pen
color, but it's actually the fill color that controls the color of the text, so
we need to set the fill color of the CGContext based on the pen color of the
GraphicsContext". That way instead of creating doubt and confusion for later
programmers, we clarify things for them.

+    // This is another entry into WebCore from AppKit.  We build a new
GraphicsContext from the current context before continuing.

I think that comment is not all that helpful. Sure, you need to create a
GraphicsContext since you are calling a function that takes one. But there's no
need to have a comment about the "bigger picture", in my opinion. A better
comment might be more like "Draw into the NSGraphicsContext currentContext
because that's what NSTableView expects". I don't think we need to highlight
the creation of a WebCore::GraphicsContext, but it is good to highlight why
this code is picking up the current context.

Similarly, for WebCoreDrawTextAtPoint, I think the comment should just be
"Draws into the current NSGraphicsContext" or the like.

How did you test? I'm particularly curious about why testing didn't show the
problem with the 10.4 ifdef.

review- because of that 10.3/4 snafu and the missing release in
WebCoreNSGraphicsContextBridge. Please consider my other comments too.