462508 - nsPrimitiveHelpers::CreateDataFromPrimitive will fail to return a string for most nsISupportsCString

Status: UNCONFIRMED

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P5)

Description

[Tomeu Vizoso]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008092510 Ubuntu/8.04 (hardy) Firefox/3.0.3
Build Identifier: xulrunner 1.9

Using nsIClipboardDragDropHooks, my embedding application would like to add a UTF-8 selection to an image that is being dragged away.

Unfortunately, nsPrimitiveHelpers::CreateDataFromPrimitive will only supply back an UTF-16-encoded string if nsIFlavorDataProvider returned a nsISupportString or an UTF-8-encoded string if nsIFlavorDataProvider returned a nsISupportCString _and_ the flavor is kTextMime.

Fro embedders to be able to add UTF-8-encoded selection values, I would like to change that so that if the nsIFlavorDataProvider returned a nsISupportCString, return an UTF-8-encoded string.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Tomeu Vizoso]

Attachment: don't fail to return a string if the string is nsISupportsCString but the flavor is not text/plain

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wouldn't it be simpler to completely ignore the flavor (and remove that parameter) and just check whether aPrimitive QIs to nsISupportsCString?

Comment 3

[Tomeu Vizoso]

(In reply to comment #2)
> Wouldn't it be simpler to completely ignore the flavor (and remove that
> parameter) and just check whether aPrimitive QIs to nsISupportsCString?

Would work for me, but that code is called from several platform-dependant places, so I'm not very comfortable making such big changes to mozilla.

I wonder if some nsIFlavorDataProvider could return something that implements both nsISupportsCString and nsISupportsString, in that case the flavor could be the hint that helps choose the best string type to return. But I'm just guessing here...

Regardless, I volunteer to do the grunt work if someone else can verify it's safe to do so.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems OK to me. Enndeakin, what do you think?

Comment 5

[Neil Deakin]

The callers of that method generally assume that text/plain returns single-byte and that text/unicode returns double-byte strings, and do static_casts on the results using that assumption. So the patch here I think isn't sufficient without fixing up the callers as well.

For now we should just do whatever is easiest. In the next round, I plan to convert the clipboard apis to use dataTransfer like I did for drag and drop, and then just have both use the dataTransfer directly, eliminating the tranferable and this nsPrimitiveHelpers class entirely.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks. Then I'm not sure what's the right thing to do here. Tomeu, what's your MIME type here?

Comment 7

[Tomeu Vizoso]

(In reply to comment #6)
> Thanks. Then I'm not sure what's the right thing to do here. Tomeu, what's your
> MIME type here?

Not really a mime type, we are adding some selections for passing metadata about the clipping that is displayed in Sugar's clipboard manager. We'd prefer these selections to be in UTF-8, as I understand that it's the preferred encoding for selections that contain text (we'd like these properties to be added to some spec in freedesktop.org some day).

If we need to maintain the contract that Neil explained above, then we may do the following:

if text/plain:
    QI to nsISupportsCString
    return single-byte string if available
elif text/unicode:
    QI to nsISupportsString
    return doublebyte string if available
elif QIs to nsISupportsCString
    return single-byte string
elif QIs to nsISupportsString
    return double-byte string

Sounds good?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds good to me. How about you Neil?

Comment 9

[Tomeu Vizoso]

ping!

Comment 10

[Neil Deakin]

It looks OK to me too, as long as the callers can deal with it.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 345702 [details] [diff] [review]
don't fail to return a string if the string is nsISupportsCString but the flavor is not text/plain

Minusing because I think we need to do what comment #7 says

Comment 12

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.