Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Description • 14 years ago

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100427 Minefield/3.7a5pre

If you start a search in the top bar for add-on types other than extensions no result is returned. We should support searching for all kinds of add-ons.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 1 • 14 years ago

Er, can you double check this? It's never been restricted to just extensions. I just tested by searching for flash and got the Flash plugin as a result.

	Robert Strong (they/them - no direct email) Assignee
	Comment 2 • 14 years ago

When I search for flash (it is installed) I get no results and the following in the error console
Warning: WARN addons.manager: Exception calling callback: TypeError: aStr is null

	Robert Strong (they/them - no direct email) Assignee
	Comment 3 • 14 years ago

btw: if I search for an installed extension it does display in the results but I see that same error. Searching for a plugin or a theme has so far failed each time. I have also seen the following in the error console
Warning: reference to undefined property aTheme[prop]
Source File: file:///C:/Program%20Files%20(x86)/Minefield/modules/LightweightThemeManager.jsm
Line: 398

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 4 • 14 years ago

(In reply to comment #3)
> btw: if I search for an installed extension it does display in the results but
> I see that same error. Searching for a plugin or a theme has so far failed each
> time. I have also seen the following in the error console
> Warning: reference to undefined property aTheme[prop]
> Source File:
> file:///C:/Program%20Files%20(x86)/Minefield/modules/LightweightThemeManager.jsm
> Line: 398

Filed bug 562848 for this - which will fix that warning, but may also fix this bug too.

	Robert Strong (they/them - no direct email) Assignee
	Comment 5 • 14 years ago

That isn't the cause :(

	Robert Strong (they/them - no direct email) Assignee
	Comment 6 • 14 years ago

This appears to be caused by extensions that don't have a description.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 7 • 14 years ago

(In reply to comment #6)
> This appears to be caused by extensions that don't have a description.

Yea, that was my thinking. As in, the description property is null or undefined - right? I'd consider that an API bug, since description isn't an optional property.

	Robert Strong (they/them - no direct email) Assignee
	Comment 8 • 14 years ago

The following returns null
function getRDFValue(aLiteral) {
  if (aLiteral instanceof Ci.nsIRDFLiteral)
    return aLiteral.Value;
  if (aLiteral instanceof Ci.nsIRDFResource)
    return aLiteral.Value;
  if (aLiteral instanceof Ci.nsIRDFInt)
    return aLiteral.Value;
  return null;
}

and causes this since there is no description in the install.rdf... previously the code returned undefined.

	Robert Strong (they/them - no direct email) Assignee
	Comment 9 • 14 years ago

I suspect description should be made optional (extensions have been doing this since there were ) should be handled in the same way this type of extension was created) and it should be handled in the same way as a missing homepageURL which I see is handled properly and is part of this same group in XPIProvider.jsm.

	Robert Strong (they/them - no direct email) Assignee
	Comment 10 • 14 years ago

(In reply to comment #9)
> I suspect description should be made optional (extensions have been doing this
> since there were )
since there were this type of extension

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 11 • 14 years ago

(In reply to comment #9)
> I suspect description should be made optional

To me, optional means a provider doesn't have to implement it for its Addon objects (because of either technical restrictions, or it just doesn't make sense to implement it for a given addon type). But in this case, its implemented, but there's just lack of data - so an empty string should be supplied.

	Robert Strong (they/them - no direct email) Assignee
	Comment 12 • 14 years ago

What if it is an optional integer or some other type? I personally think optional properties within a provider should return that the property doesn't exist (e.g. null). It looks like this is already handled when the homepageURL doesn't exist and could be handled in the same way.

	Robert Strong (they/them - no direct email) Assignee
	Comment 14 • 14 years ago

Comment on attachment 442623 [details] [diff] [review]
patch rev1

also fixes bug 562848... when a light weight theme doesn't have a description it returns undefined and ends up with undefined as the description.

Blair, I pretty strongly think that the backend shouldn't return an empty string for an optional property that doesn't exist... are you ok with this?

	Robert Strong (they/them - no direct email) Assignee
	Comment 15 • 14 years ago

btw: I could more easily split out the properties... kind of late.

	Robert Strong (they/them - no direct email) Assignee
	Comment 16 • 14 years ago

Comment on attachment 442623 [details] [diff] [review]
patch rev1

going to fix this up

	Dave Townsend [:mossop]
	Comment 18 • 14 years ago

(In reply to comment #11)
> (In reply to comment #9)
> > I suspect description should be made optional
> 
> To me, optional means a provider doesn't have to implement it for its Addon
> objects (because of either technical restrictions, or it just doesn't make
> sense to implement it for a given addon type). But in this case, its
> implemented, but there's just lack of data - so an empty string should be
> supplied.

To me an optional property is one where either the provider need not implement it at all (and so it is not defined on the Addon instance) or the property's value may be null (or I suppose undefined too). In this case I think it was a mistake to say that description is a required property since we have never required that extensions have one so this looks like the correct fix to me.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 19 • 14 years ago

(In reply to comment #14)
> Blair, I pretty strongly think that the backend shouldn't return an empty
> string for an optional property that doesn't exist... are you ok with this?

For optional properties, yes that makes sense. And making description an optional property (comment 18) also sounds good. If description were to stay a required property and possibly be null/undefined, then I'd be uncomfortable.

	Robert Strong (they/them - no direct email) Assignee
	Comment 20 • 14 years ago

In regards to providers, I think it would be a "good thing" to standardize whether a provider should return null or undefined for a value that isn't present and to throw when a required value the value isn't present which I believe it already.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 21 • 14 years ago

http://hg.mozilla.org/projects/addonsmgr/rev/44975b9df5b1

(In reply to comment #20)
> In regards to providers, I think it would be a "good thing" to standardize
> whether a provider should return null or undefined for a value that isn't
> present and to throw when a required value the value isn't present which I
> believe it already.

Agreed. Though, I don't think any of the providers currently throw in that case (its unfortunate that its up to the provider to ensure that).

	Dave Townsend [:mossop]
	Comment 22 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/42101b017a4b

	Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Comment 23 • 14 years ago

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100517 Minefield/3.7a5pre (.NET CLR 3.5.30729) ID:20100517035949

Can we do something here with automated tests regarding the missing availability of different types of add-ons on the testing machines. I think a Litmus test is a must have.

	Dave Townsend [:mossop]
	Comment 24 • 14 years ago

It's likely difficult to cover this specific case with an automated test, Blair what do you think?

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 25 • 14 years ago

(In reply to comment #24)
> It's likely difficult to cover this specific case with an automated test, Blair
> what do you think?

Agreed.

	Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Comment 26 • 14 years ago

When we are talking about the description, which one is causing this problem?  The one under <em:name>, right? Dave, when I would create a sample XPI, is there a way to make that extension compatible with any future version of Firefox? Setting maxVersion to something like 100 or is there a better approach?

	Robert Strong (they/them - no direct email) Assignee
	Comment 27 • 14 years ago

You could use Clean Fox or any persona that is missing a description and combine this with the litmus test for bug 562848. With Clean Fox it will not show up in the search results for "Clean" without this fix and will with this fix.

	Dave Townsend [:mossop]
	Comment 28 • 14 years ago

(In reply to comment #26)
> When we are talking about the description, which one is causing this problem? 
> The one under <em:name>, right? Dave, when I would create a sample XPI, is
> there a way to make that extension compatible with any future version of
> Firefox? Setting maxVersion to something like 100 or is there a better
> approach?

The one in <em:description> was the problem here.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 29 • 14 years ago

Want to try automating this after all.

	Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Comment 30 • 14 years ago

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100611 Minefield/3.7a6pre

Thanks Blair, I will unflag in-litmus for now. If you can't automate it, I will add a Mozmill test instead.