280798 - InstallTrigger checks http://foo.com/undefined when no IconURL is specified

Status: RESOLVED WORKSFORME

(Toolkit :: Add-ons Manager, defect)

Description

[Met - Martin Hassman]

For the Firefox and Thundebird there is recomended new XPI installation code.
see on <http://www.mozilla.org/projects/firefox/extensions/web-api.html>

It meand you can declare object:

var params = {
    "Bar": { URL: aEvent.target.href,
             IconURL: aEvent.target.getAttribute("iconURL"),
             toString: function () { return this.URL; }
    }

But if you doesn't include declaration of IconURL (which means it is undefined),
Firefox still genererate 404 request to the server for icon on the URL like
<http://server.cz/download/undefined>.

There should be some check and if IconURL == undefined Firefox should not try to
load the icon.

Old code InstallTrigger.install({'name': URL}) has not this problem.

Comment 1

[Nickolay_Ponomarev]

I don't see your point. On that page I see "You could also use the old style
parameter block ({ "Bar": aEvent.target.href }) if you wanted - and didn't have
an Icon to use for the confirmation dialog."

That's it. If you don't have an icon, use the old variant. If you have an icon,
use the new variant.

Comment 2

[David Majda]

Nickolay: "You could also use the old style
parameter block ({ "Bar": aEvent.target.href }) *if you wanted*" - what if I
don't want?

What you are basically saying is that it is OK that the new method is slightly
broken (I assume you consider described behavior broken, at least you didn't say
the opposite) as long as there exists some other way how to do things. But what
if the install code is generated mechanically (from the database of extensions,
for exapmle) and sometimes there will be icon and sometimes not? Should I use
two different install codes in this case (branch the HTML/JS writing code...)?
What if developer wants to remember/use only one way how to do the install?

Or, look form the other side: Can you please tell me some reason why the
solution Met proposed shouldn't be applied? Will it break something? Will it
make something worse? IMHO the opposite is true, it will make thing sligtly better.

I mean no offense, I'm just asking for reconsidering your resolution of this bug.

Comment 3

[Nickolay_Ponomarev]

"I assume you consider described behavior broken, at least you didn't say
the opposite"
I don't consider this behaviour broken. I don't think it's very hard to use two
different patterns when generating code. I don't say it shouldn't be applied, my
point is there's no reason (IMO) for it to be applied. I will reopen the bug if
you so desire, but I don't think it will be a priority.

Comment 4

[Nickolay_Ponomarev]

I believe it belongs to XPInstall component

Comment 5

[Daniel Veditz [:dveditz]]

This was assigned correctly. The xpinstall engine does nothing with the icon
argument, it's simply passed along to the front end.

Comment 6

[Bill Gianopoulos [:WG9s]]

(In reply to comment #0)
> For the Firefox and Thundebird there is recomended new XPI installation code.
> see on <http://www.mozilla.org/projects/firefox/extensions/web-api.html>
> 
> It meand you can declare object:
> 
> var params = {
>     "Bar": { URL: aEvent.target.href,
>              IconURL: aEvent.target.getAttribute("iconURL"),
>              toString: function () { return this.URL; }
>     }
> 
> But if you doesn't include declaration of IconURL (which means it is undefined),
> Firefox still genererate 404 request to the server for icon on the URL like
> <http://server.cz/download/undefined>.
> 
> There should be some check and if IconURL == undefined Firefox should not try to
> load the icon.
> 
> Old code InstallTrigger.install({'name': URL}) has not this problem.

Just to clarify the issue here, is what you are asking for is that it you use
the code above and the link does not include a definition for iconURL that it
should default to using chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png?

Comment 7

[Met - Martin Hassman]

> Just to clarify the issue here, is what you are asking for is that it you use
> the code above and the link does not include a definition for iconURL that it
> should default to using chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png?

Yes, xpinstallItemGeneric.png is a good idea.

Comment 8

[Robert Strong (they/them - no direct email)]

I just tried to reproduce this with committing the iconURL parameter and wasn't able to with a latest nightly. Met, can you check if this is still a problem?

Comment 9

[Robert Strong (they/them - no direct email)]

s/committing/omitting/

Comment 10

[Robert Strong (they/them - no direct email)]

When I tested this I wasn't able to reproduce it and I suspect one of the many em patches that have landed since this was reported fixed this. Resolving -> wfm - reporter, if you can reproduce this still please reopen or file a new bug. Thanks!