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
Product: WebKit
Classification: Unclassified
Component: Plug-ins
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Kevin Decker
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-18 15:24 PDT by Kevin Decker
Modified: 2010-03-25 12:33 PDT (History)
8 users (show)

See Also:


Attachments
updated design (13.39 KB, patch)
2010-03-18 15:40 PDT, Kevin Decker
sullivan: review-
Details | Formatted Diff | Diff
FrameLoader refactoring patch (5.96 KB, patch)
2010-03-19 10:45 PDT, Kevin Decker
darin: review-
Details | Formatted Diff | Diff
Updated FrameLoader refactoring patch per Darin's feedback. (3.98 KB, patch)
2010-03-19 11:06 PDT, Kevin Decker
no flags 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 PDT, Kevin Decker
no flags 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 PDT, Kevin Decker
no flags 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 PDT, Kevin Decker
no flags 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 PDT, Kevin Decker
sullivan: review+
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 PDT, Kevin Decker
simon.fraser: review-
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 PDT, Kevin Decker
no flags 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 PDT, Kevin Decker
darin: review+
Details | Formatted Diff | Diff
Eliminate WebNullPluginView from Mac OS X WebKit (20.42 KB, patch)
2010-03-22 14:18 PDT, Kevin Decker
no flags Details | Formatted Diff | Diff
51352: Eliminate WebNullPluginView from Mac OS X WebKit (22.80 KB, patch)
2010-03-22 14:57 PDT, Kevin Decker
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Decker 2010-03-18 15:24:01 PDT
Missing plug-ins should be represented by text only, instead of lego block.
Comment 1 Kevin Decker 2010-03-18 15:40:13 PDT
Created attachment 51104 [details]
updated design
Comment 2 John Sullivan 2010-03-19 09:01:10 PDT
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.
Comment 3 John Sullivan 2010-03-19 09:14:19 PDT
Dan Bernstein pointed out that CGFloor and CGRound are not API, so ignore the suggestions to use them.
Comment 4 Darin Adler 2010-03-19 09:16:15 PDT
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 Kevin Decker 2010-03-19 09:54:59 PDT
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 Kevin Decker 2010-03-19 10:45:52 PDT
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.
Comment 7 Darin Adler 2010-03-19 10:51:09 PDT
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!
Comment 8 Kevin Decker 2010-03-19 11:06:56 PDT
Created attachment 51169 [details]
Updated FrameLoader refactoring patch per Darin's feedback.
Comment 9 Brady Eidson 2010-03-19 11:10:58 PDT
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.
Comment 10 Darin Adler 2010-03-19 11:12:04 PDT
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
Comment 11 Kevin Decker 2010-03-20 18:24:36 PDT
Created attachment 51239 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk
Comment 12 Kevin Decker 2010-03-21 10:52:55 PDT
Created attachment 51249 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk
Comment 13 WebKit Review Bot 2010-03-21 10:53:22 PDT
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 Kevin Decker 2010-03-21 10:55:20 PDT
Created attachment 51250 [details]
Added "Missing plug-in" localizable string to the following ports: mac, win, chromium, and gtk 

Now without tabs...
Comment 15 WebKit Review Bot 2010-03-21 10:58:53 PDT
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 Simon Hausmann 2010-03-21 16:06:29 PDT
(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
Comment 17 Kevin Decker 2010-03-22 09:17:23 PDT
(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!!
Comment 18 Kevin Decker 2010-03-22 09:18:31 PDT
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.
Comment 19 WebKit Review Bot 2010-03-22 09:19:24 PDT
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 John Sullivan 2010-03-22 09:21:42 PDT
Is it OK that the qt port uses "Plugin" while the others all use "Plug-in"?
Comment 21 Csaba Osztrogonác 2010-03-22 09:25:52 PDT
Attachment 51292 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1055079
Comment 22 John Sullivan 2010-03-22 09:29:05 PDT
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.
Comment 23 John Sullivan 2010-03-22 09:30:26 PDT
Oops, I meant "change Plugin to Plug-in", obviously.
Comment 24 Kevin Decker 2010-03-22 10:10:13 PDT
Created attachment 51300 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"
Comment 25 Simon Fraser (smfr) 2010-03-22 10:31:28 PDT
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().
Comment 26 Kevin Decker 2010-03-22 11:57:18 PDT
Created attachment 51323 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

Incorporates above feedback from Simon Fraser.
Comment 27 Kevin Decker 2010-03-22 12:09:35 PDT
Created attachment 51326 [details]
Add code to draw a subtle rounded rectangle containing the text "Missing Plug-in"

Added a few more small tweaks.
Comment 28 Darin Adler 2010-03-22 12:20:02 PDT
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
Comment 29 Kevin Decker 2010-03-22 12:35:29 PDT
(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!!
Comment 30 Eric Seidel 2010-03-22 13:20:09 PDT
This broke the leopard compile. :(
Comment 31 Eric Seidel 2010-03-22 13:32:48 PDT
I'll start a rollout in a minute unless I hear otherwise.
Comment 32 Kevin Decker 2010-03-22 13:34:57 PDT
(In reply to comment #31)
> I'll start a rollout in a minute unless I hear otherwise.


working on a fix.
Comment 33 Kevin Decker 2010-03-22 13:49:36 PDT
(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 Kevin Decker 2010-03-22 14:18:17 PDT
Created attachment 51352 [details]
Eliminate WebNullPluginView from Mac OS X WebKit
Comment 35 Kevin Decker 2010-03-22 14:57:17 PDT
Created attachment 51361 [details]
51352: Eliminate WebNullPluginView from Mac OS X WebKit

Fix a find/replace Xcode mistake..
Comment 36 Simon Fraser (smfr) 2010-03-22 15:03:43 PDT
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 @@
> +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 Eric Seidel 2010-03-24 14:30:59 PDT
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.