421652 - Installing webapp from extension should automatically fill in name

Status: RESOLVED FIXED

(Mozilla Labs :: Prism, enhancement)

Description

[u278084]

The title attribute of the <link/> tag is often a good name for the webapp. Currently the name is either left blank, or is the name of a previous webapp.

Comment 1

[u278084]

Attachment: version 1

pretty simple. I don't expect more than one webapp to be downloading at any one time. I think we should be fine, but I made a note of it in the source.

Comment 2

[Matthew Gertner]

Comment on attachment 312649 [details] [diff] [review]
version 1

It should use the name in the webapp.ini of the bundle. Doesn't that work?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)
> (From update of attachment 312649 [details] [diff] [review])
> It should use the name in the webapp.ini of the bundle. Doesn't that work?
>

Not if you don't have a webapp bundle. Like if you're just on a web page and say "Convert to Application"

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 312649 [details] [diff] [review]
version 1

Cesar - you can skip a lot of this code.

In "installWebApp" when you get the name of the webapp you could set the WebAppProperties.name property. That would keep it set until you display the dialog.

Note: There is another bug closely related to this code: If I manually attempt to create more than 1 webapp, the WebAppProperties object is not reset, so options I pick in the first webapp are displayed as default for the next webapp.

We should be clearing the WebAppProperties object.

I would r- this patch

Comment 5

[Matthew Gertner]

Comment on attachment 312649 [details] [diff] [review]
version 1

My understanding of the patch is that it is grabbing the name in a kind of quirky way from the link that points to the webapp bundle, which is why I think (in these cases at least) it would be much better to get the name from the bundle itself.

Comment 6

[u278084]

(In reply to comment #2)
> (From update of attachment 312649 [details] [diff] [review])
> It should use the name in the webapp.ini of the bundle. Doesn't that work?
> 
It should work. The reason why I went with the name from the title attribute is because many bundle names are usually short forms (eg. gmail, gcalendar) of their app names, and the title is usually more user friendly. At least, I would hope they were a more user friendly version.

But anyways, I can see the argument work both ways, and since we already do all the work to extract the app id, I suppose you do make a better point.

(In reply to comment #4)
> (From update of attachment 312649 [details] [diff] [review])
> In "installWebApp" when you get the name of the webapp you could set the
> WebAppProperties.name property. That would keep it set until you display the
> dialog.
> 

I thought about that, but thought about the situation where two webapps could be downloading and installing at once, and that would cause issues. But right now, webapps are small enough that it just isn't gonna happen. Plus, I make that assumption in my code anyways :)

> Note: There is another bug closely related to this code: If I manually attempt
> to create more than 1 webapp, the WebAppProperties object is not reset, so
> options I pick in the first webapp are displayed as default for the next
> webapp.
> 
> We should be clearing the WebAppProperties object.
> 
Yeah.

(In reply to comment #5)
> (From update of attachment 312649 [details] [diff] [review])
> My understanding of the patch is that it is grabbing the name in a kind of
> quirky way from the link that points to the webapp bundle, which is why I think
> (in these cases at least) it would be much better to get the name from the
> bundle itself.
> 

Ok, Fair enough.

Comment 7

[u278084]

Clearly, I'm too tired, what better time to write a 1500 word essay on fair trade?

If webapp.ini doesn't have the name key in Parameters, then it still ends up blank (or showing the name of previous webapp, as Mark said). That was the scenerio I was hitting so often, I thought it ignored the name key ;)
So maybe we should use link title as a fallback method? I can remove a lot of the code that I have written and just assume one webapp can be installed at any one time.

We can also use the document title if user is using convert to application.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> Clearly, I'm too tired, what better time to write a 1500 word essay on fair
> trade?

There is never a good time.

> So maybe we should use link title as a fallback method? I can remove a lot of
> the code that I have written and just assume one webapp can be installed at any
> one time.
> 
> We can also use the document title if user is using convert to application.
> 

Yes, both of these ideas sound good to me.

Comment 9

[Matthew Gertner]

Agreed, the document title is the way to go.

Comment 10

[Matthew Gertner]

Attachment: Set webapp name from document title

I do a little bit of regexp on the title since for the sites I checked there is often some additional text after a dash (e.g. Gmail) or bar (e.g. Facebook) or some other punctuation. The regexp can be refined over time if necessary. I select the name in the install shortcut dialog so the user can change it right away or tab/click away if they are happy with the default value.

This also fixes https://bugzilla.mozilla.org/show_bug.cgi?id=469321. I took the liberty of fixing a few nits in install.rdf and adding an ellipsis to the menu item.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 353072 [details] [diff] [review]
Set webapp name from document title

+				<em:maxVersion>3.0.*</em:maxVersion>

Use the highest allowed max on AMO (3.1b3pre ??)

Comment 12

[Matthew Gertner]

I went ahead and committed with 3.0.* as maxversion. Are we absolutely sure we work properly on trunk? If so, we can update install.rdf appropriately in a separate patch.

Sending        extension/chrome/content/refractor-overlay.js
Sending        extension/chrome/locale/en-US/refractor-overlay.dtd
Sending        extension/install.rdf
Sending        newapp/chrome/content/install-shortcut.js
Transmitting file data ....
Committed revision 21452.