Last Comment Bug 667201 - Front end changes for Bug 545070: plugin-problem UI shouldn't say "click here".
: Front end changes for Bug 545070: plugin-problem UI shouldn't say "click here".
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.5
Assigned To: Philip Chee
:
Mentors:
Depends on: 545070
Blocks: 724499
  Show dependency treegraph
 
Reported: 2011-06-25 08:48 PDT by Philip Chee
Modified: 2012-02-06 12:05 PST (History)
2 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Patch v1.0 (14.45 KB, patch)
2011-06-25 08:56 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.1 (14.46 KB, patch)
2011-07-29 07:41 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review
Patch v1.2 for checkin. r=Neil (14.51 KB, patch)
2011-07-30 02:44 PDT, Philip Chee
philip.chee: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Philip Chee 2011-06-25 08:48:49 PDT

    
Comment 1 Philip Chee 2011-06-25 08:56:15 PDT
Created attachment 541937 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-06-27 16:45:55 PDT
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 2011-07-29 07:41:42 PDT
Created attachment 549370 [details] [diff] [review]
Patch v1.1

> neil@parkwaycc.co.uk      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 neil@parkwaycc.co.uk 2011-07-29 08:03:37 PDT
(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 neil@parkwaycc.co.uk 2011-07-29 16:57:30 PDT
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 2011-07-30 02:44:37 PDT
Created attachment 549558 [details] [diff] [review]
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 2011-07-30 02:48:15 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/b0be62c945b9
Comment 8 Philip Chee 2011-07-30 02:51:04 PDT
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 2011-07-30 03:02:24 PDT
(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 2011-07-30 03:09:28 PDT
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 2011-07-30 09:54:29 PDT
> 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 13 Serge Gautherie (:sgautherie) 2012-02-06 12:05:08 PST
Test will be updated by bug 724499.

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