port some parts of webkit/glue/plugins/test to Linux...

port some parts of webkit/glue/plugins/test to Linux
these are not all parts (about a half), but still something

BUG=1949

[please use my chromium address, 2008-09-09 12:57:41]

I'm not sure about building this on Windows. There are places where I check for
defined(OS_WIN), but I'm not sure whether it will be defined there even when on
Windows.

With this patch about half of webkit/glue/plugins/test builds on Linux.

[Nicolas Sylvain, 2008-09-09 19:49:44]

Adding reviewers.

Amanda: to check if it does not break mac
evan: to review linux stuff
sgk: to review the SConscript file changes.

[Evan Martin, 2008-09-09 20:24:24]

I think you should use the functions in base::string_util rather than the
#defines here.  I guess you might need to add calls to strlen() so you can use
strncasecmp(), but that is probably acceptable.

http://codereview.chromium.org/1661/diff/1/6
File webkit/glue/plugins/test/SConscript (right):

http://codereview.chromium.org/1661/diff/1/6#newcode26
Line 26: if env['PLATFORM'] == 'win32':
Can you put this list of input files directly under the one on L10?  It'll mean
you'll have two tests for env['PLATFORM'] but it's nice to keep the "needs to be
ported" list separate from the "definitely will never work" list (like the
Windows-specific CFLAGS below).

http://codereview.chromium.org/1661/diff/1/8
File webkit/glue/plugins/test/npapi_test.cc (right):

http://codereview.chromium.org/1661/diff/1/8#newcode47
Line 47: #define WINAPI
I think this part conflicts with
http://codereview.chromium.org/1662
maybe you need to update and re-uplaod?

[please use my chromium address, 2008-09-10 09:26:53]

On 2008/09/09 20:24:24, Evan Martin wrote:
> I think you should use the functions in base::string_util rather than the
> #defines here.  I guess you might need to add calls to strlen() so you can use
> strncasecmp(), but that is probably acceptable.

After consultations on #chromium-dev I added strcasecmp to base/string_utils.

I'm also somewhat worried about #define PtrToUlong I added in
plugin_get_javascript_url_test.cc - probably there's much better way to do it.

There's also #define BYTE in plugin_test.cc which could be moved into something
like windows_compat.h (name made up as an example).

> http://codereview.chromium.org/1661/diff/1/6#newcode26
> Line 26: if env['PLATFORM'] == 'win32':
> Can you put this list of input files directly under the one on L10?

Done.

> http://codereview.chromium.org/1661/diff/1/8#newcode47
> Line 47: #define WINAPI
> I think this part conflicts with
> http://codereview.chromium.org/1662
> maybe you need to update and re-uplaod?

Updated (to revision 1981) and got no merge conflicts, and it still builds fine
(on Linux at least). WINAPI is still there, and
http://codereview.chromium.org/1662 touched only default_plugin. I'm not sure
what to do about that.

[Mark Mentovai, 2008-09-10 16:15:09]

http://codereview.chromium.org/1661/diff/212/222
File webkit/glue/plugins/test/npapi_test.cc (right):

http://codereview.chromium.org/1661/diff/212/222#newcode44
Line 44: #if defined(OS_WIN)
OS_WIN is not defined at this point.  You would need an early #include of
"build/build_config.h".  But I'm not sure if we should be using OS_WIN in here,
maybe ask around.

http://codereview.chromium.org/1661/diff/212/222#newcode47
Line 47: #define WINAPI
What we really need is something for OS callbacks (say, called OSCallback)
that's WINAPI on Windows and nothing on other platforms.  We should probably add
this to base somewhere, and if we can depend on base here, use it.  We shouldn't
just be defining WINAPI as empty on non-Windows.

http://codereview.chromium.org/1661/diff/212/223
File webkit/glue/plugins/test/plugin_arguments_test.cc (right):

http://codereview.chromium.org/1661/diff/212/223#newcode5
Line 5: #if defined(OS_WIN)
Same here.

http://codereview.chromium.org/1661/diff/212/217
File webkit/glue/plugins/test/plugin_client.cc (right):

http://codereview.chromium.org/1661/diff/212/217#newcode5
Line 5: #if !defined(OS_WIN)
And here.

http://codereview.chromium.org/1661/diff/212/217#newcode6
Line 6: #define HIBYTE(x) ((unsigned char)(((x) >> 8) & 0xff))
I wouldn't do this, it relies on x being a 16-bit type but doesn't enforce that.
 This macro is used exactly once in this file, I'd just take it out entirely and
write the shift-and-mask below in the code.

