Bug 211274 - Use LocalCurrentGraphicsContext in WebKit::convertPlatformImageToBitmap()
Summary: Use LocalCurrentGraphicsContext in WebKit::convertPlatformImageToBitmap()
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
Keywords: InRadar
Depends on:
Blocks: 211660
  Show dependency treegraph
Reported: 2020-04-30 20:05 PDT by David Kilzer (:ddkilzer)
Modified: 2020-05-08 21:09 PDT (History)
5 users (show)

See Also:

Patch v1 (3.88 KB, patch)
2020-05-03 09:47 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch for landing (3.66 KB, patch)
2020-05-03 20:30 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-04-30 20:05:22 PDT
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 {
    LocalCurrentNSGraphicsContext(NSGraphicsContext *newContext);
    RetainPtr<NSGraphicsContext> m_savedContext;


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

    NSGraphicsContext.currentContext = m_savedContext.get();
Comment 1 Darin Adler 2020-05-01 12:10:42 PDT
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.
Comment 2 Darin Adler 2020-05-01 12:11:05 PDT
(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
Comment 3 David Kilzer (:ddkilzer) 2020-05-03 09:40:57 PDT
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):

    [image drawInRect:CGRectMake(0, 0, bitmap->size().width(), bitmap->size().height())];
Comment 4 David Kilzer (:ddkilzer) 2020-05-03 09:47:03 PDT
Created attachment 398315 [details]
Patch v1
Comment 5 Darin Adler 2020-05-03 16:20:20 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2020-05-03 20:30:46 PDT
Created attachment 398345 [details]
Patch for landing
Comment 7 David Kilzer (:ddkilzer) 2020-05-03 20:32:44 PDT
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.
Comment 8 EWS 2020-05-03 21:14:56 PDT
Committed r261071: <https://trac.webkit.org/changeset/261071>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398345 [details].
Comment 9 Radar WebKit Bug Importer 2020-05-03 21:15:15 PDT
Comment 10 Yusuke Suzuki 2020-05-04 01:27:10 PDT
Committed r261077: <https://trac.webkit.org/changeset/261077>
Comment 11 David Kilzer (:ddkilzer) 2020-05-04 02:27:22 PDT
(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.  :(