Status

()

Firefox
General
P1
normal
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Doron Rosenberg (IBM))

Tracking

({fixed-aviary1.0})

unspecified
x86
All
fixed-aviary1.0
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0PR +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [have patch] ready to land)

Attachments

(9 attachments, 9 obsolete attachments)

34.81 KB, patch
Ben Goodger (use ben at mozilla dot org for email)
: review+
Ben Goodger (use ben at mozilla dot org for email)
: approval-aviary+
Details | Diff | Splinter Review
43.61 KB, patch
Details | Diff | Splinter Review
15.81 KB, patch
Ben Goodger (use ben at mozilla dot org for email)
: superreview+
Ben Goodger (use ben at mozilla dot org for email)
: approval-aviary+
Details | Diff | Splinter Review
66.03 KB, image/jpeg
Details
66.03 KB, image/jpeg
Details
2.93 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
4.75 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.99 KB, patch
Details | Diff | Splinter Review
6.58 KB, patch
jst
: review+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
This bug is for the missing plugin installer work that is being done.  I'll post
the current code tomorrow for the aviary branch.

When is the UI freeze for 1.0?
(Assignee)

Comment 1

13 years ago
Created attachment 154391 [details] [diff] [review]
current code

The main problem is that onwizardfinish doesn't get called, need to figure why.
(Assignee)

Comment 2

13 years ago
Screenshots of the flow:
http://www.nexgenmedia.net/ff/screenshots/

Comment 3

13 years ago
Will this go with the webservice on UMO?
(Assignee)

Comment 4

13 years ago
I'm working on the client part - I do have the server part (Apache Axis)
working, but its currently doing if/else checks for return values.  Its up to
the foundation to decide what sort of infrastructure will be used in the end (I
assume mysql db with the relavant information, as is planned for the extension
updating code).
(Assignee)

Comment 5

13 years ago
And before I forget, this patch will only work with a patch jst is working on
for sending events when no plugins are found, which I believe hasn't gone in yet.

Comment 6

13 years ago
The database structure we use for themes and extensions should pretty much hold
for plugins.  That's bug 247814.  Bug 229590 is about whatever licensing
mozilla.org needs to accomplish.  And of course not all plugins install
smoothly, thus the current need for PluginDoc http://plugindoc.mozdev.org/
Wow! Doron, this looks fantastic! Thanks for doing the heavy lifting here!

I'll try to get a cleaned up patch for what I've been working on for this up
tomorrow.
Personally I'd hold 1.0 for this if it were close (hence nomination '?' flag
set).  I think one of the most popular questions are plugin related (how do I
get Quicktime, Acrobat, Flash).  

Would be best if it uses u.m.o.  That way Mozilla Foundation can keep systems
unified.

This is very sexy Doron.  I really really can't wait to take it for a spin.  Of
people I recommend firefox to... this is normally what they ask for help with.
Flags: blocking-aviary1.0?
(Assignee)

Comment 9

13 years ago
The main problem is getting the agreements to create the XPIs.  The actuall
client code is my priority this week.
Here's another thought:

Shouldn't the UI be somewhat unified and consistant?

For themes and extensions we share a pretty similar UI.

Ideally, plugins would be just another subset (perhaps we can call them
collectively 'components', or 'modules').

If it goes into u.m.o we could push updates (and take care of things like a
quicktime, or flash security bug), which would address some concerns expressed here:
http://news.netcraft.com/archives/2004/07/26/will_firefox_repeat_netscapes_mistakes.html


Just some thoughts.

Then collectively they could be updated, installed, removed from a common
interface.  The upgrade part is what I'm most interested in.

Comment 11

13 years ago
I agree that the UI should be common.  In the future, we could disable a plugin
or access its options from there.  For now I'll be happy with install and
uninstall.  
I'd like update as part of the default (for reasons mentioned above in comment #10).

This whole thing should block 1.0f if it is possible.  We should have the method
in place so that plugins can come online if/when licensing works out.  There's a
big advantage here regarding security.  Mozilla's getting lots of press as
secure.  We should have the same emphasis as the media (well... continue to have).

Even if the xpi's aren't available... perhaps we can have the browser prompt:

"a newer version of the plugin 'XXPLUGIN NAMEXX' has been detected, please visit:
'http://www.developer.com' and update..."

them u.m.o could perhaps just store the plugin's homepage and pass that along.

So no matter what, we can provide update notification.
(Assignee)

Comment 13

13 years ago
Created attachment 154514 [details] [diff] [review]
Cleanup and bug fixes - needs OS detection still.
Attachment #154391 - Attachment is obsolete: true
(Assignee)

Comment 14

13 years ago
Some stuff I still want to do:
  - add a link to plugindoc if certain plugins were not found or if a plugin
installation failed.
  - OS detection needs to be done.
  - more error handling
  - probably change the progress meters to undetermined first
(Assignee)

Comment 15

13 years ago
Created attachment 154578 [details] [diff] [review]
OS detection, code cleanup, comments added and undetermined progressbars.
(Assignee)

Updated

13 years ago
Attachment #154514 - Attachment is obsolete: true
(Assignee)

Comment 16

