Bug 211274 – Use LocalCurrentGraphicsContext in WebKit::convertPlatformImageToBitmap()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Refactor LocalCurrentGraphicsContext to save/restore NSGraphicsContext.

In Bug 211160, there was some discussion about using LocalCurrentGraphicsContext with, "a tiny bit of refactoring," [Bug 211160 Comment #11] could be used to save/restore NSGraphicsContext.

However, I don't see how reusing that class would be a good design since I don't need (a) the CGContextRef or (b) the WebCore::GraphicsContext.  Or is the consensus that it's okay to add the nullptr checks and change m_savedGraphicsContext to a RefPtr<GraphicsContext>?


I really just need a LocalCurrentNSGraphicsContext:

class LocalCurrentNSGraphicsContext {
    WTF_MAKE_NONCOPYABLE(LocalCurrentNSGraphicsContext);
public:
    LocalCurrentNSGraphicsContext(NSGraphicsContext *newContext);
    ~LocalCurrentNSGraphicsContext();
private:
    RetainPtr<NSGraphicsContext> m_savedContext;
};

And:

LocalCurrentNSGraphicsContext::LocalCurrentNSGraphicsContext(NSGraphicsContext *newContext)
    : m_savedContext([NSGraphicsContext currentContext])
{
    NSGraphicsContext.currentContext = newContext;
}

LocalCurrentNSGraphicsContext::~LocalCurrentNSGraphicsContext()
{
    NSGraphicsContext.currentContext = m_savedContext.get();
}

[Darin Adler]

I don’t think we should add a new class. The first step would be to use this as-is:

    LocalCurrentGraphicsContext savedContext(*graphicsContext);

Then the question is whether we want to optimize to remove the unneeded calls to GraphicsContext::save/restore that LocalCurrentGraphicsContext does for us. There are two separate reasons save/restore are not needed:

1) we don’t make any changes to the context while using it

2) this temporary context is going to be deallocated after we are doing using it before anyone else uses it

But despite that, I am not sure the optimization is needed. If we wanted to optimize we would either need to make a separate class only used in cases like this, or have some way to tell the constructor either about (1) or about (2).

I would use it as-is and not worry about the modest additional cost of the calls to save/restore, but I can see going further if you like.

[Darin Adler]

(In reply to Darin Adler from comment #1)
> 2) this temporary context is going to be deallocated after we are doing
> using it before anyone else uses it

doing -> done

[David Kilzer (:ddkilzer)]

Well, I feel silly now because it was easy to use LocalCurrentGraphicsContext in WebKit::convertPlatformImageToBitmap().

Maybe the next step is to make LocalCurrentGraphicsContext work for PLATFORM(IOS_FAMILY):

#if PLATFORM(IOS_FAMILY)
    UIGraphicsPushContext(graphicsContext->platformContext());
    [image drawInRect:CGRectMake(0, 0, bitmap->size().width(), bitmap->size().height())];
    UIGraphicsPopContext();
#elif USE(APPKIT)

[David Kilzer (:ddkilzer)]

Created attachment 398315 [details]
Patch v1

[Darin Adler]

Comment on attachment 398315 [details]
Patch v1

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

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:412
> +    {
> +        LocalCurrentGraphicsContext savedContext(*graphicsContext);
> +        [image drawInRect:NSMakeRect(0, 0, bitmap->size().width(), bitmap->size().height()) fromRect:NSZeroRect operation:NSCompositingOperationSourceOver fraction:1 respectFlipped:YES hints:nil];
> +    }

We don’t really need the braces here since the scope only goes one line further (after the return), but if you prefer it this way I am OK with it.

[David Kilzer (:ddkilzer)]

Created attachment 398345 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 398345 [details]
Patch for landing

Marking cq+ since "Patch v1" passed all EWS queues and this simply removed curly braces creating a code block.

[EWS]

Committed r261071: <https://trac.webkit.org/changeset/261071>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398345 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62829922>

[Yusuke Suzuki]

Committed r261077: <https://trac.webkit.org/changeset/261077>

[David Kilzer (:ddkilzer)]

(In reply to Yusuke Suzuki from comment #10)
> Committed r261077: <https://trac.webkit.org/changeset/261077>

Thanks!  I’m amazed that EWS didn’t catch this.  :(