Bug 77759 - Missing include in TestNetscapePlugIn
Summary: Missing include in TestNetscapePlugIn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-03 11:43 PST by Kalev Lember
Modified: 2012-02-22 12:51 PST (History)
3 users (show)

See Also:


Attachments
Explicitly include unistd.h in TestNetscapePlugIn (1.97 KB, patch)
2012-02-03 11:44 PST, Kalev Lember
morrita: review-
Details | Formatted Diff | Diff
Patch v2 (1.27 KB, patch)
2012-02-20 23:22 PST, Kalev Lember
morrita: review+
morrita: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (1.28 KB, patch)
2012-02-22 01:21 PST, Kalev Lember
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalev Lember 2012-02-03 11:43:05 PST
gcc 4.7 in Fedora 17 needs unistd.h include for sleep(). The include is there, but guarded by #ifdef (ANDROID), which should be expanded to cover all Linux/Unix targets.
Comment 1 Kalev Lember 2012-02-03 11:44:14 PST
Created attachment 125376 [details]
Explicitly include unistd.h in TestNetscapePlugIn

Fixes build with gcc 4.7.

r100432 added the include only for ANDROID; this changes the ifdef to
cover all unix platforms. On Fedora 17 the gcc 4.7 compiler no longer
implicitly includes unistd.h from standard headers and other unix
platforms are likely to get gcc 4.7 as well in the future.
Comment 2 Martin Robinson 2012-02-03 12:54:34 PST
Comment on attachment 125376 [details]
Explicitly include unistd.h in TestNetscapePlugIn

Are you sure the Android build defines XP_UNIX?
Comment 3 Kalev Lember 2012-02-03 13:27:33 PST
Adding Peter Beverloo to CC.

Peter, could you check please if XP_UNIX gets defined on Android?
Comment 4 Hajime Morrita 2012-02-20 20:32:23 PST
Comment on attachment 125376 [details]
Explicitly include unistd.h in TestNetscapePlugIn

We can just have both XP_UNIX and ANDROID. That would be intention revealing and barely hurt the code.
I hope we had EWS for Android but currently we don't. Let us be conservative until then.
Comment 5 Kalev Lember 2012-02-20 23:22:57 PST
Created attachment 127925 [details]
Patch v2
Comment 6 Kalev Lember 2012-02-20 23:24:05 PST
(In reply to comment #4)
> We can just have both XP_UNIX and ANDROID. That would be intention revealing and barely hurt the code.

Fair enough. Uploaded a new patch that does this.
Comment 7 Peter Beverloo 2012-02-21 04:02:10 PST
Sorry for the late reply, this slipped through!

The change works well for Android, SGTM. Only nit would be that the title could be slightly clearer by appending "for GCC 4.7", but I was guilty of not doing that myself in the Android patch as well, so it should be fine :-). Thanks!
Comment 8 Hajime Morrita 2012-02-21 16:00:57 PST
Comment on attachment 127925 [details]
Patch v2

Perter, thanks for investigation!
@kalevlember could you update the bug summary? I'm happy to land this then.
Comment 9 Hajime Morrita 2012-02-21 16:02:17 PST
Oops s/Perter/Peter/.
Comment 10 Kalev Lember 2012-02-22 01:21:40 PST
Created attachment 128154 [details]
Patch v3

Thanks! Updated the patch title as requested.
Comment 11 WebKit Review Bot 2012-02-22 12:51:15 PST
Comment on attachment 128154 [details]
Patch v3

Clearing flags on attachment: 128154

Committed r108540: <http://trac.webkit.org/changeset/108540>
Comment 12 WebKit Review Bot 2012-02-22 12:51:23 PST
All reviewed patches have been landed.  Closing bug.