13 years ago
I think the patch is ready for review comments.  Also feel free to suggest
better wording :)

Comment 17

13 years ago
(In reply to comment #16)
> Also feel free to suggest better wording :)

OK! Here goes:

> missingpluginsMessage.title=This page contains missing plug-in(s).

The page isn't missing plug-in(s), it contains items that need plug-ins to be
displayed. Something like "Additional plug-ins are required to display all the
media on this page." would be better. I think "plug-in(s)" looks ugly and should
be avoided.

> missingpluginsMessage.button.label=Install Missing Plugins

Requires additional information, so an ellipsis is needed: "Install Missing
Plug-ins...".

> <!ENTITY pluginWizard.title "Plug-in Finder Service">
> <!ENTITY pluginWizard.checkingForPlugins.description.label "&brandShortName;
> is now checking for available plug-ins...">
> <!ENTITY pluginWizard.availablePluginsPage.title "Available Plug-in
> Downloads">
> <!ENTITY pluginWizard.availablePluginsPage.description.label "The following
plug-ins are available:">

All look fine.

> <!ENTITY pluginWizard.availablePluginsPage.continueMsg.label "To install these
> plug-ins, continue on the next button below.">

How about just "Press Next to install these plug-ins."? That said, it feels like
something is wrong if you have to tell someone to press a button.

><!ENTITY pluginWizard.availablePluginsPage.installerUI "Plug-ins may have their
> own seperate installation windows that will show during installation.">

Sep_a_rate. :-) I'd question whether this is needed at all, but "Some plug-ins
may require additional information from you during installation." seems better
(I don't think most users would be too spooked by a few progress windows
flashing up but being asked to select installation folders/directories and agree
to EULAs may scare some).

><!ENTITY pluginWizard.installPluginsPage.title "Installing Plug-ins">

Cool.

> <!ENTITY pluginWizard.installPluginsPage.description.label "&brandShortName;
is installing the plug-ins...">

Don't think "the" is needed.

> <!ENTITY pluginWizard.finalPage.description.label "&brandShortName; finished
> installing the missing plug-ins.">

I would make it more of an intro to the list: "&brandShortName; has finished
installing the following plug-ins:".

> pluginInstallation.download.start=Downloading %S...
> pluginInstallation.download.finish=Finished downloading %S.
> pluginInstallation.install.start=Installing %S...

All fine.

> pluginInstallation.install.finish=Finished installing %S.

What about "Successfully installed %S." (to go with failed text below)?

> pluginInstallation.install.error=Installation of %S failed (%S).

Maybe "Failed to install %S (%S)."?

> pluginInstallation.complete=Finished installing all plug-ins.

I don't think "all" is needed (and it could suggest the user now has every
plug-in ever - not what we want).

> pluginInstallationSummary.success=Installed
> pluginInstallationSummary.failed=Failed (%S)

No complaints.

If it's simple, it would be nice to display some additional text if any plug-in
installs fails. Something along the lines of "You may wish to try downloading
and installing the failed plug-ins manually." (Or would that just confuse users?)

> pluginInstallation.noPluginsFound=Could not find any of the missing plug-ins.

Maybe "No suitable plug-ins were found." would be better.

This is one of those features that makes me all giddy inside. It's been a while
since that happened. :-)
(Assignee)

Comment 18

13 years ago
The text about the plugin installer showing its own UI was something I did
quickly, wasn't sure if we should do something with that data from the server or
not.  Probably giving a warning is usefull, but I am no usability expert.

And I was thinking if missing/failed installation occured, to add a link to
plugindoc's website for that platform (or simply to the main plugindoc page). 
Mozdev isn't a very reliable place though, which makes me hesitant to do that.

I'm also using the old-skool Plug-in Finder Service name currently, I liked it
more than "Plug-in Installation Wizard".  Its up to the foundation to choose if
that is ok or to use a new name.

I'll change the text values tomorrow and post a new patch for aviary branch.
Status: NEW → ASSIGNED
s/Plug-in/Plugin/g for consistency with what we've got up on the website etc.
Created attachment 154620 [details] [diff] [review]
Disable default plugin and handle missing plugin internally, and fire events so that our UI knows about it etc.
(Assignee)

Comment 21

13 years ago
Created attachment 154698 [details] [diff] [review]
more goodness

Changed the wording per the recommendations.  Also added support for manual
installation urls, which get shown if a plugin installation failed (and a url
is provided by the web service) in form of a button on the last screen.  It
opens a new window, but the wizard is always infront, need to figure out how
(and if) to do that correctly. I also moved the failure message to the tooltip
text of the "Failed" label.
(Assignee)

Updated

13 years ago
Attachment #154578 - Attachment is obsolete: true

Comment 22

13 years ago
Instead of using a grid to display the list of plugins, what about using a
listbox with checkboxes and let the user uncheck which plugins they don't want
to install? 
(Assignee)

Comment 23

13 years ago
Listboxes aren't very accessable.  As for checkboxes, maybe if I get the time.

Comment 24

13 years ago
people have forced us to use plug-in claiming it's the correct spelling (tm).

please don't help us become even less consistent (from about:plugins)

