667201 - Front end changes for Bug 545070: plugin-problem UI shouldn't say "click here".

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Comment 1

[Philip Chee]

Attachment: Patch v1.0

Adapt the changes from Bug 545070

> diff --git a/suite/themes/modern/mozapps/plugins/contentPluginMissing.png b/suite/themes/modern/mozapps/plugins/contentPluginMissing.png
> new file mode 100644

pngcrush -rem gAMA -rem cHRM -rem iCCP -rem sRGB contentPluginMissing.png contentPluginMissing-1.png
Didn't do anything. Experimenting with other switches made the resulting file larger.

optipng -zc1-9 -zm1-9 -zs0-3 -f0-5  -nx contentPluginMissing.png
Complained that the file is already optimized.

Comment 2

[[e-mail]]

Comment on attachment 541937 [details] [diff] [review]
Patch v1.0

>+            var plugin  = aEvent.target;
Nit: two spaces before =

>+            // Hide the in-content UI if it's too big. The crashed plugin handler already does this.
On the other hand, the crashed plugin handler doesn't show the infobar in this case...

>+      <method name="installSinglePlugin">
But you actually end up installing all the missing plugins...

>+            var doc = plugin.ownerDocument;;
Nit: double semicolon

>+            var installStatus = doc.getAnonymousElementByAttribute(plugin, "class", "installStatus");
>+            installStatus.setAttribute("status", "ready");
>+            var iconStatus = doc.getAnonymousElementByAttribute(plugin, "class", "icon");
>+            iconStatus.setAttribute("status", "ready");
So, what's the point of this?

Comment 3

[Philip Chee]

Attachment: Patch v1.1

> [e-mail]      2011-06-27 16:45:55 PDT
> 
> Comment on attachment 541937 [details] [diff] [review] [diff] [details] [review]
> Patch v1.0
> 
>>+            var plugin  = aEvent.target;
> Nit: two spaces before =

Fixed.

>>+            // Hide the in-content UI if it's too big. The crashed plugin handler already does this.
> On the other hand, the crashed plugin handler doesn't show the infobar in this case...

I see code in PluginCrashed() that calls showPluginCrashedNotification() if the in-content UI overflows the plugin container. So I don't know what you want me to do here.

>>+      <method name="installSinglePlugin">
> But you actually end up installing all the missing plugins...

I've changed method name to "installMissingPlugins". Note despite installing all missing plugins, Firefox calls their function installSinglePlugin().

>>+            var doc = plugin.ownerDocument;;
> Nit: double semicolon

Fixed.

>>+            var installStatus = doc.getAnonymousElementByAttribute(plugin, "class", "installStatus");
>>+            installStatus.setAttribute("status", "ready");
>>+            var iconStatus = doc.getAnonymousElementByAttribute(plugin, "class", "icon");
>>+            iconStatus.setAttribute("status", "ready");
> So, what's the point of this?

From https://bugzilla.mozilla.org/show_bug.cgi?id=545070#c6

> * for plugin-not-found <embed>s, set the new "status" attribute to enable
> display of the "click to install" text. [As noted above, this is the first
> step towards giving it the smarts to check for something being available
> _before_ inviting the user to click.]

So somewhere in /toolkit/mozapps/plugins/content/pluginProblemContent.css
we have:

http://hg.mozilla.org/mozilla-central/rev/6c3b228838e5#l7.39

> 7.39 +.installStatus[status="ready"] .msgInstallPlugin {
> 7.40 +  display: block;
> 7.41 +}
> 7.42 +

And in stripe we have:

> 11.14 +:-moz-type-unsupported .icon[status="ready"] {
> 11.15    background-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
> 11.16  }

Comment 4

[[e-mail]]

