[webkit-reviews] review granted: [Bug 37111] Draw replacement text when plug-in host crashes : [Attachment 52566] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 37111: Draw replacement text when plug-in host crashes
https://bugs.webkit.org/show_bug.cgi?id=37111

Attachment 52566: proposed patch
https://bugs.webkit.org/attachment.cgi?id=52566&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>      , m_showsMissingPluginIndicator(false)
> +    , m_showsCrashedPluginIndicator(false)

An enum would be better than two booleans. We never want to show both.

>      void setShowsMissingPluginIndicator(bool showsMissingPluginIndicator) {
m_showsMissingPluginIndicator = showsMissingPluginIndicator; }
>      bool showsMissingPluginIndicator() const { return
m_showsMissingPluginIndicator; }
> +    void setShowsCrashedPluginIndicator(bool showsCrashedPluginIndicator) {
m_showsCrashedPluginIndicator = showsCrashedPluginIndicator; }
> +    bool showsCrashedPluginIndicator() const { return
m_showsCrashedPluginIndicator; }

I know that someone (Simon) suggested adding the getter to Kevin's original
patch, but I don't think we need the getters. Nor do I think we need a way to
set either value to false.

> +2010-04-05  Alexey Proskuryakov  <ap at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=37111
> +	   <rdar://problem/7790327> Draw replacement text when plug-in host
crashes
> +
> +	   * English.lproj/Localizable.strings: Added a string for plug-in
failure.
> +
> +2010-04-05  Alexey Proskuryakov  <ap at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Need a short description and bug URL (OOPS!)
> +
> +	   * English.lproj/Localizable.strings:
> +

Double change log.

I think this will break the build on platforms other than Mac because the
crashedPluginText function will be called, but not defined.

An interesting way to sidestep that would be to have the API take a string and
have WebHostedNetscapePluginView pass the string in. Then you'd use a String
instead of an enum in RenderEmbeddedObject.

r=me as long as you make it build on non-Mac platforms