Bug 36328 - Missing plug-ins should be represented by text only, instead of lego block
: Missing plug-ins should be represented by text only, instead of lego block
Status: CLOSED FIXED
: WebKit
Plug-ins
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-03-18 15:24 PST by
Modified: 2010-03-25 12:33 PST (History)


Attachments
updated design (13.39 KB, patch)
2010-03-18 15:40 PST, Kevin Decker
sullivan: review-
Review Patch | Details | Formatted Diff | Diff
FrameLoader refactoring patch (5.96 KB, patch)
2010-03-19 10:45 PST, Kevin Decker
darin: review-
Review Patch | Details | Formatted Diff | Diff
Updated FrameLoader refactoring patch per Darin's feedback. (3.98 KB, patch)
2010-03-19 11:06 PST, Kevin Decker
no flags Review Patch | Details | Formatted Diff | Diff
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk (7.35 KB, patch)
2010-03-20 18:24 PST, Kevin Decker
no flags Review Patch | Details | Formatted Diff | Diff
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk (7.39 KB, patch)
2010-03-21 10:52 PST, Kevin Decker
no flags Review Patch | Details | Formatted Diff | Diff
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk (7.39 KB, patch)
2010-03-21 10:55 PST, Kevin Decker
no flags Review Patch | Details | Formatted Diff | Diff
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, gtk, and qt (8.04 KB, patch)
2010-03-22 09:18 PST, Kevin Decker
sullivan: review+
Review Patch | Details | Formatted Diff | Diff
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in" (5.41 KB, patch)
2010-03-22 10:10 PST, Kevin Decker
simon.fraser: review-
Review Patch | Details | Formatted Diff | Diff
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in" (7.24 KB, patch)
2010-03-22 11:57 PST, Kevin Decker
no flags Review Patch | Details | Formatted Diff | Diff
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in" (7.56 KB, patch)
2010-03-22 12:09 PST, Kevin Decker
darin: review+
Review Patch | Details | Formatted Diff | Diff
Eliminate WebNullPluginView from Mac OS X WebKit (20.42 KB, patch)
2010-03-22 14:18 PST, Kevin Decker
no flags Review Patch | Details | Formatted Diff | Diff
51352: Eliminate WebNullPluginView from Mac OS X WebKit (22.80 KB, patch)
2010-03-22 14:57 PST, Kevin Decker
simon.fraser: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-18 15:24:01 PST
Missing plug-ins should be represented by text only, instead of lego block.
------- Comment #1 From 2010-03-18 15:40:13 PST -------
Created an attachment (id=51104) [details]
updated design
------- Comment #2 From 2010-03-19 09:01:10 PST -------
(From update of attachment 51104 [details])
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.
------- Comment #3 From 2010-03-19 09:14:19 PST -------
Dan Bernstein pointed out that CGFloor and CGRound are not API, so ignore the suggestions to use them.
------- Comment #4 From 2010-03-19 09:16:15 PST -------
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.
------- Comment #5 From 2010-03-19 09:54:59 PST -------
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..
------- Comment #6 From 2010-03-19 10:45:52 PST -------
Created an attachment (id=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.
------- Comment #7 From 2010-03-19 10:51:09 PST -------
(From update of attachment 51167 [details])
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!
------- Comment #8 From 2010-03-19 11:06:56 PST -------
Created an attachment (id=51169) [details]
Updated FrameLoader refactoring patch per Darin's feedback.
------- Comment #9 From 2010-03-19 11:10:58 PST -------
(From update of attachment 51169 [details])
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.
------- Comment #10 From 2010-03-19 11:12:04 PST -------
(From update of attachment 51169 [details])
> +#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
------- Comment #11 From 2010-03-20 18:24:36 PST -------
Created an attachment (id=51239) [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk
------- Comment #12 From 2010-03-21 10:52:55 PST -------
Created an attachment (id=51249) [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk
------- Comment #13 From 2010-03-21 10:53:22 PST -------
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.
------- Comment #14 From 2010-03-21 10:55:20 PST -------
Created an attachment (id=51250) [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk 

Now without tabs...
------- Comment #15 From 2010-03-21 10:58:53 PST -------
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.
------- Comment #16 From 2010-03-21 16:06:29 PST -------
(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
------- Comment #17 From 2010-03-22 09:17:23 PST -------
(In reply to comment #16)
> (In reply to comment #14)
> > Created an attachment (id=51250) [details] [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!!
------- Comment #18 From 2010-03-22 09:18:31 PST -------
Created an attachment (id=51292) [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, gtk, and qt

Now incorporates Simon's above change.
------- Comment #19 From 2010-03-22 09:19:24 PST -------
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.
------- Comment #20 From 2010-03-22 09:21:42 PST -------
Is it OK that the qt port uses "Plugin" while the others all use "Plug-in"?
------- Comment #21 From 2010-03-22 09:25:52 PST -------
Attachment 51292 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1055079
------- Comment #22 From 2010-03-22 09:29:05 PST -------
(From update of attachment 51292 [details])
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.
------- Comment #23 From 2010-03-22 09:30:26 PST -------
Oops, I meant "change Plugin to Plug-in", obviously.
------- Comment #24 From 2010-03-22 10:10:13 PST -------
Created an attachment (id=51300) [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"
------- Comment #25 From 2010-03-22 10:31:28 PST -------
(From update of attachment 51300 [details])
> 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().
------- Comment #26 From 2010-03-22 11:57:18 PST -------
Created an attachment (id=51323) [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

Incorporates above feedback from Simon Fraser.
------- Comment #27 From 2010-03-22 12:09:35 PST -------
Created an attachment (id=51326) [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

Added a few more small tweaks.
------- Comment #28 From 2010-03-22 12:20:02 PST -------
(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);

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
------- Comment #29 From 2010-03-22 12:35:29 PST -------
(In reply to comment #28)
> (From update of attachment 51326 [details] [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!!
------- Comment #30 From 2010-03-22 13:20:09 PST -------
This broke the leopard compile. :(
------- Comment #31 From 2010-03-22 13:32:48 PST -------
I'll start a rollout in a minute unless I hear otherwise.
------- Comment #32 From 2010-03-22 13:34:57 PST -------
(In reply to comment #31)
> I'll start a rollout in a minute unless I hear otherwise.


working on a fix.
------- Comment #33 From 2010-03-22 13:49:36 PST -------
(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
------- Comment #34 From 2010-03-22 14:18:17 PST -------
Created an attachment (id=51352) [details]
Eliminate WebNullPluginView from Mac OS X WebKit
------- Comment #35 From 2010-03-22 14:57:17 PST -------
Created an attachment (id=51361) [details]
51352: Eliminate WebNullPluginView from Mac OS X WebKit

Fix a find/replace Xcode mistake..
------- Comment #36 From 2010-03-22 15:03:43 PST -------
(From update of attachment 51361 [details])
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 56360)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> +2010-03-22  Kevin Decker  <kdecker@apple.com>
> +
> +        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
------- Comment #37 From 2010-03-24 14:30:59 PST -------
(From update of attachment 51169 [details])
Cleared Brady Eidson's review+ from obsolete attachment 51169 [details] so that this bug does not appear in http://webkit.org/pending-commit.