395182 - multiple content-types listed in content-handling pref ui

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Steve England [:stevee]]

Attachment: multiple entries for the same content-type

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007090602 Minefield/3.0a8pre ID:2007090602

Check out the screenshot. There are multiple entries for mp3, jp2, etc.

Comment 2

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

I think these are entries for variations on a content type rather than duplicate entries.  In the long run, this might be best addressed by the fix for bug 395645.  In the short run, this should be addressed by the fix for bug 395646, which is essentially a duplicate of this bug, so I have duped it to this one.

Comment 3

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

Attachment: patch v1: fixes problem

This patch fixes the problem by adding the content type in parentheses after the type description for types with duplicate names, i.e.:

Unique Name
Duplicate Name (type/one)
Duplicate Name (type/two)
Another Unique Name

It works by counting the number of times each description appears in the _handledTypeDescriptions hash and then describing each type via the bundle string |%S (%S)| (where the first %S is the description and the second %S is the type) when _handledTypeDescriptions[type] is greater than one.

Adding the type via a bundle string allows localizers to localize this using some other technique besides putting the type in parentheses.  In addition to the display code, I also updated the filtering and sorting code to use the annotated description, when present.

In the process, I factored out the common code in the _load*Handler* methods into an _addHandledType method.

Comment 4

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

Attachment: screenshot: annotated JPEG and PDF types

Here's a screenshot showing annotated JPEG and PDF types.

I've also noted one flaw in the patch: it annotates types before checking if they're visible, so if there are two types with the same description, but one is invisible, the other will be annotated even though only one type with that description appears.

That's the case for the type described as "Microsoft PowerPoint Viewer (PPVIEW32.EXE)" in the screenshot, which is why that type has an annotation next to it.  Extra annotations isn't terrible, but it's suboptimal.  I'll look into what needs to be done to fix that.

Comment 5

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

Comment on attachment 280517 [details] [diff] [review]
patch v1: fixes problem

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

>-  // Plugin Handling
>-
>-  handledOnlyByPlugin: false,

Why remove this? 

>+        let wrappedHandlerInfo =
>+          this._mimeSvc.getFromTypeAndExtension(type, null);
>+        let handlerInfoWrapper =
>+          this._addHandledType(type, wrappedHandlerInfo, true);

You could avoid calling getFromTypeAndExtension if (type in this._handledTypes), but maybe that's not worth optimizing?

>Index: browser/locales/en-US/chrome/browser/preferences/preferences.properties

>+typeDescriptionWithType=%S (%S)

A note to localizers about what each parameter represents would be useful, especially given the lack of context. See e.g. http://mxr.mozilla.org/seamonkey/source/browser/locales/en-US/chrome/browser/search.properties#15

Comment 6

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

Attachment: patch v2: fixes review comments, bug in comment 4; improves perf

