Bug 91965 – [chromium mac] Transforms to GraphicsContext need to be synced to CGContext on Chromium Mac

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Keishi Hattori]

Transforms to GraphicsContext are being ignored by chromium.
It's because Skia requires CGContext to be recreated to reflect the current SkCanvas state.

[Keishi Hattori]

Created attachment 153751 [details]
Patch

[Keishi Hattori]

I think this needs to be fixed by either
- use NSAffineTransform instead of GraphicsContext
- or move the LocalCurrentGraphicsContext initializations to after the transforms

[Simon Fraser (smfr)]

The title of this bug is overly general. Please fix it.

[Keishi Hattori]

Changed to: Transforms to GraphicsContext need to be synced to CGContext on Chromium Mac

[Keishi Hattori]

Could caryclark@ or eseidel@ give me feedback?

I read the commends on Bug 62213 but I can't understand why LocalCurrentGraphicsContext was implemented this way. How should this be fixed?

[Cary Clark]

At the time I coded it, LocalCurrentGraphicsContext appeared to be a convenient place to set up the SkCanvas with the current graphics transform and clip, set up a bitmap for CoreGraphics to draw into, let CoreGraphics draw the UI element, and then at the end (in the LocalCurrentGraphicsContext destructor), draw the resulting bitmap.

As you've noted, the calls through paintInfo.context->translate and so on and the calls to GraphicsContext::save() and restore() (as wrapped by GraphicsContextStateSaver) don't work as intended because they are operating on the Skia context instead of the CG Context.

One possible solution would be to vector these calls to CG during the scope of the LocalCurrentGraphicsContext. That is, while an SkBitLocker is in play, changes to the graphics context and transform would be redirected to the current CoreGraphics context instead of the SkCanvas context.

Does this sound like it would work?

[Build Bot]

Comment on attachment 153751 [details]
Patch

Attachment 153751 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13342654

[Eric Seidel (no email)]

LocalCurrentGraphicsContext was created solely as a way to bridge our CG-only world of WebKIt (where "graphics context" objects are passed around on the stack) into AppKit drawing methods, which operate on the "current context" -- a global static.

http://trac.webkit.org/changeset/16286

I have vague recollections of bug 62213 and Skia adding additional functionality to LocalCurrentGraphicsContext, but you'd have to talk to Cary for more detailed design information.

[Eric Seidel (no email)]

Before LocalCurrentGraphicsContext, we had a bunch of ugly mac-specific code for dealing with setting up/restoring the current graphics context.  We can certainly move where LocalCurrentGraphicsContext is initialized if that would help, including using local blocks {} to restrict it's duration.

[Keishi Hattori]

Created attachment 157365 [details]
Patch

[Keishi Hattori]

(In reply to comment #9)
> Before LocalCurrentGraphicsContext, we had a bunch of ugly mac-specific code for dealing with setting up/restoring the current graphics context.  We can certainly move where LocalCurrentGraphicsContext is initialized if that would help, including using local blocks {} to restrict it's duration.

OK so obviously we shouldn't revert to using Mac specific code.

The problem isn't that LocalCurrentGraphicsContext lives too long, but that it is initialized too early. It needs to come after the transform calls and before the draw calls.

The patch I uploaded right now is an my attempt at implementing Cary's idea of syncing the transform calls to GraphicsContext to CGContext. Could you take a look and see if I am headed in the right direction?

[Build Bot]

Comment on attachment 157365 [details]
Patch

Attachment 157365 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13452861

[Build Bot]

Comment on attachment 157365 [details]
Patch

Attachment 157365 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13452897

[Keishi Hattori]

Created attachment 157922 [details]
Patch

[Keishi Hattori]

(In reply to comment #11)
> (In reply to comment #9)
> > Before LocalCurrentGraphicsContext, we had a bunch of ugly mac-specific code for dealing with setting up/restoring the current graphics context.  We can certainly move where LocalCurrentGraphicsContext is initialized if that would help, including using local blocks {} to restrict it's duration.
> 
> OK so obviously we shouldn't revert to using Mac specific code.
> 
> The problem isn't that LocalCurrentGraphicsContext lives too long, but that it is initialized too early. It needs to come after the transform calls and before the draw calls.
> 
> The patch I uploaded right now is an my attempt at implementing Cary's idea of syncing the transform calls to GraphicsContext to CGContext. Could you take a look and see if I am headed in the right direction?

caryclark@ could you take a look?

[Cary Clark]

(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > Before LocalCurrentGraphicsContext, we had a bunch of ugly mac-specific code for dealing with setting up/restoring the current graphics context.  We can certainly move where LocalCurrentGraphicsContext is initialized if that would help, including using local blocks {} to restrict it's duration.
> > 
> > OK so obviously we shouldn't revert to using Mac specific code.
> > 
> > The problem isn't that LocalCurrentGraphicsContext lives too long, but that it is initialized too early. It needs to come after the transform calls and before the draw calls.
> > 
> > The patch I uploaded right now is an my attempt at implementing Cary's idea of syncing the transform calls to GraphicsContext to CGContext. Could you take a look and see if I am headed in the right direction?
> 
> caryclark@ could you take a look?
My apologies for not replying sooner. I was on vacation last week.
The change looks good, but is difficult to follow because of its complexity. Are there old tests that this fixes, or new tests that exercise this? I couldn't see the difference in the checked-in test images.