131230 - handle known image decoder mime types internally

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Peter Lubczynski]

We need to handle PNG's internally and not let plugins take over.

Need to take a look why nsObjectFrame::IsSupportedImage isn't working for PNGs
like it does GIFs and JPGs.

Comment 1

[rbs]

mng too?

Comment 2

[Peter Lubczynski]

Attachment: possible fix?

Here's a patch that prevents plugins from taking over full-page mode for mime
types that we have image decoders.

This patch comes at a high cost to me as I accidentally nuked my registry and
lost my profiles :(

Grepping my new registry, these look like the type that will be overriden:

image/x-icon
image/bmp
image/gif
image/icon
image/jpeg
video/x-mng
image/x-jng
image/png
image/x-png
image/x-portable-pixmap
image/inspector-bitmap

Does anyone have testcases for any of the above types?

Comment 3

[av (gone)]

Comment on attachment 74468 [details] [diff] [review]
possible fix?

No wonder IsSupportedImage does not work -- this is full-page mode :) Although
the patch seems to help, we should probably make this mechanism more flexible
when we come to implementing plugin/mimetype manager.
r=av.

Comment 4

[Matthias Versen [:Matti]]

*** Bug 131844 has been marked as a duplicate of this bug. ***

Comment 5

[Scott Collins]

You would probably do less work with something of this form

  nsCAutoString decoderId = NS_LITERAL_CSTRING("...")
                          + nsDependentString(pluginTag->mMimeTypeArray[i]);

in case the total length goes over the size limit of an auto string.  If it is
impossible to excede this length, then your code is fine.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Please wait with this patch until the patch for bug 104906 is in. It adds
functions to imgILoader for checking if an image type is supported, e.g.:

PRBool isReg;
nsCOMPtr<imgILoader> loader(do_GetService("@mozilla.org/image/loader;1"));
loader->SupportImageWithMimeType(pluginTag->mMimeTypeArray[i], &isReg);

Comment 7

[av (gone)]

Nominating nsbeta1+ per weekly plugin meeting.

Comment 8

[Winnie Lam]

adding in ADT2 as per discussion with beppe.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

patch for bug 104906 is in, removing dependency

Comment 10

[Peter Lubczynski]

Attachment: patch using |SupportImageWithMimeType| and adding pref

Here's a better patch to use the new |SupportImageWithMimeType|. Just in case
someone ever wants to write a super-duper PNG plugin, this behavior is now
controled by a pref, "plugin.override_internal_types". When set to TRUE, the
old behavior of allowing plugins to override image decoder mime types will be
present. You must restart the browser for the pref to take effect.

Please review/comment.

Comment 11

[av (gone)]

Maybe it would make sense to check this pref not only on start up but also when
we refresh plugins? Plugin may access this pref during installation process and
ask user if he wants to reconfigure the browser. As an additional benefit there
will be no need for |mOverrideInternalTypes|. I understand that accessing prefs
is kind of expensive but this will not happened often, and besides, I feel that
any prefs related to plugins in general case should be updated with any
potential plugin change, i.e. plugins.refresh.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 75968 [details] [diff] [review]
patch using |SupportImageWithMimeType| and adding pref

r=biesi

Comment 13

[Peter Lubczynski]

There are bugs preventing this pref from being set dynamically! It doesn't
appear that the document loader factories are ever unregistered for either
plugins or image loaders. It doesn't work to set the pref at run-time when the
code is arranged to do it dynamically. We continue to use the plugin or imglib
until a restart.

Comment 14

[av (gone)]

Comment on attachment 75968 [details] [diff] [review]
patch using |SupportImageWithMimeType| and adding pref

OK, that explains special treatment for this case, r=av

Comment 15

[Patrick C. Beard]

Comment on attachment 75968 [details] [diff] [review]
patch using |SupportImageWithMimeType| and adding pref

sr=beard

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 75968 [details] [diff] [review]
patch using |SupportImageWithMimeType| and adding pref

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 17

[Peter Lubczynski]

patch in trunk, marking FIXED

Comment 18

[shrirang khanzode]

awesome, fixed on trunk 0326 (win, mac9x, OSx)

Comment 19

[Greg K.]

Confirmed, PNG files do appear to be handled internally by Mozilla rather than
by the QT plug-in (with QT set to not handle them) using FizzillaCFM/2002032915.