(In reply to comment #5)
> (From update of attachment 280517 [details] [diff] [review])
> >Index: browser/components/preferences/applications.js
> 
> >-  // Plugin Handling
> >-
> >-  handledOnlyByPlugin: false,
> 
> Why remove this? 

Now that _addHandledType sets the handledOnlyByPlugin property, and _loadFeedType calls _addHandledType to register the feed type, we don't need to specify handledOnlyByPlugin in the feedHandlerInfo object.  The object can just inherit that property from its prototype (an instance of HandlerInfoWrapper) and have it get set when the data gets loaded.


> >+        let wrappedHandlerInfo =
> >+          this._mimeSvc.getFromTypeAndExtension(type, null);
> >+        let handlerInfoWrapper =
> >+          this._addHandledType(type, wrappedHandlerInfo, true);
> 
> You could avoid calling getFromTypeAndExtension if (type in
> this._handledTypes), but maybe that's not worth optimizing?

Actually, I think you are right that it is worth optimizing, since getFromTypeAndExtension is an XPCOM call, and we might otherwise call it many more times than necessary if there is plugin contention, i.e. two or more plugins that claim to handle the same set of types, f.e. QuickTime and Windows Media Player both claiming to handle the same bunch of audio and video types.

So I've changed back to only calling it if we haven't already retrieved a handler info object for the given type.  Changing this back required de-refactoring much of the code I'd refactored into _addHandledType, however, and in the end it didn't seem worth it to keep that method for the small amount of code I could leave in it, so I've removed it.

But I do continue to have _loadFeedType set handledOnlyByPlugin, just now it sets it directly instead of calling _addHandledType to set it.

Note: in the original patch, I added a comment saying that _loadPluginHandlers must be called before _loadApplicationHandlers.  That's no longer true now that I only set handledOnlyByPlugin to true in _loadPluginHandlers when creating a new wrapper, so I've removed that comment.

Nevertheless, the code does still call the two methods in the same order, so nothing has changed about the current behavior, the code is just a bit more robust against future changes.


> >Index: browser/locales/en-US/chrome/browser/preferences/preferences.properties
> 
> >+typeDescriptionWithType=%S (%S)
> 
> A note to localizers about what each parameter represents would be useful,
> especially given the lack of context. See e.g.
> http://mxr.mozilla.org/seamonkey/source/browser/locales/en-US/chrome/browser/search.properties#15

Good idea, added.


This version also fixes the bug I noted in comment 4.  In order to fix this bug, I had to rearrange how the controller maintains the list of visible types and rebuilds the view when it changes.

The controller used to rebuild the list of types to show and then rebuild the view whenever a preference, filter, or sort column/order changed, and it would filter and sort the list every time it rebuilt it.

But some criteria for determining if an item should be visible (f.e. whether or not "always ask" is set) never change, and others (f.e. the preference for whether or not to show plugins in the list) rarely change.  Also, a sort change doesn't affect which types are visible, and a filter change doesn't affect sort order.

So now the controller builds a preliminary list of "visible" types (where "visible" means "might be shown", i.e. meeting all the criteria that don't change very often or at all, but subject to possible further paring by the filter) just once after loading the data and every time a preference changes.

It then sorts that list only after rebuilding it and every time the sort column/direction changes.  And it filters the list only when rebuilding the view.

So if a preference changes, then everything happens: the list gets rebuilt, resorted, and refiltered, and the view gets rebuilt.  But if a sort column/direction changes, the list only gets resorted and refiltered (and the view rebuilt).  And if the filter changes, the list only gets refiltered (and the view rebuilt).

We then rebuild the count of duplicate descriptions after we rebuild the list (but before sorting, filtering, and displaying it, so all those activities act upon the annotated descriptions), so only visible types count when it comes to determining which types have duplicate descriptions.

There's more optimization work we could do here (f.e. we could resort the existing view items on sort changes instead of resorting the list of types and then refiltering it), but I think what we have here is probably sufficient.

This patch has changed significantly since the one reviewed earlier, so it needs re-review.

Comment 7

[Steffen Wilberg]

Could you post a screenshot of how it looks like on Windows with two dozen or so audio and video file types with identical content types like "Winamp media file" (cf. bug 395381 comment 9)?

Might consider adding subtle borders like in the add-ons manager between the groups.

Comment 8

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

Attachment: patch v3: unrotted

Comment 9

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

Requesting blocking-firefox3 for this Applications prefpane regression fix.

Comment 10

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

Attachment: patch v4: unrotted

Comment 11

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

Attachment: patch v5: unrotted

Comment 12

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

Attachment: patch v6: unrotted

Comment 13

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

Attachment: patch v7: unrotted

Comment 14

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

Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.22; previous revision: 1.21
done
Checking in browser/locales/en-US/chrome/browser/preferences/preferences.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/preferences.properties,v  <--  preferences.properties
new revision: 1.20; previous revision: 1.19
done

Comment 15

[Masahiro YAMADA]

It is impossible to see content-type on some locales and OS.
Because some MIME type name and Plug-in name is too long.
See bug 399539.