257472 - Move plugin finder service to RDF/XML

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Doron Rosenberg (IBM)]

Comment 1

[Doron Rosenberg (IBM)]

Attachment: move to rdf datasource

Comment 2

[chris hofmann]

getting on the PR radar

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 157753 [details] [diff] [review]
move to rdf datasource

>   <script type="application/x-javascript" src="chrome://mozapps/content/plugins/pluginInstallerWizard.js"/>
>+  <script type="application/x-javascript" src="chrome://mozapps/content/plugins/pluginInstallerDatasource.js"/>
>   <script type="application/x-javascript" src="chrome://mozapps/content/plugins/pluginWSConnection.js"/>

Don't you need to remove pluginWSConnection too?

>+
>+        function getPFSValueFromRDF(aValue, aTarget, aDatasource, aRDFService){
>+          var rv = null;
>+          var myTarget = aDatasource.GetTarget(aTarget, aRDFService.GetResource(PFS_NS + aValue), true);
>+          if (myTarget)
>+            rv = myTarget.QueryInterface(Components.interfaces.nsIRDFLiteral).Value;
>+
>+          return rv;
>+        }
>+
>+        pluginInfo = new Object();
>+        pluginInfo.name = getPFSValueFromRDF("name", target, aDatasource, this._rdfService);

Just a point here about functions defined within the scope of another
function... you don't need to pass in datasource/rdf service each time. You
can:

var rdfs = this._rdf;
function getPFSValueFromRDF(aValue, aTarget) {
  ...
  var myTarget = aDatasource.GetTarget(aTarget, rdfs.GetResource(...
  ...
}

Variables available in the outer scope are available in the inner, although I
had to create a local for the RDF service since |this| in the sub-function is
bound to the sub-function, not the parent. 

Just a thought, might make lines shorter. 

>+function nsExtensionUpdateXMLRDFDSObserver(aUpdater, aPluginRequestItem){
>+  this._updater = aUpdater;
>+  this._item    = aPluginRequestItem;
>+}

If you wanted, you could fold this separate object into nsRDFItemUpdater.. it
doesn't really have to be a separate object - I realized this in
nsExtensionManager.js.in and folded it in when I updated that file recently. 

>--- /dev/null	2004-02-23 15:02:56.000000000 -0600
>+++ toolkit/mozapps/plugins/service/PluginFinderService.php	2004-09-02 

I'll get vlad to review this. 

As far as the chrome changes go, make sure any unneeded WS code is removed and
follow any of my advice you agree with and r+a=[e-mail]

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 157753 [details] [diff] [review]
move to rdf datasource

>+    var dsURI = this.dsURI;
>+    dsURI = dsURI.replace(/%PLUGIN_MIMETYPE%/g, aPluginRequestItem.mimetype);
>+    dsURI = dsURI.replace(/%APP_ID%/g, this.appID);
>+    dsURI = dsURI.replace(/%APP_VERSION%/g, this.buildID);

Hold up... where's the platform and lang info?

Comment 5

[Doron Rosenberg (IBM)]

> If you wanted, you could fold this separate object into nsRDFItemUpdater.. it
> doesn't really have to be a separate object - I realized this in
> nsExtensionManager.js.in and folded it in when I updated that file recently. 
> 
Not sure how extensions work, but I call for each missing plugin mimetype a
nsRDFItemUpdater in a for loop, and due to the async nature of the RDF loading,
the next for item might have been called before the previous RDF request
returns.  And since I need to know the mimetype to be able to query the RDF
(unlike web services, where I could check a member of the returned object), I
need to create multiple observers that preserve the plugin request scope.  That
was long :)

Comment 6

[Doron Rosenberg (IBM)]

Attachment: cleaned up patch

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 157852 [details] [diff] [review]
cleaned up patch


>+$reqTargetAppGuid = $_GET['appID'];
>+$reqTargetAppVersion = $_GET['appVersion'];
>+$clientOS = $_GET['clientOS'];
>+$chromeLocale = $_GET['chromeLocale'];

[...]

>+if ($mimetype == "application/x-mtx") {
>+  $name = "Viewpoint Media Player";
>+  $guid = "{03F998B2-0E00-11D3-A498-00104B6EB52E}";
>+  $version = "5.0";
>+  $iconUrl = "";
>+  $XPILocation = "http://www.nexgenmedia.net/flashlinux/invalid.xpi";
>+  $InstallerShowsUI = false;
>+  $manualInstallationURL = "http://www.viewpoint.com/pub/products/vmp.html";
>+  $licenseURL = "http://www.viewpoint.com/pub/privacy.html";
>+} else if ($mimetype == "application/x-shockwave-flash") {
>+  $name = "Flash Player";
>+  $guid = "{D27CDB6E-AE6D-11cf-96B8-444553540000}";
>+  $version = "7.0.16";
>+  $iconUrl = "http://goat.austin.ibm.com:8080/flash.gif";
>+  $XPILocation = "http://www.nexgenmedia.net/flashlinux/flash-linux.xpi";
>+  $InstallerShowsUI = "false";
>+  $manualInstallationURL = "http://www.macromedia.com/go/getflashplayer";
>+  $licenseURL = "http://www.macromedia.com/shockwave/download/license/desktop/";

Unless i'm missing something, this is ignoring clientOS (and chromeLocale) as
passed in, and is always returning linux XPIs, no?

Comment 8

[Doron Rosenberg (IBM)]

Right, the php doesn't check os or locale as it was only used in testing on
linux :)  Since the whole logic has to be replaced with a DB call anyways,
didn't bother to add it.  Should I?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #8)
> Right, the php doesn't check os or locale as it was only used in testing on
> linux :)  Since the whole logic has to be replaced with a DB call anyways,
> didn't bother to add it.  Should I?

