Summary: | forward-declare libsoup types | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Martin <evan> | ||||||||
Component: | WebKitGTK | Assignee: | 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: |
|
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. |
Created attachment 45049 [details] Patch Reduce header interdependencies by forward-declaring types in headers.