Bug 36328 – Missing plug-ins should be represented by text only, instead of lego block

• Component: Plug-ins
• Status: CLOSED FIXED

[Kevin Decker]

Missing plug-ins should be represented by text only, instead of lego block.

[Kevin Decker]

Created attachment 51104 [details]
updated design

[John Sullivan]

Comment on attachment 51104 [details]
updated design

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.

[John Sullivan]

Dan Bernstein pointed out that CGFloor and CGRound are not API, so ignore the suggestions to use them.

[Darin Adler]

I suggest we get rid of the WebNullPluginView class entirely and put the responsibility for this into platform-independent code in WebCore.

One way to do that would be to make a MissingPluginIndicator class in cross-platform code, derived from Widget, to draw the missing plug-in text. This would be a Widget that is not backed by a platform-specific object. Then WebFrameLoaderClient::createPlugin could create this object and not involve an NSView at all.

Another, probably better, way to do it would be to change RenderEmbeddedObject to give it the capability of drawing the missing plug-in text. To do this we would change the argument of FrameLoader::requestObject to be a RenderEmbeddedObject* instead of a RenderPart* and possibly rename it to requestEmbeddedObject. Then do the same thing with the first argument to FrameLoader::loadPlugin. Then inside FrameLoader::loadPlugin we would call RenderEmbeddedObject::setShowsMissingPluginIndicator and add the code to draw the missing plug-in message to the RenderEmbeddedObject class.

The only issue is that our design for missing plug-ins for Safari may not be exactly what other platforms want, so we might want to put #ifdefs or use a theme for the missing plug-in indicator eventually over time, but that’s not needed for the first cut at this.

[Kevin Decker]

These are helpful suggestions thanks. The second solution seems like a better fit here, since widgets are responsible for drawing themselves. I'll start working a couple small refactoring patches..

[Kevin Decker]

Created attachment 51167 [details]
FrameLoader refactoring patch

First step toward eliminating the WebKit WebNullPluginView class. The responsibility for this will soon be in platform-independent code in WebCore. And our plan is to change RenderEmbeddedObject and give it the capability of drawing the missing plug-in text.

[Darin Adler]

Comment on attachment 51167 [details]
FrameLoader refactoring patch

Thanks. I think this is a good approach.

> -bool FrameLoader::requestObject(RenderPart* renderer, const String& url, const AtomicString& frameName,
> +bool FrameLoader::requestObject(RenderEmbeddedObject* requestEmbeddedObject, const String& url, const AtomicString& frameName,

The type change here seems good, but I don't understand why the argument is now named "request embedded object". That sounds like a sentence, with "request" being a verb. I think that renderer was actually a fine name for the argument. Or we could use "container" or "object".

The function name "requestObject", is not good, but you didn't change that.

> +class RenderEmbeddedObject;
>  class RenderPart;

These functions were the only thing using RenderPart, so I suggest you remove the forward declaration of RenderPart too.

> -    bool requestObject(RenderPart* frame, const String& url, const AtomicString& frameName,
> +    bool requestObject(RenderEmbeddedObject*, const String& url, const AtomicString& frameName,
>          const String& serviceType, const Vector<String>& paramNames, const Vector<String>& paramValues);

> -    bool loadPlugin(RenderPart*, const KURL&, const String& mimeType,
> +    bool loadPlugin(RenderEmbeddedObject*, const KURL&, const String& mimeType,
>      const Vector<String>& paramNames, const Vector<String>& paramValues, bool useFallback);

Indenting is wrong here. You should fix that.

Since the purpose of the patch is primarily renaming and I didn't like the name change, I'll say review-, but lets keep going!

[Kevin Decker]

Created attachment 51169 [details]
Updated FrameLoader refactoring patch per Darin's feedback.

[Brady Eidson]

Comment on attachment 51169 [details]
Updated FrameLoader refactoring patch per Darin's feedback.

I know I told you to do that loadPlugin indenting on IRC, but I lied - it should match the requestObject indenting just above.

r=me with that change.

[Darin Adler]

Comment on attachment 51169 [details]
Updated FrameLoader refactoring patch per Darin's feedback.

> +#include "RenderEmbeddedObject.h"
>  #include "RenderPart.h"
>  #include "RenderView.h"
>  #include "RenderWidget.h"

No need for the RenderPart.h or RenderWidget.h includes, since RenderEmbeddedObject.h includes both of them.

> -    bool loadPlugin(RenderPart*, const KURL&, const String& mimeType,
> -    const Vector<String>& paramNames, const Vector<String>& paramValues, bool useFallback);
> +    bool loadPlugin(RenderEmbeddedObject*, const KURL&, const String& mimeType,
> +                    const Vector<String>& paramNames, const Vector<String>& paramValues, bool useFallback);

It's supposed to be indented one tab stop (4 spaces), not lined up with the parenthesis.

r=me

[Kevin Decker]

Created attachment 51239 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk

[Kevin Decker]

Created attachment 51249 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk

[WebKit Review Bot]

Attachment 51249 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kevin Decker]

Created attachment 51250 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk 

Now without tabs...

[WebKit Review Bot]

Attachment 51250 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

(In reply to comment #14)
> Created an attachment (id=51250) [details]
> Added "Missing plug-in" localizable string to the following ports: mac, win,
> chromium, and gtk 

Can you add this hunk for the Qt port, too? :)

--- a/WebCore/platform/qt/Localizations.cpp
+++ b/WebCore/platform/qt/Localizations.cpp
@@ -524,5 +524,10 @@ String validationMessageStepMismatchText()
     return String();
 }
 
+String missingPluginText()
+{
+    return QCoreApplication::translate("QWebPage", "Missing Plugin", "Label text to be used when a plugin is missing");
+}
+
 }
 // vim: ts=4 sw=4 et

[Kevin Decker]

(In reply to comment #16)
> (In reply to comment #14)
> > Created an attachment (id=51250) [details] [details]
> > Added "Missing plug-in" localizable string to the following ports: mac, win,
> > chromium, and gtk 
> 
> Can you add this hunk for the Qt port, too? :)
> 
> --- a/WebCore/platform/qt/Localizations.cpp
> +++ b/WebCore/platform/qt/Localizations.cpp
> @@ -524,5 +524,10 @@ String validationMessageStepMismatchText()
>      return String();
>  }
> 
> +String missingPluginText()
> +{
> +    return QCoreApplication::translate("QWebPage", "Missing Plugin", "Label
> text to be used when a plugin is missing");
> +}
> +
>  }
>  // vim: ts=4 sw=4 et

Thanks a lot, Simon!!

[Kevin Decker]

Created attachment 51292 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, gtk, and qt

Now incorporates Simon's above change.

[WebKit Review Bot]

Attachment 51292 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/qt/Localizations.cpp:356:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
WebCore/platform/qt/Localizations.cpp:357:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Sullivan]

