Bug 32652

Comment on attachment 45049 [details] Patch Please don't cq+ this, it requires a change in libsoup.

Created attachment 45050 [details] patch

Comment on attachment 45050 [details] patch (Fixed ChangeLog) Please don't cq+ this, it requires a change in libsoup.

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

the soup change is now there in git master

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

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

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

Created attachment 53190 [details] reupload of patch to trigger bots Just want bots to test this again.

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.

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

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

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

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

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.

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.

Is this bug obsolete?

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