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(); }
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.
(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
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)
Created attachment 398315 [details] Patch v1
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.
Created attachment 398345 [details] Patch for landing
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.
Committed r261071: <https://trac.webkit.org/changeset/261071> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398345 [details].
<rdar://problem/62829922>
Committed r261077: <https://trac.webkit.org/changeset/261077>
(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. :(