Ahh, I see.. ben said that this stuff isn't in the database anyway, so I thought
that you had written a php bit that we could use as the final one.  If you
already have all the data (URLs and whatnot for the XPIs), go for it.. your php
should be fine as the production one, we can always replace it later once the
plugin stuff starts living in the database.

Comment 10

[Doron Rosenberg (IBM)]

Those xpis are self rolled and probably violate the vendor's agreement - cc:
chofmann and asking what the foundation thinks (since they would be the host and
thus be at the recieving end of any problems).

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 157852 [details] [diff] [review]
cleaned up patch

>+  var rdfUpdater = new nsRDFItemUpdater(this.getOS(), this.getChromeLocale());

Ah, I see, you create only one updater, and many observes. My bad. The EM
creates one of these per item (initially because it was only used for
extensions that had supplied custom RDF update files and so there weren't many
of them - now that everyone is using this same system it seems a little silly
in EM).

>+    var dsURI = this.dsURI;
>+    dsURI = dsURI.replace(/%PLUGIN_MIMETYPE%/g, aPluginRequestItem.mimetype);
>+    dsURI = dsURI.replace(/%APP_ID%/g, this.appID);
>+    dsURI = dsURI.replace(/%APP_VERSION%/g, this.buildID);
>+    dsURI = dsURI.replace(/%CLIENT_OS%/g, this.clientOS);
>+    dsURI = dsURI.replace(/%CHROME_LOCALE%/g, this.chromeLocale);

Cool. 

>+if (!array_key_exists('appID', $_GET) ||
>+    !array_key_exists('appVersion', $_GET))
>+    bail ("Invalid request.");

It's probably worthwhile checking for clientOS and chromeLocale too since we
can't make many decisions without them... just a precaution. 

We can load the plugins into this file once we get them. 

r=[e-mail] ... thanks for picking this up so quickly, Doron.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 157852 [details] [diff] [review]
cleaned up patch

a=[e-mail]

Comment 13

[Doron Rosenberg (IBM)]

checked in.

Comment 14

[Doron Rosenberg (IBM)]

Attachment: I screwed up the argument order on an interface, so it breaks if an errror occures.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 157960 [details] [diff] [review]
I screwed up the argument order on an interface, so it breaks if an errror occures.

r+a=[e-mail]