Is it OK that the qt port uses "Plugin" while the others all use "Plug-in"?

[Csaba Osztrogonác]

Attachment 51292 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1055079

[John Sullivan]

Comment on attachment 51292 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, gtk, and qt

r+ if you fix the qt build breakage and style warning by not splitting the string across multiple lines, and change "Plug-in" to "Plugin" for the qt build also.

[John Sullivan]

Oops, I meant "change Plugin to Plug-in", obviously.

[Kevin Decker]

Created attachment 51300 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

[Simon Fraser (smfr)]

Comment on attachment 51300 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

> Index: rendering/RenderEmbeddedObject.h
> ===================================================================
> --- rendering/RenderEmbeddedObject.h	(revision 56339)
> +++ rendering/RenderEmbeddedObject.h	(working copy)
> @@ -34,12 +34,15 @@
>      virtual ~RenderEmbeddedObject();
>  
>      void updateWidget(bool onlyCreateNonNetscapePlugins);
> +    void setShowsMissingPluginIndicator() { m_showsMissingPluginIndicator = true; }

This needs an associated getter (which should be const).

> +    void paint(PaintInfo& paintInfo, int tx, int ty);

I think you should override paintReplaced.

>  private:
> +    bool m_showsMissingPluginIndicator;

Please put this after the private methods.

r- because it should override paintReplaced().

[Kevin Decker]

Created attachment 51323 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

Incorporates above feedback from Simon Fraser.

[Kevin Decker]

Created attachment 51326 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

Added a few more small tweaks.

[Darin Adler]

Comment on attachment 51326 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

> +    virtual void paint(PaintInfo& paintInfo, int tx, int ty);

Should omit the argument name paintInfo here.

> +    tx += borderLeft() + paddingLeft();
> +    ty += borderTop() + paddingTop();
> +    FloatRect pluginRect = FloatRect(tx, ty, contentWidth(), contentHeight());

It'd be cleaner not to modify the passed-in tx and ty here. I suggest writing this like this:

    FloatRect pluginRect = contentBoxRect();
    pluginRect.move(tx, ty);

This accomplishes the same thing and reads a bit cleaner to me.

> +    fontDescription.setFontSmoothing(Antialiased);

I don't understand why the setFontSmoothing call is needed here.

> +    FloatRect missingPluginRect = FloatRect();

The "= FloatRect()" is not needed. That's the same thing you'd get if you didn't explicitly initialize it.