(In reply to comment #3)
>(In reply to comment #2)
>> (From update of attachment 541937 [details] [diff] [review])
>>>+            // Hide the in-content UI if it's too big. The crashed plugin handler already does this.
>> On the other hand, the crashed plugin handler doesn't show the infobar in this case...
>I see code in PluginCrashed() that calls showPluginCrashedNotification() if
>the in-content UI overflows the plugin container. So I don't know what you
>want me to do here.
Well, I guess we could leave it for now, the plugin crashed notification is slightly different for other reasons anyway.

>>>+      <method name="installSinglePlugin">
>> But you actually end up installing all the missing plugins...
>I've changed method name to "installMissingPlugins". Note despite installing
>all missing plugins, Firefox calls their function installSinglePlugin().
Yeah, well that's Firefox for you...

>>>+            var installStatus = doc.getAnonymousElementByAttribute(plugin, "class", "installStatus");
>>>+            installStatus.setAttribute("status", "ready");
>>>+            var iconStatus = doc.getAnonymousElementByAttribute(plugin, "class", "icon");
>>>+            iconStatus.setAttribute("status", "ready");
>> So, what's the point of this?
>From https://bugzilla.mozilla.org/show_bug.cgi?id=545070#c6
Fair enough.

Comment 5

[[e-mail]]

Comment on attachment 549370 [details] [diff] [review]
Patch v1.1

>+            if (this.isTooSmall(plugin, overlay))
>+                overlay.style.visibility = "hidden";
Nit: wrong indentation

>+      <method name="installMissingPlugins">
>         <parameter name="aEvent"/>
Nit: don't need aEvent any more, addLinkClickCallback handles it.

>+            this.addLinkClickCallback(installLink, installMissingPlugins, event);
As above.

>+            callback: this.installMissingPlugins.bind(this, event)
As above.

>diff --git a/suite/themes/modern/mozapps/plugins/contentPluginMissing.png b/suite/themes/modern/mozapps/plugins/contentPluginMissing.png
>new file mode 100644
Nit: needs to be run through optipng

Comment 6

[Philip Chee]

Attachment: Patch v1.2 for checkin. r=Neil

> Comment on attachment 549370 [details] [diff] [review] [diff] [details] [review]
> Patch v1.1
> 
>>+            if (this.isTooSmall(plugin, overlay))
>>+                overlay.style.visibility = "hidden";
> Nit: wrong indentation
Fixed.

>>+      <method name="installMissingPlugins">
>>         <parameter name="aEvent"/>
> Nit: don't need aEvent any more, addLinkClickCallback handles it.
Fixed.


>>+            this.addLinkClickCallback(installLink, installMissingPlugins, event);
> As above.
Fixed.

>>+            callback: this.installMissingPlugins.bind(this, event)
> As above.
Fixed.

>>diff --git a/suite/themes/modern/mozapps/plugins/contentPluginMissing.png b/suite/themes/modern/mozapps/plugins/contentPluginMissing.png
>>new file mode 100644
> Nit: needs to be run through optipng
Already done in Comment 1 !!

Comment 7

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/b0be62c945b9

Comment 8

[Philip Chee]

Comment on attachment 549558 [details] [diff] [review]
Patch v1.2 for checkin. r=Neil

Target Milestone for the mozilla-central Bug 545070 is mozilla6.
Requesting approval for comm-aurora and comm-beta. No L10N changes.

Comment 9

[Ian Neal]

(In reply to comment #7)
> Pushed to comm-central
> http://hg.mozilla.org/comm-central/rev/b0be62c945b9

http://hg.mozilla.org/comm-central/rev/b0be62c945b9#l1.167
You might want to sort the unneeded event argument.

Comment 10

[Ian Neal]

Comment on attachment 549558 [details] [diff] [review]
Patch v1.2 for checkin. r=Neil

a=me for comm-aurora/beta with extraneous event argument removed.

Comment 11

[Philip Chee]

> http://hg.mozilla.org/comm-central/rev/b0be62c945b9#l1.167
> You might want to sort the unneeded event argument.

Puushed a fix r=typo
http://hg.mozilla.org/comm-central/rev/423a04323c61

Comment 12

[Philip Chee]

Pushed:
http://hg.mozilla.org/releases/comm-aurora/rev/e8172bb6b4f1
http://hg.mozilla.org/releases/comm-beta/rev/2e92298d561c

Comment 13

[Serge Gautherie (:sgautherie)]

Test will be updated by bug 724499.