Installed plug-ins
Find more information about browser plug-ins at Netscape.com.
Help for installing plug-ins is available from plugindoc.mozdev.org.
Mozilla Default Plug-in
    Default Plug-in
* 	Mozilla Default Plug-in 	* 	Yes
Java Plug-in
    Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
Java Plug-in
    Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
Java Plug-in
    Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
Java Plug-in
    Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
Java Plug-in
    Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
Java Plug-in
    Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
Java Plug-in
    Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
    Adobe Acrobat Plug-In Version 6.00 for Netscape
application/x-drm-v2 	Network Interface Plugin 	nip 	Yes
    DRM Store Netscape Plugin
application/x-drm 	Network Interface Plugin 	nip 	Yes
Windows Media Player Plug-in Dynamic Link Library
(Assignee)

Comment 25

13 years ago
We use plugin and plug-in intermixed, usually in the same paragraph :)
(Assignee)

Comment 26

13 years ago
Created attachment 155107 [details] [diff] [review]
Allows user to choose what plugins to install, use pluginspage attribute for manual install link
Attachment #154698 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #155107 - Flags: review?(bugs)
(Assignee)

Comment 27

13 years ago
New screenshots - http://www.nexgenmedia.net/ff/screenshots
Flags: blocking-aviary1.0PR+
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0+
Priority: -- → P1
Target Milestone: --- → Firefox1.0beta
(Assignee)

Comment 28

13 years ago
So one problem with the wizard is that existing content has attributes to urls
to download plugins., which we would load if you click on the puzzle piece.

The solutions:

- List those plugins that are not in the db but have a pluginspage attribute. 
Problem is we don't have a name for the plugin.

- Create dummy entries in the webservice so that we can show the manual install
url with pretty names and all.

Flags: blocking-aviary1.0+ → blocking-aviary1.0?
Priority: P1 → --
Target Milestone: Firefox1.0beta → ---
I'll look at this on Tuesday. 
Priority: -- → P1
Comment on attachment 155107 [details] [diff] [review]
Allows user to choose what plugins to install, use pluginspage attribute for manual install link

>+      <handler event="PluginNotFound">
>+        <![CDATA[
>+          const browsers = this.mPanelContainer.childNodes;
>+
>+          var i = 0;
>+          for (; i < browsers.length; i++) {
>+            if (this.getBrowserAtIndex(i).contentWindow == event.target.ownerDocument.__parent__)

Help my rusty memory... what does __parent__ refer to? What happens if the
event 
is fired on a frame document, doesn't browser.contentWindow point to the root
frame? 
If so, doesn't this simple logic fail? 

>+          if (!tab.missingPlugins)
>+            tab.missingPlugins = new Array();

You create an Array....

>+          tab.missingPlugins[event.target.type] = [event.target.type, event.target.getAttribute("pluginspage")];

... and yet you're using it like an Object... use:

  tab.missingPlugins = new Object(); 

or 

  tab.missingPlugins = { }; 

instead.

>+          this.showMessage(browser, iconURL, messageString, buttonString, 
>+                                    "", "missing-plugin",
>+                                    null, "top");
>+

I'm not entirely comfortable with locating the showMessage code inside the 
tabbrowser, especially given that the handler which shows UI (which lives in
mozapps, according to your patch) is invoked in browser.js. This means that
when someone places a xul:tabbrowser element in their page and their 
embedded tabbrowser visits a page with a missing plugin, a UI section is shown
that they have to manually close and handle (probably with your UI)... not
sure yet where all of the code should live - either in browser with the
xpinstall and popup blocking notifications or in tabbrowser, but I think it
should all live *together*... as a result, can this PluginNotFound event 
handler move into browser.js code? See below for recommendation.

>     case nsIXPIProgressDialog.DOWNLOAD_DONE:
>+      alert(aValue)
>       element.setAttribute("progress", "100");

oops!

>+
>+    case "missing-plugin":
>+      // get the urls of missing plugins
>+      var tabbrowser = getBrowser();
>+      var missingPluginsArray = tabbrowser.mCurrentTab.missingPlugins;
>+
>+      if (missingPluginsArray) {
>+        window.openDialog("chrome://mozapps/content/plugins/pluginInstallerWizard.xul",
>+          "_blank", "chrome,resizable=yes", [missingPluginsArray, tabbrowser.mCurrentTab]);

Why not pass in an object instead of an array, like so:

... resizable=yes", { plugins: missingPluginArray, tab: tabbrowser.mCurrentTab
});

so that (see below for why)

>+      }            
>+
>+      tabbrowser.hideMessage(tabbrowser.selectedBrowser, "top"); 

... see below:

>   os.addObserver(gXPInstallObserver, "xpinstall-install-blocked", false);
>   os.addObserver(gXPInstallObserver, "xpinstall-install-edit-permissions", false);
>   os.addObserver(gXPInstallObserver, "xpinstall-install-edit-prefs", false);
>+  os.addObserver(gXPInstallObserver, "missing-plugin", false);

So, with reference to my comment towards the top of this review about location
of
code, what I recommend doing is creating a gMissingPluginHandler or
something...
adding a function to it that listens to the PluginNotFound event, calling
showMessage 
on the tabbrowser and has an observe method that handles "missing-plugin" that
calls
hideMessage... this way we keep all the code together, and don't overload
another
object called "XPInstallObserver"... 

>+ <wizardpage label="&pluginWizard.availablePluginsPage.title;"  
>+              onpageshow="gPluginInstaller.showPluginList()">

nit: whitespace on <wizardpage line is off by 1

>+    <grid style="margin-left:20px;">

Does class="indent" have the same visual effect without needing the inline
style?

>+    <description id="plugin_install_progress_message" value=""/>
>+    <progressmeter id="plugin_install_progress" mode="undetermined" value="0%"/>

You use interCaps elsewhere for IDs, but im_a_c_hacker here... make up your
mind! ;-) 