http://codereview.chromium.org/1661/diff/212/217#newcode103
Line 103: } else if (base::strcasecmp(argv[name_index],
"execute_script_delete_in_paint") == 0) {
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Line_L...

http://codereview.chromium.org/1661/diff/212/217#newcode113
Line 113: } else if (base::strcasecmp(argv[name_index],
"self_delete_plugin_stream") == 0) {
Line length again.

http://codereview.chromium.org/1661/diff/212/217#newcode116
Line 116: } else if (base::strcasecmp(argv[name_index],
"npobject_lifetime_test") == 0) {
And again.

http://codereview.chromium.org/1661/diff/212/217#newcode120
Line 120: "npobject_lifetime_test_second_instance") == 0) {
Be careful here, the wrapped continuation line used to line up with the line
above it and now it doesn't.

http://codereview.chromium.org/1661/diff/212/217#newcode127
Line 127: "npobject_delete_plugin_in_evaluate") == 0) {
Same.

http://codereview.chromium.org/1661/diff/212/217#newcode276
Line 276: 
There are four blank lines at the end of the file that don't need to be here. 
You can kill them.

http://codereview.chromium.org/1661/diff/212/221
File webkit/glue/plugins/test/plugin_get_javascript_url_test.cc (right):

http://codereview.chromium.org/1661/diff/212/221#newcode19
Line 19: #if !defined(OS_WIN)
...which may not be defined...

http://codereview.chromium.org/1661/diff/212/221#newcode20
Line 20: #define PtrToUlong(x) (reinterpret_cast<unsigned long>((x)))
Rather than doing a macro, just write the cast where we need it.

Macros suck,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
.

http://codereview.chromium.org/1661/diff/212/224
File webkit/glue/plugins/test/plugin_npobject_proxy_test.cc (right):

http://codereview.chromium.org/1661/diff/212/224#newcode5
Line 5: #if defined(OS_WIN)
I think you know what's up here...

http://codereview.chromium.org/1661/diff/212/224#newcode7
Line 7: #include <strsafe.h>
I don't see why this header (or the macro above it) is needed even on Windows,
but unless you can test on Windows, it's safest to leave.

http://codereview.chromium.org/1661/diff/212/218
File webkit/glue/plugins/test/plugin_test.cc (right):

http://codereview.chromium.org/1661/diff/212/218#newcode9
Line 9: #define BYTE unsigned char
We should just use unsigned char in the code then.  It's only used once.

http://codereview.chromium.org/1661/diff/212/219
File webkit/glue/plugins/test/plugin_test.h (right):

http://codereview.chromium.org/1661/diff/212/219#newcode15
Line 15: #define sprintf sprintf_s
Instead of doing this, use StringPrintf or SStringPrintf from base.

[please use my chromium address, 2008-09-10 18:56:17]

On 2008/09/10 16:15:09, Mark Mentovai wrote:
> http://codereview.chromium.org/1661/diff/212/222
> File webkit/glue/plugins/test/npapi_test.cc (right):
> 
> http://codereview.chromium.org/1661/diff/212/222#newcode44
> Line 44: #if defined(OS_WIN)
> OS_WIN is not defined at this point.  You would need an early #include of
> "build/build_config.h".  But I'm not sure if we should be using OS_WIN in
here,
> maybe ask around.

Solved it by re-ordering headers (so that build/build_config.h gets included on
the way). I asked on #chromium-dev and there seems to be nothing wrong with
OS_WIN here (provided build/build_config.h is included - I checked this).

> http://codereview.chromium.org/1661/diff/212/222#newcode47
> Line 47: #define WINAPI
> What we really need is something for OS callbacks (say, called OSCallback)
> that's WINAPI on Windows and nothing on other platforms.  We should probably
add
> this to base somewhere, and if we can depend on base here, use it.  We
shouldn't
> just be defining WINAPI as empty on non-Windows.

I changed that to API_CALL from base/port.h. See
http://codereview.chromium.org/1662 for a similar case.

> http://codereview.chromium.org/1661/diff/212/217#newcode6
> Line 6: #define HIBYTE(x) ((unsigned char)(((x) >> 8) & 0xff))
> I wouldn't do this, it relies on x being a 16-bit type but doesn't enforce
that.
>  This macro is used exactly once in this file, I'd just take it out entirely
and
> write the shift-and-mask below in the code.

Taken out, still with a conditional define for Windows, because I'm not very
much confident about it (will probably need to be corrected). But as far as I've
seen it's done just like that on Windows. HIBYTE might be slightly more
descriptive.

> http://codereview.chromium.org/1661/diff/212/217#newcode120
> Line 120: "npobject_lifetime_test_second_instance") == 0) {
> Be careful here, the wrapped continuation line used to line up with the line
> above it and now it doesn't.

I'm not sure if I understood this correctly. Please check.

Other issues fixed as well (I can't test on Windows so left strsafe.h and
friends in place, for safety).

[Mark Mentovai, 2008-09-10 20:16:09]

This is in much better shape.  I still haven't really focused too much on the
SConscript changes, I'd like Steven or Evan or someone else to comment on those.

http://codereview.chromium.org/1661/diff/18/251
File webkit/glue/plugins/test/npapi_test.cc (right):

http://codereview.chromium.org/1661/diff/18/251#newcode46
Line 46: #if defined(OS_WIN)
I would advise #including "base/basictypes.h" in this file anyway. 
plugin_client.h doesn't #include anything directly that makes it obvious that
this should work, which makes it a little more likely that some day, someone
will take whatever #include we rely on here out of the header that provides it
for us.  I appreciate what you've done with the #include reordering, but it's
still not obvious where the right definitions come from, and since we're only
gating out OS_WIN here, it's even more likely that when someone changes another
header in a way that drops OS_WIN from Windows, they'll only test on Linux and
not realize they're breaking the Windows build because someone had depended on
their #include when they should have provided their own.

Our general philosophy is "include what you use" in each file, which is why
we've structured our #include ordering requirements the way they are. 
Personally, I'm OK with relaxing that rule for cases where foo.cc picks up
includes from foo.h, but even that's contentious.  Certainly in this case, I
think that the connection to base/port.h via webkit/glue/plugins/nphostapi.h via
webkit/glue/plugins/test/plugin_client.h is too tenuous.

For that matter, in this file, <windows.h> should precede plugin_client.h, so
I'd do:

#include "base/basictypes.h"

#if defined(OS_WIN)
#include <windows.h>
#endif

#include "webkit/glue/plugins/test/plugin_client.h"

We make an exception for base/basictypes.h or build/build_config.h and allow
them to come before system includes when we need to access the OS_* macros for
conditional system header inclusion.

http://codereview.chromium.org/1661/diff/18/252
File webkit/glue/plugins/test/plugin_arguments_test.cc (right):

http://codereview.chromium.org/1661/diff/18/252#newcode7
Line 7: #if defined(OS_WIN)
...here too still...

http://codereview.chromium.org/1661/diff/18/246
File webkit/glue/plugins/test/plugin_client.cc (right):

http://codereview.chromium.org/1661/diff/18/246#newcode54
Line 54: #if defined(OS_WIN)
Don't #ifdef this, we can use the shift-and-mask on all platforms.

http://codereview.chromium.org/1661/diff/18/246#newcode57
Line 57: if (static_cast<unsigned char>((pFuncs->version >> 8) & 0xff) >
NP_VERSION_MAJOR) {
And watch the line length.

http://codereview.chromium.org/1661/diff/18/246#newcode105
Line 105: "execute_script_delete_in_paint") == 0) {
Yup, this is what I meant.  Thanks.

http://codereview.chromium.org/1661/diff/18/253
File webkit/glue/plugins/test/plugin_npobject_proxy_test.cc (right):

http://codereview.chromium.org/1661/diff/18/253#newcode5
Line 5: #include "webkit/glue/plugins/test/plugin_npobject_proxy_test.h"
...here too still...

http://codereview.chromium.org/1661/diff/18/248
File webkit/glue/plugins/test/plugin_test.h (right):

http://codereview.chromium.org/1661/diff/18/248#newcode82
Line 82: SStringPrintf(&buf, "%d", val1);
You know, looking at this, what we really want is IntToString.  Sorry to steer
you in the wrong direction with SStringPrintf, but it was a long review and I
hadn't actually looked at the format string.

[Evan Martin, 2008-09-10 21:49:37]

The SConscript bits look good to me; I only have trivial style things which you
may as well change while you're looking at Mark's more substantial comments.

http://codereview.chromium.org/1661/diff/18/249
File webkit/glue/plugins/test/SConscript (right):

http://codereview.chromium.org/1661/diff/18/249#newcode21
Line 21: # to be ported
We generally use capitals and periods on comments.

http://codereview.chromium.org/1661/diff/18/249#newcode41
Line 41: ])
Can you line the closing paren here and elsewhere with the line it opened from? 
So here, it'd be two spaces.

