Bug 23818 – Fix mix of unicode and ansi functions in PluginDatabaseWin.cpp

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Platform

[Adrien Nader]

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).

[Adrien Nader]

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.

[Adrien Nader]

Forgot to say : I am aware that there is no changelog, I forgot it once again...

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Adrien Nader]

(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 ?

[Alexey Proskuryakov]

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.

[Adrien Nader]

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".

[Alexey Proskuryakov]

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.

[Ahmad Saleem]

PluginDatabaseWin.cpp was removed from WebkitLegacy based on following commit:

https://github.com/WebKit/WebKit/commit/6436208aea2068805937c16a62cf97b57503e33f

It does not exist in "Webkit" main source code.

Further NPAPI and Webkit Plugin support has been removed from Safari and WebkitGTK builds. Can this be marked as "RESOLVED WONTFIX"? Thanks!