397335 - When I want to modify an option in "application" tabs, it is not saved and previous action is kept

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Frederic Bezies]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a9pre) Gecko/2007092411 Firefox/3.0a9pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a9pre) Gecko/2007092411 Firefox/3.0a9pre

Simple to see. Use last nightly, to to application tab in preference box.

Select a mimetype and modify it. Click on another mimetype. Modified action is not saved.

Reproducible: Always

Steps to Reproduce:
1.See details
2.
3.
Actual Results:  
Mimetype action changed is not keeped.

Expected Results:  
Mimetype action changed keeped.

Comment 1

[Frederic Bezies]

Got this error in error console :

Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]

Hope it helps.

Comment 2

[Frederic Bezies]

Related to bug 393492 in some way ? Because when I entered "nsIHandlerService::getTypeFromExtension" on lxr.mozilla.org search bos for seamonkey, it is the only place it could be find...

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

You're correct that bug 393492 is what caused the warnings to start showing up.  The fix for that bug exposed another bug in XPConnect, bug 393627.  That's probably unrelated to this bug.

This bug I see on Mac also.  Interestingly, selecting a MIME-type that's handled by a plugin and then choosing an application does not set the picker to that application.  However, it does add that application to the list of possible handlers.  After that, I can select the application from the list of possible handlers and have it stick.  This particular use case may not be interesting, but it is a data point.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

I'm having trouble reproducing this, except for the feed type, which is bug 395250.  Can you provide an example of a MIME type handled a plugin?  In particular, I'm curious to know what the dropdown menu displays when you select the application and then what the richlistitem displays as the "Application" when you select a different type.

Comment 5

[Frederic Bezies]

I had installed mozilla-mplayer for mp3 and other video / sound files.

For mp3, I have :

Audio MP3 - MPlayer Plugin 3.31 (in Firefox) => first choice and default one.

Dropdown list : 

* MPlayer Plugin 3.31 (in Firefox)
* Video Player
* Choose an application

Only "choose an application" fails. Other choice is kept. Strange !

And when I switch to a different type, default application is still chosen.

And this in "error console" :

Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]

I don't understand what's happening :(

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes problem

The "Component is not available" messages are unrelated to this bug.

The problem is that chooseApp() doesn't update state correctly when you choose an application.  The solution is to have chooseApp merely set a possible handler and then call onSelectAction to update the datastore the same way it gets updated when you select an application from the menu manually.

Here's a patch that does that.  In order to make this work, I had to make feedHandlerInfo::possibleApplicationHandlers mutable and do some refactoring.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: unrotted

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: unrotted

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 283463 [details] [diff] [review]
patch v3: unrotted

>Index: browser/components/preferences/applications.js

>+function ArrayEnumerator(aItems) {
>+  this._index = 0;
>+  if (aItems) {
>+    for (var i = 0; i < aItems.length; ++i) {
>+      if (!aItems[i])
>+        aItems.splice(i, 1);
>+    }
>+  }

I'm not sure I understand why this is needed, but you could use aItems.filter(function (i) !!i); instead.

>   get possibleApplicationHandlers() {

>-    if (preferredAppFile && preferredAppFile.exists()) {
>+    if (preferredAppFile) {

Why this change?

>+    // If the user picked a new app from the menu, select it.
>+    if (handlerApp) {
>+      let typeItem = this._list.selectedItem;
>+      let actionsMenu =
>+        document.getAnonymousElementByAttribute(typeItem, "class", "actionsMenu");
>+      let menuItems = actionsMenu.firstChild.childNodes;

Use actionsMenu.menupopup.childNodes ?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: addresses review comments

(In reply to comment #9)
> (From update of attachment 283463 [details] [diff] [review])
> >Index: browser/components/preferences/applications.js
> 
> >+function ArrayEnumerator(aItems) {
> >+  this._index = 0;
> >+  if (aItems) {
> >+    for (var i = 0; i < aItems.length; ++i) {
> >+      if (!aItems[i])
> >+        aItems.splice(i, 1);
> >+    }
> >+  }
> 
> I'm not sure I understand why this is needed, but you could use
> aItems.filter(function (i) !!i); instead.

I copied ArrayEnumerator verbatim from helperApps.js, but on second glance I don't see why this is needed either.  I could imagine it being useful for arrays containing undefined elements that shouldn't be enumerated over, but then the constructor should check for definedness, not truth.

In any case, we aren't enumerating over arrays with undefined elements, and the ArrayConverter module being introduced over in bug 380839 doesn't perform this check, so it seems unnecessary, and I've removed it.


> >   get possibleApplicationHandlers() {
> 
> >-    if (preferredAppFile && preferredAppFile.exists()) {
> >+    if (preferredAppFile) {
> 
> Why this change?

The getter's caller (rebuildActionsMenu) calls isValidHandlerApp on each element of the list, which checks to make sure the file exists, so the "exists" check here is redundant.  Besides removing the redundancy, this also makes the feed type's possibleApplicationHandlers property behave more like that property for other types, none of which ensure that the entries in their lists exist.


> >+    // If the user picked a new app from the menu, select it.
> >+    if (handlerApp) {
> >+      let typeItem = this._list.selectedItem;
> >+      let actionsMenu =
> >+        document.getAnonymousElementByAttribute(typeItem, "class", "actionsMenu");
> >+      let menuItems = actionsMenu.firstChild.childNodes;
> 
> Use actionsMenu.menupopup.childNodes ?

Yup, good catch.  I have made this change, and I also changed one other reference to menu.firstChild into a reference to menu.menupopup.

This is the version of the patch I'll check in.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.23; previous revision: 1.22
done
Checking in browser/components/preferences/handlers.xml;
/cvsroot/mozilla/browser/components/preferences/handlers.xml,v  <--  handlers.xml
new revision: 1.3; previous revision: 1.2
done

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Long fixed but verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050904 Minefield/3.0pre.