[please use my chromium address, 2008-09-11 08:30:01]

On 2008/09/10 20:16:09, Mark Mentovai wrote:
> http://codereview.chromium.org/1661/diff/18/251#newcode46
> Line 46: #if defined(OS_WIN)
> I would advise #including "base/basictypes.h" in this file anyway. 

Done (in all such cases).

> http://codereview.chromium.org/1661/diff/18/248#newcode82
> Line 82: SStringPrintf(&buf, "%d", val1);
> You know, looking at this, what we really want is IntToString.  Sorry to steer
> you in the wrong direction with SStringPrintf, but it was a long review and I
> hadn't actually looked at the format string.

Corrected, code looks clearer now. Thanks for patience.

Other issues fixed too.

On 2008/09/10 21:49:37, Evan Martin wrote:
> http://codereview.chromium.org/1661/diff/18/249#newcode21
> Line 21: # to be ported
> We generally use capitals and periods on comments.

Fixed. I also transformed it to TODO, so it can be found easier.

> http://codereview.chromium.org/1661/diff/18/249#newcode41
> Line 41: ])
> Can you line the closing paren here and elsewhere with the line it opened
from? 
> So here, it'd be two spaces.

Done. The same in some other place in the file.

[Evan Martin, 2008-09-16 17:19:57]

