Bug 32652

Summary: forward-declare libsoup types
Product: WebKit Reporter: Evan Martin <evan>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: danw, gustavo, levin, mrobinson, tonyg, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 52451    
Attachments:
Description Flags
Patch
none
patch
evan: commit-queue-
reupload of patch to trigger bots evan: review-, evan: commit-queue-

Description Evan Martin 2009-12-17 02:55:39 PST
Created attachment 45049 [details]
Patch

Reduce header interdependencies by forward-declaring types in headers.
Comment 1 Evan Martin 2009-12-17 02:56:48 PST
Comment on attachment 45049 [details]
Patch

Please don't cq+ this, it requires a change in libsoup.
Comment 2 Evan Martin 2009-12-17 02:58:34 PST
Created attachment 45050 [details]
patch
Comment 3 Evan Martin 2009-12-17 02:59:31 PST
Comment on attachment 45050 [details]
patch

(Fixed ChangeLog)

Please don't cq+ this, it requires a change in libsoup.
Comment 4 WebKit Review Bot 2009-12-17 03:00:51 PST
style-queue ran check-webkit-style on attachment 45050 [details] without any errors.
Comment 5 Dan Winship 2009-12-17 03:04:00 PST
the soup change is now there in git master
Comment 6 Evan Martin 2009-12-17 08:08:17 PST
I guess to finish this patch I should add a change to configure.ac to depend on whatever libsoup version ends up coming out.
Comment 7 Eric Seidel (no email) 2009-12-28 22:23:10 PST
Comment on attachment 45050 [details]
patch

Do we have an expected timeline for when this should be landed?  Or should we just remove the r+ until then?  I'm not in any huge rush, but it's silly to have non-actionable patches in the pending-commit list.
Comment 8 Evan Martin 2009-12-29 09:38:58 PST
Can you leave a comment saying you officially R+ this so that a later reviewer can see that?  Then it's fine with me to r- it until it's ready.
Comment 9 Eric Seidel (no email) 2009-12-29 10:54:39 PST
Comment on attachment 45050 [details]
patch

Yes.  You should feel free to land this once the soup change is in.  You have both mine and Dave Levin's review for such.

Clearing the r+ for now.  Feel free to mark this r?/cq+ again if you'd prefer to have the commit-queue land this for you.  If you email me, I'l be happy to do the official r+ing again at that time.

Unfortunately we don't have an easy way to exclude non-actionable patches form the pending-commit list.  Adam Barth suggested we create a "CommitBlocked" keyword, which is one option.  I should probably bring it up on WebKit-dev.
Comment 10 Evan Martin 2010-04-12 16:14:11 PDT
BTW, maybe Xan could r+/cq+ this if it looks good?
Comment 11 Evan Martin 2010-04-12 16:16:03 PDT
Created attachment 53190 [details]
reupload of patch to trigger bots

Just want bots to test this again.
Comment 12 WebKit Review Bot 2010-04-12 16:19:37 PDT
Attachment 53190 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/soup/ResourceResponseSoup.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/network/soup/CookieJarSoup.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Review Bot 2010-04-12 16:54:58 PDT
Attachment 53190 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1671278
Comment 14 David Levin 2010-04-13 14:17:30 PDT
This appears to break gtk. Please consider uploading anew patch (and canceling this one).
Comment 15 Gustavo Noronha (kov) 2010-04-15 05:47:07 PDT
(In reply to comment #14)
> This appears to break gtk. Please consider uploading anew patch (and canceling
> this one).

That's probably because the bots need a newer soup. What exactly is the change that is required, though?
Comment 16 Evan Martin 2010-04-15 08:01:48 PDT
To avoid the soup headers, I was forward-declaring like this:

+typedef struct _SoupMessage SoupMessage;

At the December hackfest, I think danw modified libsoup to allow such a forward declaration.  But maybe it hasn't been released yet...
Comment 17 Dan Winship 2010-04-15 08:18:47 PDT
When we did the patches, webkit wasn't using SoupMessageFlags, but now it is, so removing the soup.h include breaks it.

I don't know if it's allowed stylistically, but you could make m_soupFlags a uint in ResourceRequest.h and ResourceResponse.h, and only cast to/from SoupMessageFlags in ResourceResponseSoup.cpp.

Otherwise you'd need to submit a patch to libsoup to make SoupMessageFlags also be "enum _SoupMessageFlags" (and probably fix everything else while you're there too, so this doesn't happen again in the future), and then update the patch here to use that.
Comment 18 Evan Martin 2010-04-15 08:26:29 PDT
gcc doesn't allow forward-declaring enums.  :~(

(I believe it comes from the fact that in principle an enum with < 256 entries can be stored as a byte, while < 65536 a short, etc.)

I wouldn't be opposed to using a uint for this, but this is also not code I frequently have to deal with.  If you guys think using a uint and casting isn't nice, I'll just close this bug.
Comment 19 Tony Gentilcore 2011-03-14 14:04:26 PDT
Is this bug obsolete?
Comment 20 Martin Robinson 2014-03-25 15:22:49 PDT
I think at this point we need to restart the process if we are interested.