> +    FloatPoint labelPoint = FloatPoint();

Same here.

> +    labelPoint.setX(roundf(missingPluginRect.location().x() + (missingPluginRect.size().width() - textWidth) / 2));
> +    labelPoint.setY(floorf(missingPluginRect.location().y()+ (missingPluginRect.size().height() - font.height()) / 2 + font.ascent()));

I think this would read better if you just use the constructor:

    FloatPoint labelPoint(roundf(missingPluginRect.location().x() + (missingPluginRect.size().width() - textWidth) / 2)),
        floorf(missingPluginRect.location().y()+ (missingPluginRect.size().height() - font.height()) / 2 + font.ascent()));

I don't understand the use of floorf instead of roundf for the Y coordinate.

r=me

[Kevin Decker]

(In reply to comment #28)
> (From update of attachment 51326 [details])
> > +    virtual void paint(PaintInfo& paintInfo, int tx, int ty);
> 
> Should omit the argument name paintInfo here.
> 
> > +    tx += borderLeft() + paddingLeft();
> > +    ty += borderTop() + paddingTop();
> > +    FloatRect pluginRect = FloatRect(tx, ty, contentWidth(), contentHeight());
> 
> It'd be cleaner not to modify the passed-in tx and ty here. I suggest writing
> this like this:
> 
>     FloatRect pluginRect = contentBoxRect();
>     pluginRect.move(tx, ty);

I like that much better too, thanks!
> 
> This accomplishes the same thing and reads a bit cleaner to me.
> 
> > +    fontDescription.setFontSmoothing(Antialiased);
> 

At the time I wrote this code I thought I needed this. Testing before/after shows no visual difference. Removed.

> > +    FloatRect missingPluginRect = FloatRect();
> 
> The "= FloatRect()" is not needed. That's the same thing you'd get if you
> didn't explicitly initialize it.

Done.

> 
> > +    FloatPoint labelPoint = FloatPoint();
> 

Same here!
> 
> > +    labelPoint.setX(roundf(missingPluginRect.location().x() + (missingPluginRect.size().width() - textWidth) / 2));
> > +    labelPoint.setY(floorf(missingPluginRect.location().y()+ (missingPluginRect.size().height() - font.height()) / 2 + font.ascent()));
> 
> I think this would read better if you just use the constructor:
> 
>     FloatPoint labelPoint(roundf(missingPluginRect.location().x() +
> (missingPluginRect.size().width() - textWidth) / 2)),
>         floorf(missingPluginRect.location().y()+
> (missingPluginRect.size().height() - font.height()) / 2 + font.ascent()));

It wasn't as unwieldy as I thought it would be. Done!

> 
> I don't understand the use of floorf instead of roundf for the Y coordinate.

Fixed. They both should be the same.

Thanks for the awesome suggestions!!

[Eric Seidel (no email)]

This broke the leopard compile. :(

[Eric Seidel (no email)]

I'll start a rollout in a minute unless I hear otherwise.

[Kevin Decker]

(In reply to comment #31)
> I'll start a rollout in a minute unless I hear otherwise.


working on a fix.

[Kevin Decker]

(In reply to comment #32)
> (In reply to comment #31)
> > I'll start a rollout in a minute unless I hear otherwise.
> 
> 
> working on a fix.

(In reply to comment #32)
> (In reply to comment #31)
> > I'll start a rollout in a minute unless I hear otherwise.
> 
> 
> working on a fix.

Fixed in http://trac.webkit.org/changeset/56357

[Kevin Decker]

Created attachment 51352 [details]
Eliminate WebNullPluginView from Mac OS X WebKit

[Kevin Decker]

Created attachment 51361 [details]
51352: Eliminate WebNullPluginView from Mac OS X WebKit

Fix a find/replace Xcode mistake..

[Simon Fraser (smfr)]

Comment on attachment 51361 [details]
51352: Eliminate WebNullPluginView from Mac OS X WebKit

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 56360)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> [phone]  Kevin Decker  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=36328
> +
> +        * rendering/RenderEmbeddedObject.cpp:
> +        (WebCore::RenderEmbeddedObject::RenderEmbeddedObject): Fix a find/repalce mistake from my earlier patrch.

2 typos on that line.

> +        (WebCore::RenderEmbeddedObject::paint):Fix a find/repalce mistake from my earlier patrch.

Ditto.

r=me

[Eric Seidel (no email)]

Comment on attachment 51169 [details]
Updated FrameLoader refactoring patch per Darin's feedback.

Cleared Brady Eidson's review+ from obsolete attachment 51169 [details] so that this bug does not appear in http://webkit.org/pending-commit.