Bug 32652 – forward-declare libsoup types

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKitGTK

[Evan Martin]

Created attachment 45049 [details]
Patch

Reduce header interdependencies by forward-declaring types in headers.

[Evan Martin]

Comment on attachment 45049 [details]
Patch

Please don't cq+ this, it requires a change in libsoup.

[Evan Martin]

Created attachment 45050 [details]
patch

[Evan Martin]

Comment on attachment 45050 [details]
patch

(Fixed ChangeLog)

Please don't cq+ this, it requires a change in libsoup.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45050 [details] without any errors.

[Dan Winship]

the soup change is now there in git master

[Evan Martin]

I guess to finish this patch I should add a change to configure.ac to depend on whatever libsoup version ends up coming out.

[Eric Seidel (no email)]

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.

[Evan Martin]

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.

[Eric Seidel (no email)]

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.

[Evan Martin]

BTW, maybe Xan could r+/cq+ this if it looks good?

[Evan Martin]

Created attachment 53190 [details]
reupload of patch to trigger bots

Just want bots to test this again.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 53190 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1671278

[David Levin]

This appears to break gtk. Please consider uploading anew patch (and canceling this one).

[Gustavo Noronha (kov)]

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

[Evan Martin]

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

[Dan Winship]

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.

[Evan Martin]

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.

[Tony Gentilcore]

Is this bug obsolete?

[Martin Robinson]

I think at this point we need to restart the process if we are interested.