>+    <grid style="margin-left:20px;">

... repeat class="indent" comment.

>+  // create the request array
>+  this.mPluginRequestArray = new Array();
>+  // since the array is a hash, store the length
>+  this.mPluginRequestArrayLength = 0;

Since it's a hash, create with new Object()... 

>+  // a hash indexed by plugin id so we don't install 
>+  // the same plugin more than once.
>+  this.mPluginInfoArray = new Array();

... new Object() ...

>+
>+  this.mTab = null;
>+  this.mSuccessfulPluginInstallation = 0;
>+
>+  // arguments[0] is an array that contains two items:
>+  //     an array of mimetypes that are missing
>+  //     a reference to the tab that needs them, so we can reload it
>+
>+  if ("arguments" in window) {
>+    for (var item in window.arguments[0][0]){

... continued from above "so that"... you can write code like this:

for (var plugin in window.arguments[0].plugins) {

which is so much more readable to the casual observe, especially for 
complex lines like the next one:

>+      this.mPluginRequestArray[window.arguments[0][0][item][0]] = new nsPluginRequest(window.arguments[0][0][item]);

>+    this.mTab = window.arguments[0][1];

and this becomes something like: this.mTab = window.arguments[0].tab;

>+    var myCheckbox = document.createElement("checkbox");
>+    myCheckbox.setAttribute("checked", "true");
>+    myCheckbox.setAttribute("onclick", "gPluginInstaller.toggleInstallPlugin('" + pluginInfo.pid + "', this)");

oncommand, not onclick... keyboard access. 

>+    myCheckbox.setAttribute("style", "list-style-image:url('" + pluginInfo.IconUrl + "')");

set the "src" attribute to the iconURL... if that doesn't work, let me know. 


>+  for (pluginInfoItem in this.mPluginInfoArray){
>+    if (this.mPluginInfoArray[pluginInfoItem].toBeInstalled) {
>+      pluginURLArray[pluginURLArray.length] = this.mPluginInfoArray[pluginInfoItem].XPILocation;

pluginURLArray.push(this.mPluginInfoArray[pluginInfoItem].XPILocation);

... a little neater.

>+      pluginPidArray[pluginPidArray.length] = this.mPluginInfoArray[pluginInfoItem].pid;

... ditto. 

>+  for (var run = myRows.childNodes.length; run--; run > 0)

?? don't you mean for (var run = myRows.childNodes.length; run > 0; run--) ? 

as it stands, the run > 0 piece is never being evaluated (and is useless) 
but what makes sure the for loop bails is that when run becomes 0 through
ongoing decrementing, the lhs becomes 0 and so the for loop exits. This is
valid, but weird, and based on the "run > 0" piece probably not what you
intended?

>+  onStateChange: function (aIndex, aState, aValue)

nit: you do whitespace like this: functionName : function ( for all the 
others, but not here. 

>+    switch (aState) {
>+      case nsIXPIProgressDialog.DOWNLOAD_START:
>+        gPluginInstaller.pluginInstallationProgress(pid, 0);

is the magic number you're passing to pluginInstallProgress == aState? If 
so, you could simplify this entire section like so:

  var errorMsg = "";
  switch (aState) {
  case nsIXPIProgressDialog.INSTALL_DONE:
    if (aValue != 0) {
      var xpinstallStrings = document.getElementById("xpinstallStrings");
      try {
	errorMsg = xpinstallStrings.getString("error" + aValue);
      }
      catch (e) {
	errorMsg = xpinstallStrings.getFormattedString("unknown.error",
[aValue]);
      }
    }
    break;
  default:
    break;
  }

  // errorMsg is "" for all states except INSTALL_DONE when there's been 
  // an error...
  gPluginInstaller.pluginInstallationProgress(pid, aState, errorMsg);


>+var ws_proxy = null;

Throughout the WS code... c_hacker_caps... 

>+const ws_service_uri = "http://localhost:8080/axis/services/PluginFinderService?wsdl"

This should be localizable in a properties file, just in case. 

We may want to massage the text a little in the various screens, but what you
have 
will do to start with. 

I've made a lot of comments here but they're mostly organizational so probably 
not too much of a trouble ;-)
(Assignee)

Comment 31

13 years ago
I'll post a new patch tomorrow, have almost all the issues covered.
(Assignee)

Comment 32

13 years ago
Created attachment 155978 [details] [diff] [review]
Addresses review feedback

Fixed the mentioned issues.
(Assignee)

Updated

13 years ago
Attachment #154620 - Attachment is obsolete: true
Attachment #155107 - Attachment is obsolete: true
Cool I'll look at this later tonight.

Updated

13 years ago
Whiteboard: [have patch]
Comment on attachment 155978 [details] [diff] [review]
Addresses review feedback

>   os.addObserver(gXPInstallObserver, "xpinstall-install-blocked", false);
>   os.addObserver(gXPInstallObserver, "xpinstall-install-edit-permissions", false);
>   os.addObserver(gXPInstallObserver, "xpinstall-install-edit-prefs", false);
>+  os.addObserver(gMissingPluginInstaller, "missing-plugin", false);

Don't forget to remove the observer later on (search for "gXPInstallObserver"
to find the place) or you'll leak. 

Fix that and r+a=ben@mozilla.org.
Attachment #155978 - Flags: review+
Attachment #155978 - Flags: approval-aviary+
(Assignee)

Comment 35

13 years ago
Any idea when the web serivce on mozilla.org will be ready?
Doron, I suggest contacting leaf, myk or bryner about getting access to rodan,
where tomcat is currently running.... once you have access you can check out
your web service there (I recommend checking it into cvs with your installer),
compile and deploy it. I have a script that you might be able to use to make
that process easier... let me know if you need it. 

Comment 37

13 years ago
It is planned to translate this plugin installer ?
(Assignee)

Comment 38

13 years ago
Is there a freeze date for string stuff for the aviary branch?  The patch from
244125 is needed to make this work, but this could be checked in without it.

I'll post a new patch that fixes the leak and get someone to check it in tomorrow.
this blocks PR, that's all you need to know. 

btw, I just re-read the web services part of this patch. I forgot one thing. You
need to pass the installed language to the web service function so that we can
offer multiple language versions of flash, etc. 

You can infer the selected locale from the locale the global package is using...
so all you have to do is: 

1) add a string parameter to your ws api function for the locale
2) call this code:

var cr = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
                   .getService(Components.interfaces.nsIXULChromeRegistry);
var locale = cr.getSelectedLocale("global");

3) pass the param in
4) then we can easily make the web service hand back varying urls depending on
language, without having to update the clients.

and remember, i before e except after c: pluginInfoRecieved should be
pluginInfoReceived. 

;-) 
(Assignee)

