Bug 23818 - Fix mix of unicode and ansi functions in PluginDatabaseWin.cpp
Summary: Fix mix of unicode and ansi functions in PluginDatabaseWin.cpp
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 2000
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-07 08:05 PST by Adrien Nader
Modified: 2010-06-11 23:07 PDT (History)
2 users (show)

See Also:


Attachments
Use unicode functions everywhere and always (7.65 KB, patch)
2009-02-07 08:08 PST, Adrien Nader
ap: review-
Details | Formatted Diff | Diff
Let the compiler decide which function version use, but be consistent ! (11.08 KB, patch)
2009-02-08 09:44 PST, Adrien Nader
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien Nader 2009-02-07 08:05:51 PST
In WebCore/plugins/win/PluginDatabaseWin.cpp, there is currently a mix of unicode and ansi functions. That makes me think nobody ever tried to build that with UNICODE not defined (in the -DUNICODE sense).
Comment 1 Adrien Nader 2009-02-07 08:08:02 PST
Created attachment 27448 [details]
Use unicode functions everywhere and always

I decided to use the unicode functions everywhere and unconditionnaly. That should make it equivalent to what is available currently.
Comment 2 Adrien Nader 2009-02-07 08:09:17 PST
Forgot to say : I am aware that there is no changelog, I forgot it once again...
Comment 3 Darin Adler 2009-02-07 11:18:08 PST
Comment on attachment 27448 [details]
Use unicode functions everywhere and always

You meant to set this to review? -- asking for review. Not review+ which means it has been reviewed.
Comment 4 Alexey Proskuryakov 2009-02-08 02:23:04 PST
Comment on attachment 27448 [details]
Use unicode functions everywhere and always

This patch doesn't look right to me.  The functions you are renaming are not ANSI - e.g. SHGetValue is a macro defined to either SHGetValueA or SHGetValueW, depending on whether UNICODE is defined. The same is true of the TEXT macro.

I'm not a huge Windows expert, but I'm sufficiently sure about these macros' behavior to say r-. But please feel free to mark this for review again if I'm actually wrong.
Comment 5 Adrien Nader 2009-02-08 03:47:12 PST
(In reply to comment #4)
> (From update of attachment 27448 [details] [review])
> This patch doesn't look right to me.  The functions you are renaming are not
> ANSI - e.g. SHGetValue is a macro defined to either SHGetValueA or SHGetValueW,
> depending on whether UNICODE is defined. The same is true of the TEXT macro.
> 
> I'm not a huge Windows expert, but I'm sufficiently sure about these macros'
> behavior to say r-. But please feel free to mark this for review again if I'm
> actually wrong.
> 

Right, by "mix" I actually meant "potential mix", when UNICODE is not defined.

Of course it's possible to use the macros everywhere but not trying to restore ansi and only having unicode looked like the right choice. 

So the question is : do we want to restore the possibility to use the ansi version of these functions or should we go unicode-only ?
Comment 6 Alexey Proskuryakov 2009-02-08 04:09:26 PST
FWIW, building WebKit with UNICODE undefined doesn't sound like a worthy goal to me - and RegOpenKeyExW looks even noisier than RegOpenKeyEx. Maybe someone spending more time with Win32 code will have a different opinion though.
Comment 7 Adrien Nader 2009-02-08 09:44:26 PST
Created attachment 27466 [details]
Let the compiler decide which function version use, but be consistent !

The patch now uses the macros, not the functions or types directly.
That nearly makes it possible to compile the file using the ansi functions and types. However that is not possible (nor desirable) because two functions from webkit's String.cpp are used so we display a nice error stating : "You need UNICODE defined to compile this file and webkit in general".
Comment 8 Alexey Proskuryakov 2010-06-11 23:07:11 PDT
This patch has never been marked for review, so it got overlooked. I'm not sure if it's worth marking it for review as is now, or if it has bit rotted.