Bug 77759

Summary: Missing include in TestNetscapePlugIn
Product: WebKit Reporter: Kalev Lember <kalevlember>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: morrita, peter, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Explicitly include unistd.h in TestNetscapePlugIn
morrita: review-
Patch v2
morrita: review+, morrita: commit-queue-
Patch v3 none

Kalev Lember
Reported 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.
Attachments
Explicitly include unistd.h in TestNetscapePlugIn (1.97 KB, patch)
2012-02-03 11:44 PST, Kalev Lember 		morrita: review-
DetailsFormatted DiffDiff
Patch v2 (1.27 KB, patch)
2012-02-20 23:22 PST, Kalev Lember 		morrita: review+
morrita: commit-queue-
DetailsFormatted DiffDiff
Patch v3 (1.28 KB, patch)
2012-02-22 01:21 PST, Kalev Lember 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kalev Lember
Comment 1 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.
Martin Robinson
Comment 2 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?
Kalev Lember
Comment 3 2012-02-03 13:27:33 PST
Adding Peter Beverloo to CC. Peter, could you check please if XP_UNIX gets defined on Android?
Hajime Morrita
Comment 4 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.
Kalev Lember
Comment 5 2012-02-20 23:22:57 PST
Created attachment 127925 [details] Patch v2
Kalev Lember
Comment 6 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.
Peter Beverloo
Comment 7 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!
Hajime Morrita
Comment 8 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.
Hajime Morrita
Comment 9 2012-02-21 16:02:17 PST
Oops s/Perter/Peter/.
Kalev Lember
Comment 10 2012-02-22 01:21:40 PST
Created attachment 128154 [details] Patch v3 Thanks! Updated the patch title as requested.
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2012-02-22 12:51:23 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.