Comment 40

13 years ago
Created attachment 156278 [details] [diff] [review]
Patch to be checked in

Fixed the observer leak, sends locale to the web service, includes webservice
java code and make shell script.
(Assignee)

Comment 41

13 years ago
yes, the installer is fully localizable, and second, someone needs to check this
is as I don't have cvs access.

Comment 42

13 years ago
can some one help in getting this landed?

Comment 43

13 years ago
If I visit a page with Flash, this bar is going to appear at the top of my
screen, right?  There is a way to dismiss the bar without installing, correct? 
And there's a way to dismiss the bar telling it to never ask me again, right?  

I prefer to browse without Flash (display:none in userContent), so I really
don't want this bar popping up for every site that has a Flash ad.  

As far as I know, the popup blocker only appears once and then you can tell it
to never pester you again.  

Comment 44

13 years ago
Comment on attachment 156278 [details] [diff] [review]
Patch to be checked in

(just skimming)

> +    myCheckbox.setAttribute("label", pluginInfo.name + " " + (pluginInfo.version + ""));

that's not very l10n friendly.
This is now checked in on the aviary branch, ws code n' all.
(Assignee)

Comment 46

13 years ago
I'm working on extending this to allow viewing/accepting of licenses should the
plugin vendor decide to choose so (they all do pretty much).

I should probably get cvs access finally :)
So this is mostly fixed on the aviary branch now, what's still needed is Doron's
license changes he mentioned, and we need to make this code point to the web
service on update.mozilla.org, once available.
Oh, and Doron, how about an 'X' in that bar so that people can close the bar w/o
installing any plugins? And ideally there'd be a "Never show this again for this
plugin", but I can live w/o that for now...
Whiteboard: [have patch] → [have patch] additional license work affects l10n
(Assignee)

Comment 49

13 years ago
Created attachment 156637 [details] [diff] [review]
Adds license page and [x] to the yellow message bar.  Had to change the tabbrowser message api.
(Assignee)

Updated

13 years ago
Attachment #156637 - Flags: superreview?(bugs)
Attachment #156637 - Flags: approval-aviary?
Comment on attachment 156637 [details] [diff] [review]
Adds license page and [x] to the yellow message bar.  Had to change the tabbrowser message api.

r+a=ben@mozilla.org providing you test the various xpinstall/popup blocked
cases well
Attachment #156637 - Flags: superreview?(bugs)
Attachment #156637 - Flags: superreview+
Attachment #156637 - Flags: approval-aviary?
Attachment #156637 - Flags: approval-aviary+
(Assignee)

Comment 51

13 years ago
On my cvs build, popups do not open a message.  is that turned on?
(Assignee)

Comment 52

13 years ago
I tested the patch on XPI installation and popups and the message toolbar shows
fine with it.

Comment 53

