|
Description
Bruce Q. Hammond
2007-01-31 15:20:03 PST
Created attachment 12836 [details]
This corrects 12515: Plug-ins that draw through the Quickdraw interface fail in a CGBitmapContex.
checkin message "This corrects 12515: Plug-ins that draw through the Quickdraw interface fail in a CGBitmapContex."
Comment on attachment 12836 [details]
This corrects 12515: Plug-ins that draw through the Quickdraw interface fail in a CGBitmapContex.
A couple of style issues with the patch.
We usually #define these to be equal to '1', not 'true'
+#define ALLOW_QUICKDRAW_OFFSCREEN_DRAWING true
There shouldn't be spaces around cgByteOrder and the brace should go on the same line
+ switch ( cgByteOrder )
+ {
Spaces are needed around the =, and the extra newline seems unneeded
+ case kCGBitmapByteOrder16Little:
+ format= k16LE555PixelFormat;
+ break;
+ case kCGBitmapByteOrder32Little:
+ format= k32BGRAPixelFormat;
+ break;
+
+ case kCGBitmapByteOrder16Big:
+ format = k16BE555PixelFormat;
+ break;
+ case kCGBitmapByteOrder32Big:
+ format= k32ARGBPixelFormat;
+ break;
Please use ASSERT_NOT_REACHED() instead of ASSERT(0) and the ending brace is indented too far.
+ default:
+ ASSERT(0); // should not be here.
+ }
You might want to consider not using the local variable 'format' at all just return from inside the switch.
Spaces inside the braces here
+enum ContextType_priv {kCGBitmapContextType = 4};
There are two spaces between the words 'struct' and 'CGContext_priv'. Please put the * next to the word 'void'.
+struct CGContext_priv {
+ void * s1;
We tend to just use 'unsigned' in these cases.
+ unsigned int s4;
Please remove or explain this comment
+ // blah blah blah
Please use a static_cast<> here. There is also extra whitespace inside the parenthesis.
+ CGContext_priv* context = (CGContext_priv*) contextRef;
+ return ( context->type == kCGBitmapContextType);
Are all the newlines necessary?
+
+#endif
+
+
+
+
Braces should be on the same line.
+ if (IsBitmapContext(currentContext))
+ {
Two spaces are being used again here.
+ Rect offscreenBounds;
+ int rowBytes = CGBitmapContextGetBytesPerRow(currentContext);
+ offscreenBounds.top = 0;
+ offscreenBounds.left = 0;
+ offscreenBounds.right = CGBitmapContextGetWidth(currentContext);
The brace should be on the same line, the space on the right of the 0 should be removed, and you can change this to if (!pixelFormat) {.
+ if (pixelFormat == 0 )
+ {
Split on to multiple lines.
+ offscreenBounds.top = offscreenBounds.left = offscreenBounds.right = offscreenBounds.bottom = 0;
Need spaces on right of commas.
+ QDErr err = NewGWorldFromPtr(&pOffScreenGWorld, pixelFormat,&offscreenBounds,0,0, 0, (char*)bits, rowBytes);
Brace should be on the same line.
+ if (!err)
+ {
Same here.
+ }
+ else
and here
+ if (IsBitmapContext(currentContext))
+ {
You seem to just be adding extra whitespace here, please revert it.
- [self setWindowIfNecessary];
+ [self setWindowIfNecessary];
and here
- if (drawingModel == NPDrawingModelQuickDraw && !isTransparent && event->what == updateEvt) {
+ if (drawingModel == NPDrawingModelQuickDraw && !isTransparent && event->what == updateEvt) {
Finally, one last brace should be on the same line
+ if (newWebView)
+ {
There are also a couple places where you added trailing whitespace, but that is just a person pet peeve :)
Marking r-.
You can find our coding style guidelines here: http://webkit.org/coding/coding-style.html. OK, I will make these style changes and resubmit. (In reply to comment #4) > OK, I will make these style changes and resubmit. Please include a ChangeLog in the patch using WebKitTools/Scripts/prepare-ChangeLog. Thanks! Created attachment 12885 [details]
Allows QD based plugins to draw into offscreen bitmap contexts.
This fixes style guidelines conformance.
Changes technique to detect CGBitmapContext to be less fragile and one that we hope will be sanctioned in future OS X releases.
Also removes trailing whitespace from lines.
This has been tested for regressions in Safari.
No unit tests seems to be practical.
Comment on attachment 12885 [details]
Allows QD based plugins to draw into offscreen bitmap contexts.
All the trailing space stripping in this patch makes it unnecessarily hard to review. Lets do that in a separate patch either before or after this one.
What is this new ALLOW_QUICKDRAW_OFFSCREEN_DRAWING flag? Why would it ever be 0?
+// We would like to simply say: if(CGBitmapContextGetBitmapInfo(currentContext)) {...}
+// however using the public API in the documented and approved way to check
+// for an CGBitmapContext, will log a warning to stdout if it's not a bitmap context.
+// ...So we cheat a little bit.
+CG_EXTERN_C_BEGIN
+enum ContextType_priv { PrivateCGBitmapContextType = 4 };
+extern ContextType_priv CGContextGetType(CGContextRef c);
+CG_EXTERN_C_END
It's not really OK to put this kind of thing into WebCore. If something like this is necessary we do it in WebKitSystemInterface.
Why is it OK to bypass all the clipping logic? It looks like the new code path ignores clipping and will ask the plug-in to paint too much each time which will presumably hurt performance.
This is not really a P1, it would not block shipping a new webkit. Created attachment 13265 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V3)
Addresses requests by previous reviewers.
NOTE: I don't have access required to put a routine in the WebKitSystemInterface.
Comment on attachment 13265 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V3)
Needs a ChangeLog entry.
This identifier is missing a "t": WKCGContextIsBitmapContex
Adding things to WebKitSystemInterface is something that needs to be handled internally at Apple, so I don't think it's appropriate to have it here in an open source patch! You need to work with someone on my team -- please contact Oliver Hunt.
Are you sure we don't want the "forUpdate" logic when the target is a CGBitmapContext? I'm concerned that we'll end up drawing outside the damaged area in that case -- I think the logic applies equally well to the CGBitmapContext case, but there may be something I'm missing.
+ SetGWorld(qdPortState->oldPort, NULL);
SetPort(qdPortState->oldPort);
Do we really need both the SetGWorld call and the SetPort call? My QuickDraw is a little rusty, but I don't think that's right. I seem to recall that in the old days you had to separately save and restore the port and the gdevice, and SetGWorld would set both.
How is the TSMEventHandler change related to this bug? It doesn't seem to have anything to do with CGBitmapContext.
Created attachment 13442 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V4)
Now determines if we have a CGBitmapContext via WebKitSystemInteface.
Minimizes drawing by normalizing window clip data to the offscreen context (requested by Darin Adler)
Comment on attachment 13442 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V4)
+#import "WebKitSystemInterface.h"
Additional includes go down in the second block of includes, sorted alphabetically. The topmost include is the one for the file's own header and stands alone. Please put this down below in sorted order.
+ // Get the clip bounds for the existing context to minimize overdraw
When I said we wanted to respect the clip, I actually meant the entire set of rectangles from NSView, not just the CGContext's clip.
+ // This prevents a potential crash if we don't obtain newWebView.
That's a nice additional fix, but we normally land our bug fixes separately, with a test case for each one.
I'm going to say r=me, but I need to review- this because it doesn't have a ChangeLog -- otherwise the person who commits this would have to write the change log entry for you. Please post a new patch with a change log entry.
And ideally we'd come up with a way to test the lack of null check so someone doesn't come and break it in the future. Since this involves both custom ObjC code and a plug-in it might be challenging, but I think we have tests involving both of those in our DumpRenderTree suite.
I've landed a similar fix for the case where the webview returned is nil, and also a layout test. This was done in revision 19966 in order to fix rdar://problem/5025212 Created attachment 13495 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V5)
Adds clarification to comments regarding clip construction.
Makes the #import order of headers conformant to guidelines.
Removes unrelated - and now redundant - patch to bulletproof loadPluginRequest:
Created attachment 13496 [details]
Change Log for "13495: Allows QD based plugins to draw into offscreen bitmap contexts. (V5)"
Created attachment 13498 [details]
Change Log for "13495: Allows QD based plugins to draw into offscreen bitmap contexts. (V5)"
generated by WebKitTools/Scripts/prepare-ChangeLog --update
Bruce, can you include the change log entry as part of your patch? Use prepare-ChangeLog to give you a template ChangeLog, add your notes to it, then include the ChangeLog file in your diff. This will allow the person that eventually lands your patch to simply use 'patch' (or WebKit's smarter svn-apply) to get your patch into their tree. Comment on attachment 13495 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V5)
This looks good and it is *almost* ready to go!
I looked at what effect this patch will have when the context is *not* a bitmap context, because that's the part that could cause regressions.
And, I'm not trying to be nitpicky but ...
The patch moves the SetPort, PenNormal, ForeColor, BackColor, SetOrigin, and SetPortClipRegion calls inside the "if (forUpdate)" if statement.
The ChangeLog doesn't say why that change is correct. Was it a mistake that those functions were called in the "forUpdate is false" case before? If so, was it simply inefficient to do the extra work, or was there actually a bug?
Another less important question. Is SetGWorld on the saved port from GetPort really the best practice for restoring the old port? Or do we need to do a GetGWorld rather than a GetPort to get the oldPort in the first place?
Otherwise looks great.
review- because the patch should contain the change to the ChangeLog (as Mark said in his comment) but mainly because of the forUpdate change/issue above.
Created attachment 13524 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V6)
Addresses concern about potential regression caused by change in GraphPort setup.
Includes ChangeLog in patch.
Comment on attachment 13524 [details]
Allows QD based plugins to draw into offscreen bitmap contexts. (V6)
Looks good. r=me
Landed in r20042. Thanks Bruce! Please see Bug 13009 for a new issue this patch caused. |