Is this ready for commit?  (I'm looking through old reviews.)

[Mark Mentovai, 2008-09-16 17:53:36]

LGTM

http://codereview.chromium.org/1661/diff/269/409
File webkit/glue/plugins/test/plugin_get_javascript_url_test.cc (right):

http://codereview.chromium.org/1661/diff/269/409#newcode40
Line 40: unsigned long stream_id = reinterpret_cast<unsigned
long>(stream->notifyData);
Let's add:

#include "base/basictypes.h"

and:

COMPILE_ASSERT(sizeof(unsigned long) <= sizeof(stream->notifyData));

and similar at each cast point in this file.

http://codereview.chromium.org/1661/diff/269/406
File webkit/glue/plugins/test/plugin_test.cc (right):

http://codereview.chromium.org/1661/diff/269/406#newcode37
Line 37: return x > 9 ? x + 55: x + 48;
Existing code, but I think this is far less obtuse as:

return x > 9 ? (x + 'A' - 10) : (x + '0');

and the indent should be two spaces.

[please use my chromium address, 2008-09-16 19:02:38]

On 2008/09/16 17:53:36, Mark Mentovai wrote:
> LGTM

Corrected issues you pointed out. Some comments about other changes:

- changed TODO: to TODO(port): in SConscript as in other changesets
- some parts of http://codereview.chromium.org/2418 got merged to
webkit/glue/plugins/test/plugin_npobject_proxy_test.cc, but that should be ok
(generally changing #pragma specific to MSVC++ to correct compiler_specific
macro).

[Evan Martin, 2008-09-16 22:50:27]

I'm gonna try this patch out.  Some comments below, which I'll take care of
myself unless you have something to add.

http://codereview.chromium.org/1661/diff/808/618
File webkit/glue/plugins/test/plugin_npobject_proxy_test.cc (right):

http://codereview.chromium.org/1661/diff/808/618#newcode12
Line 12: #include "webkit/glue/plugins/test/plugin_npobject_proxy_test.h"
Should this be the topmost #include?

http://codereview.chromium.org/1661/diff/808/612
File webkit/glue/plugins/test/plugin_test.cc (right):

http://codereview.chromium.org/1661/diff/808/612#newcode6
Line 6: #include "webkit/glue/plugins/test/plugin_test.h"
Should this be the topmost #include?

http://codereview.chromium.org/1661/diff/808/612#newcode37
Line 37: return x > 9 ? (x + 'A' - 10) : (x + '0');
Thanks for cleaning that up.  It's kind of weird that x is passed by reference
as well, though I suppose the compiler will figure it out.

[Evan Martin, 2008-09-16 23:02:12]

This doesn't apply cleanly to trunk.  Can you update it?

[please use my chromium address, 2008-09-17 06:51:02]

On 2008/09/16 23:02:12, Evan Martin wrote:
> This doesn't apply cleanly to trunk.  Can you update it?

Fixed order of includes, changed &x to x in inline function and updated to
revision 2299. Now it should apply cleanly. If it still doesn't, please include
names of troublesome files and approximate place of conflict.

[please use my chromium address, 2008-09-22 07:04:25]

*ping* - Please take a look.

[sgk, 2008-09-22 16:31:02]

SCons changes lgtm

[Evan Martin, 2008-09-24 00:39:33]

r2534 -- sorry for the delay, have been busy.