13 years ago
Are there plans for a pref to disable this? I normally just toss
libnullplugin.so and all is well, but that's no good with this. I'm really not
into plugins and would rather not see this all over the place...
http://www.gozer.org/test/mozilla/253046/shot.png

I'd even settle for some css bits if anyone has some (id or class). :P
There is a pref that'll disable this new code if the null plugin is available,
but there's no real way to turn all of this off, short of rolling your own
version of the default plugin that does absolutely nothing (i.e. no silly modal
dialog etc). Given enough demand we could add such a pref, but I doubt that's of
interested to more than a really small number of people...

Comment 55

13 years ago
I tried using the 8/20 & 8/21 official aviary builds and can't get the windows
media player plugin to load, I get the new missing plugin installer message.
Everything works fine on the 8/16 aviary build on win 2000 & xp.

example failing url:
http://www.espnradio.com/
clik on one of the listen now links on the right hand side

Do you belive my problem is related to the code being checked in for this bug?
(Assignee)

Comment 56

13 years ago
That seems to be a bug with jst's plugin changes - the UI gets notified by it.

I can reproduce. Probably should file a seperate bug.

Comment 57

13 years ago
(In reply to comment #56)
> I can reproduce. Probably should file a seperate bug.

Bug #256711.
Currently, Fx Help says this:

"To see a full list of Firefox plug-ins you can install, see the Browser
Plug-ins page <http://home.netscape.com/plugins/index.html> at Netscape.com."

Will there be a replacement page for the Netscape.com one, or should that
reference be left in Help?

Comment 59

13 years ago
(In reply to comment #58)
> Will there be a replacement page for the Netscape.com one, or should that
> reference be left in Help?

There *might* be a page with a list on update.mozilla.org for plugins by 1.0..
But for 1.0PR, I doubt it. I don't speak for what mozilla.org itself will do though.

(Assignee)

Comment 60

13 years ago
Anyone want to check this in?

Comment 61

13 years ago
jst, can you help land?
Whiteboard: [have patch] additional license work affects l10n → [have patch] ready to land

Comment 62

13 years ago
I should probably file a new bug for this but i see you guys are about to land a
patch so i want to make sure this is covered first.

someone sent me a link to a video, i go to the page and i'm missing the plugins
well the placeholder for the missing plugin has the old netscape missing plugin
icon in the middle. here is link to the page (WARNING: don't watch the video if
you have the plugin) btw the plugin is for quicktime. other pages with quicktime
i get the regular firefox version icon for missing plugin like whats at the
bottom on the page at the link.

http://zed.cbc.ca/go.ZeD?CONTENT_ID=2365&mediaSize=double&page=media-viewer

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040821
Firefox/0.9.1+
Fix checked in on the aviary branch.

Kurt, can you please test with a nightly firefox branch build from 8/25 or later?
Keywords: fixed-aviary1.0

Comment 64

13 years ago
Created attachment 157036 [details]
wrong plugin icon

still present with fresh install of Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.7.2) Gecko/20040825 Firefox/0.9.1+

Comment 65

13 years ago
Created attachment 157037 [details]
wrong plugin icon

still present with fresh install of Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.7.2) Gecko/20040825 Firefox/0.9.1+

but opening in IE i get a message in IE's bar saying "this site might require
the follwing ActiveX Control: 'Quicktime Installer' from 'Apple Computer Inc.'

so maybe because it require ActiveX on that page that thats the reason why for
the weird icon?
Here's what's going on... The current builds are in an odd state in that the
default plugin replacement is in, but it's not fully enabled, which brings up an
problem with the code, since that means that the new code should never be
triggerd, you should *always* see the good ol' default plugin, unless you set a
pref.

Please do file a separate bug on this problem.

Comment 67

13 years ago
alright, but does the patch for this bug (253046) fix the bug that I described?
if so no point in filing a new bug if it just going to be dependent on this bug
and be fixed in tomorrows build.


Here's what's going on... The current builds are in an odd state in that the
default plugin replacement is in, but it's not fully enabled, which brings up an
problem with the code, since that means that the new code should never be
triggerd, you should *always* see the good ol' default plugin, unless you set a
pref.

