Bug 10648 – unnecessary NSGraphicsContext usage should be removed from WebCore

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

All NSGraphicsContext usage should be removed from WebCore

This pertains particularly to the use of [NSGraphicsContext currentContext]

This isn't too hard of a task.  There are only 45 uses of NSGraphicsContext left in the code.  Most of them are unnecessary at this point now that form controls are almost all rendered inside the engine code.

A few enhancements will be necessary to GraphicsContext before this is possible (including adding an isPrinting state as well as an isFlipped state -- both for text drawing).

NSGraphicsContext currentContext will still be needed when calling up into plugins, but should be otherwise unnecessary inside WebCore.

(Removing this will fix some crashers in SVG if nothing else -- due to mishandling of NSGraphicsContext in KRenderingGraphicsContext.)

[Darin Adler]

Sounds OK.

(In reply to comment #0)
> A few enhancements will be necessary to GraphicsContext before this is possible
> (including adding an isPrinting state as well as an isFlipped state -- both for
> text drawing).

But I think that's wrong. That reflects our old thinking, but since then we've explored this further.

We won't need an "isFlipped" state because we are going to only support drawing into flipped graphics contexts.

And we won't need an isPrinting state because we can and do handle that at a higher level. The document has an isPrinting state.

[Eric Seidel (no email)]

(In reply to comment #1)
> Sounds OK.
> 
> (In reply to comment #0)
> > A few enhancements will be necessary to GraphicsContext before this is possible
> > (including adding an isPrinting state as well as an isFlipped state -- both for
> > text drawing).
> 
> But I think that's wrong. That reflects our old thinking, but since then we've
> explored this further.
> 
> We won't need an "isFlipped" state because we are going to only support drawing
> into flipped graphics contexts.

Agreed.

> And we won't need an isPrinting state because we can and do handle that at a
> higher level. The document has an isPrinting state.

We'll just have to pass a bool down into the Font routines so they can choose whether to use Screen or Printer fonts appropriately.

[Eric Seidel (no email)]

Created attachment 10446 [details]
Gets us damn close.

[Darin Adler]

Comment on attachment 10446 [details]
Gets us damn close.

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.

[Eric Seidel (no email)]

(In reply to comment #4)
> +#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.

My understanding of that line is "if the minimum deployment version required is less than or equal to tiger, do this".  Meaning "if we're not leopard or later".

I'll fix the release.  And rename the class.  I still think the wk* calls could take a context instead of relying on the current one.  But I agree, the class I added cleans up the code enough to not really care if it uses the current context or not.

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > +#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.
> 
> My understanding of that line is "if the minimum deployment version required is
> less than or equal to tiger, do this".  Meaning "if we're not leopard or
> later".

Doh! <=! Somehow I thought it was <, not <=. My bad, you had it right all along.

> I still think the wk* calls could take a context instead of relying on the current one.

Sure, if we find we're able to do that some day it would be nice cleanup.

Did any of my other comments lead you to make code changes?

[Eric Seidel (no email)]

Created attachment 10470 [details]
Addressing darin's concerns

I also ran the layout tests on this one.

[Eric Seidel (no email)]

Created attachment 10471 [details]
Patch which actually passes the layout tests

I lied.  The layout tests weren't completely done yet.  This version actually passes (at least all of the layout tests which are currently supposed to pass on TOT).

[Darin Adler]

Comment on attachment 10471 [details]
Patch which actually passes the layout tests

CurrentGraphicsContextSetter should inherit from Noncopyable

r=me