[webkit-reviews] review denied: [Bug 12515] Plug-ins that draw through the Quickdraw interface fail in a CGBitmapContex. : [Attachment 12836] This corrects 12515: Plug-ins that draw through the Quickdraw interface fail in a CGBitmapContex.

[bugzilla-request-daemon at macosforge.org]

Sam Weinig <sam at webkit.org> has denied Sam Weinig <sam at webkit.org>'s request
for review:
Bug 12515: Plug-ins that draw through the Quickdraw interface fail in a
CGBitmapContex.
http://bugs.webkit.org/show_bug.cgi?id=12515

Attachment 12836: This corrects 12515:	Plug-ins that draw through the
Quickdraw interface fail in a CGBitmapContex.
http://bugs.webkit.org/attachment.cgi?id=12836&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
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-.