Please do file a separate bug on this problem.
(In reply to comment #67)
> alright, but does the patch for this bug (253046) fix the bug that I described?
> if so no point in filing a new bug if it just going to be dependent on this bug
> and be fixed in tomorrows build.

No, the patches here do *not* fix the problem you described. For that, we need a
new bug (I'm close to a fix already).

Comment 69

13 years ago
alright, I filed a new bug http://bugzilla.mozilla.org/show_bug.cgi?id=256956 

btw sorry about the double post earlier and the other message with half quote
post...buzilla was acting up and i kept getting timeouts.

Comment 70

13 years ago
(In reply to comment #24)
> people have forced us to use plug-in claiming it's the correct spelling (tm).
> 
> please don't help us become even less consistent (from about:plugins)
> 
> Installed plug-ins
> Find more information about browser plug-ins at Netscape.com.
> Help for installing plug-ins is available from plugindoc.mozdev.org.
> Mozilla Default Plug-in
>     Default Plug-in
> * 	Mozilla Default Plug-in 	* 	Yes
> Java Plug-in
>     Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
> Java Plug-in
>     Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
> Java Plug-in
>     Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
> Java Plug-in
>     Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
> Java Plug-in
>     Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
> Java Plug-in
>     Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
> Java Plug-in
>     Java Plug-in 1.4.2_03 for Netscape Navigator (DLL Helper)
>     Adobe Acrobat Plug-In Version 6.00 for Netscape
> application/x-drm-v2 	Network Interface Plugin 	nip 	Yes
>     DRM Store Netscape Plugin
> application/x-drm 	Network Interface Plugin 	nip 	Yes
> Windows Media Player Plug-in Dynamic Link Library


Are you ever right! I looked at the screen shots on the link below
http://www.nexgenmedia.net/ff/screenshots/
When it says "plugins" it definitely stands out as a typo.
It's ok for underlying code but "not" for what the user sees.
(Assignee)

Updated

13 years ago
Depends on: 256657, 257472
(Assignee)

Comment 71

13 years ago
Ben - looking at nsIHttpProtocolHandler, I see it has a platform memeber
(http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpHandler.cpp#588)
that is generic (windows, linux, etc).  It probably makes sense to send that to
the service as well.  Or do we really want to check on the server if the oscpu
is WinNT 5.1 (XP), Win98, etc)?

Comment 72

13 years ago
The UMO db does not distinguish between versions of Windows.  I can see where
this would cause problems for some plugins.  But if we're not hosting the
plugin, and we redirect them to Macromedia's page, then I don't think its a problem.
(Assignee)

Comment 73

13 years ago
Created attachment 157969 [details] [diff] [review]
more fixes (empty license is null in RDF)
(Assignee)

Updated

13 years ago
Attachment #157969 - Flags: superreview?(bugs)
Attachment #157969 - Flags: approval-aviary?

Updated

13 years ago
Flags: blocking-aviary1.0?
(In reply to comment #18)
Doron Rosenberg (IBM) wrote:
> And I was thinking if missing/failed installation occured, to add a link to
> plugindoc's website for that platform (or simply to the main plugindoc page). 
> Mozdev isn't a very reliable place though, which makes me hesitant to do that.

While I agree that mozdev isn't exactly reliable, I still think it's better than
nothing. Looking at the screenshot
<http://www.nexgenmedia.net/ff/screenshots/pfs-4.png>, it doesn't provide the
user with any information at all.. It would certainly ease the IRC/forum
pressure on plugin-issues by a large factor.

Thanks for the great work, it looks beautiful :)
Comment on attachment 157969 [details] [diff] [review]
more fixes (empty license is null in RDF)

looks good to me, lets get this stuff working :)
Attachment #157969 - Flags: superreview?(bugs) → superreview+

Updated

13 years ago
Attachment #157969 - Flags: superreview+ → review+

Comment 76

13 years ago
Comment on attachment 157969 [details] [diff] [review]
more fixes (empty license is null in RDF)

a=asa for aviary checkin.
Attachment #157969 - Flags: approval-aviary? → approval-aviary+
(Assignee)

Comment 77

13 years ago
Checked in.  I am hosting a pfs php file which for windows links to sun's java
XPI, and on linux to my unofficial flash xpis.  If anyone wants to test :)
(Assignee)

Comment 78

13 years ago
Created attachment 158467 [details] [diff] [review]
If no XPI link is given, add an manual install button. Also link to plugins page on update on the last page.
(Assignee)

Updated

13 years ago
Attachment #158467 - Flags: superreview?(bugs)
Attachment #158467 - Flags: approval-aviary?
Is there a way to permanently suppress the yellow bar telling me that there is
content on the page I don't have plug ins for?   Many people hate flash and
never install it.  Before they could just ignore the flash ads, demos, etc. now
they have to click to get rid of the yellow bar every time.

Comment 80

13 years ago
set plugin.default_plugin_disabled to false
(In reply to comment #80)
> set plugin.default_plugin_disabled to false

Not quite what I had in mind.  I don't have a problem with the click to load the
plugin (on the puzzle pieces in the content area) in this new version, I like it
much better in fact.  I just want the ablilty to supress the yellow bar.  You
can do this with the popup blocker, click the yellow bar, a menu appears then
click "Don't show this message when popups are blocked".  If there is no plugin
(or I just don't want to install it for some reason) for the OS I'm running, the
yellow bar will get old very quickly.

Comment 82

13 years ago
(In reply to comment #81)
> I just want the ablilty to supress the yellow bar.  You
> can do this with the popup blocker, click the yellow bar, a menu appears then
> click "Don't show this message when popups are blocked".  

Agreed.  For consistency of user experience, behavior should match the bar shown
for popups and blocked software (extensions) installation. 
(Assignee)

Comment 83

13 years ago
Software installation does not have such a feature btw.  The only sensible way
would be a checkbox in the infobar, which would be UI overkill probably.  I
defer to ben to decided what he wants.

Comment 84

13 years ago
why even have the bar? Just add a comment for hover over so that when mouse is
over the puzzle piece a balloon comes up and says something like 'A required
Plug-in is missing, please click to install the required plug-in.'
The bar is necessary if not for any other reason than to notify the user in the
case where the plugin is hidden (i.e. not displayed at all), and it's also very
useful when the plugin is very small and/or hard to locate on the page (the page
could be huge etc).
At some point in the future it might make sense to add the ability to suppress
the yellow bar for a certain mime type. I.e. if I really don't ever want a flash
plugin on my system, I should be able to say that I don't want this plugin, and
Firefox shouldn't ask or notify you with the yellow bar again. But that's not
1.0 material, we're too close to a release for things like that, IMO.

Comment 87

13 years ago
Sorry for bugspam, but I keep hearing yellow bar. Is it colored on Windows or is
there something else? I just see a grey toolbar on linux.
(In reply to comment #87)
> Sorry for bugspam, but I keep hearing yellow bar. Is it colored on Windows or is
> there something else? I just see a grey toolbar on linux.

The bar uses the system's tooltip background colour, which on Windows defaults
to, and is almost always, a light yellow colour. (Tooltips not doing this is bug
253191.)
(Assignee)

Comment 89

13 years ago
Created attachment 158639 [details] [diff] [review]
Make pfs window modal.

Comment 90

13 years ago
(In reply to comment #89)
> Created an attachment (id=158639)
> Make pfs window modal.
> 

Also, PFS window needs to remember its screen position after the first use.
(Assignee)

Comment 91

13 years ago
Created attachment 158674 [details] [diff] [review]
make window modal and remember position/size

your wish is my command.
Attachment #158639 - Attachment is obsolete: true
Comment on attachment 158467 [details] [diff] [review]
If no XPI link is given, add an manual install button. Also link to plugins page on update on the last page.

r+sr=jst
Attachment #158467 - Flags: superreview?(bugs)
Attachment #158467 - Flags: superreview+
Attachment #158467 - Flags: review+

Comment 93

13 years ago
Comment on attachment 158467 [details] [diff] [review]
If no XPI link is given, add an manual install button. Also link to plugins page on update on the last page.

a=asa (on behalf of the aviary team) for checkin to the aviary branch.
Attachment #158467 - Flags: approval-aviary? → approval-aviary+
Are all the l10n changes done here? I didn't realize that there were more
changes needed.
(Assignee)

Comment 95

13 years ago
Created attachment 159031 [details] [diff] [review]
fixes numerous issues (restart warning, allow license accpeting only after iframe has loaded, make applet tag work) and adds the final localizations
(Assignee)

Updated

13 years ago
Attachment #159031 - Flags: superreview?(bugs)
Attachment #159031 - Flags: approval-aviary?
(Assignee)

Comment 96

13 years ago
Created attachment 159037 [details] [diff] [review]
fixes numerous issues (restart warning, allow license accpeting only after iframe has loaded, make applet tag work)

string changes already checked into aviary
(Assignee)

Updated

13 years ago
Attachment #159031 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #159037 - Flags: approval-aviary?
Doron, looks like that diff is broken (doesn't apply, data missing from the
beginning of the file) :-(

Comment 98

13 years ago
doronr: thanks for the position save fix.

I've tested the new plugin installer, and it worked pretty well in FF 1.0PR. FF
automatically installed Flash plugin.  The only minor interface glitch was the
(yellow) info bar that stayed on _after_ plugin was installed. It will make
sense for the missing plugin info bar to be gone once the required plugin is
installed.
(Assignee)

Comment 99

13 years ago
Created attachment 159100 [details] [diff] [review]
fixes numerous issues (restart warning, allow license accpeting only after iframe has loaded, make applet tag work)
Attachment #159037 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #159100 - Flags: approval-aviary?
Comment on attachment 155107 [details] [diff] [review]
Allows user to choose what plugins to install, use pluginspage attribute for manual install link

patch is obsolete
Attachment #155107 - Flags: review?(bugs)
Comment on attachment 159031 [details] [diff] [review]
fixes numerous issues (restart warning, allow license accpeting only after iframe has loaded, make applet tag work) and adds the final localizations

patch is obsolete
Attachment #159031 - Flags: superreview?(bugs)
Attachment #159031 - Flags: approval-aviary?
Comment on attachment 159037 [details] [diff] [review]
fixes numerous issues (restart warning, allow license accpeting only after iframe has loaded, make applet tag work)

patch is obsolete
Attachment #159037 - Flags: approval-aviary?

Comment 103

13 years ago
Comment on attachment 159100 [details] [diff] [review]
fixes numerous issues (restart warning, allow license accpeting only after iframe has loaded, make applet tag work)

a=asa for aviary checkin.
Attachment #159100 - Flags: approval-aviary? → approval-aviary+
(Assignee)

Updated

13 years ago
Attachment #159100 - Flags: review?(bugs)
Comment on attachment 159100 [details] [diff] [review]
fixes numerous issues (restart warning, allow license accpeting only after iframe has loaded, make applet tag work)

r+sr=jst
Attachment #159100 - Flags: superreview+
Attachment #159100 - Flags: review?(bugs)
Attachment #159100 - Flags: review+
(Assignee)

Comment 105

13 years ago
marking this fixed.  Any new bugs/patches should go into seperate bugs.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.