[webkit-reviews] review denied: [Bug 36328] Missing plug-ins should be represented by text only, instead of lego block : [Attachment 51104] updated design

[bugzilla-daemon at webkit.org]

John Sullivan <sullivan at apple.com> has denied Kevin Decker
<kdecker at apple.com>'s request for review:
Bug 36328: Missing plug-ins should be represented by text only, instead of lego
block
https://bugs.webkit.org/show_bug.cgi?id=36328

Attachment 51104: updated design
https://bugs.webkit.org/attachment.cgi?id=51104&action=review

------- Additional Comments from John Sullivan <sullivan at apple.com>
It would have reduced the size of this patch and therefore made it easier to
review if the renamings (error->_error, element->_element) had been done in a
separate patch.

> +- (void)drawRect:(NSRect)rect
> +{
> +    BOOL savedShouldUseFontSmoothing = [WebView _shouldUseFontSmoothing];
> +    // Turn off font smoothing so text draws correctly on a transparent
background
> +    [WebView _setShouldUseFontSmoothing:NO];
> +
> +    NSString *text = UI_STRING("Missing Plug-in", "Missing Plug-in label");
> +    NSFont *font = [NSFont boldSystemFontOfSize:TextFontSize];
> +    CGFloat textWidth = [text _web_widthWithFont:font];
> +    CGRect missingPluginRect;
> +    missingPluginRect.size = CGSizeMake(textWidth +
RoundedRectLeftRightTextMargin * 2, RoundedRectHeight);
> +    missingPluginRect.origin = CGPointMake(NSWidth(rect) / 2 -
NSWidth(missingPluginRect) /2, NSHeight(rect) / 2 - NSHeight(missingPluginRect)
/ 2);
> +    // Line up to pixel boundaries
> +    missingPluginRect = NSIntegralRect(missingPluginRect);

> +    CGFloat textX = floor(NSWidth(missingPluginRect) / 2) - (textWidth / 2)
+ NSMinX(missingPluginRect);

This should use CGFloor.

> +    CGFloat drawingFontHeight = [font ascender] - [font descender];
> +    CGFloat textY = NSMinY(missingPluginRect) + drawingFontHeight / 2;
> +    textY = MAX(roundf(textY) - 1, 0);

This should use CGRound.

This calculation of textY doesn't seem to be correct. If the goal is to
visually center arbitrary text in an arbitrary font, then the calculation needs
to be essentially (ignoring flipped-ness and assuming ascender and descender
are both positive values):

baseline = bottom of text rect + (height of text rect - (ascender + descender))
/ 2 + descender.

> +    
> +    // Draw the rounded rect, white fill with 20% opacity

I don't think commenting on the opacity value here is helpful, since it's using
a symbolic value that just happens to be 20% at this point.

> +static void addRelativeCurveToPoint(CGMutablePathRef path, CGFloat ctrlx1,
CGFloat ctrly1, CGFloat ctrlx2, CGFloat ctrly2, CGFloat endx, CGFloat endy)
> +{
> +    CGPoint currentPt = CGPathGetCurrentPoint(path);
> +    ctrlx1 += currentPt.x;
> +    ctrly1 += currentPt.y;
> +    ctrlx2 += currentPt.x;
> +    ctrly2 += currentPt.y;
> +    endx += currentPt.x;
> +    endy += currentPt.y;
> +    CGPathAddCurveToPoint(path, 0, ctrlx1, ctrly1, ctrlx2, ctrly2, endx,
endy);
> +}
> +
> +static RetainPtr<CGMutablePathRef> createRoundedRect(CGRect rect, CGFloat
radius)
> +{
> +    CGFloat inset = 1;
> +    CGFloat left = rect.origin.x;
> +    CGFloat right = left + rect.size.width;
> +    CGFloat bottom = rect.origin.y;
> +    CGFloat top = bottom + rect.size.height;
> +    
> +    RetainPtr<CGMutablePathRef> path(AdoptCF, CGPathCreateMutable());
> +    
> +    CGPathMoveToPoint(path.get(), 0, left + inset, top - (radius + inset));
> +    
> +    addRelativeCurveToPoint(path.get(), 0, KappaCurve * radius, radius -
(KappaCurve * radius), radius, radius, radius);
> +    
> +    CGPathAddLineToPoint(path.get(), 0, right - (radius + inset), top -
inset);
> +    addRelativeCurveToPoint(path.get(), KappaCurve * radius, 0, radius,
-radius + (KappaCurve * radius), radius, -radius);
> +    
> +    CGPathAddLineToPoint(path.get(), 0, right - inset, bottom + (radius +
inset));
> +    addRelativeCurveToPoint(path.get(), 0, -(KappaCurve * radius), -radius +
(KappaCurve * radius), -radius, -radius, -radius);
> +    
> +    CGPathAddLineToPoint(path.get(), 0, left + radius + inset, bottom +
inset);
> +    addRelativeCurveToPoint(path.get(), KappaCurve * -radius, 0, -radius,
radius - (KappaCurve * radius), -radius, radius);
> +    
> +    CGPathCloseSubpath(path.get());
> +    return path;

Is there a cross-platform place where this function and its helper could be
put?


r- for the apparently incorrect text placement.