| Summary: | Enable strict OwnPtrs for Chromium | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | dglazkov, jamesr, levin, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Bug Depends on: | 59664 | ||||||
| Bug Blocks: | 59428 | ||||||
| Attachments: |
|
||||||
|
Description
Adam Barth
2011-04-27 18:24:16 PDT
Created attachment 91397 [details]
Patch
Should build cleanly on Mac, but jamesr says it doesn't build cleanly on Linux. Comment on attachment 91397 [details]
Patch
Pending clean build of course.
https://bugs.webkit.org/show_bug.cgi?id=59664 is needed for chromium linux. I haven't tried chromium win, but I'm sure there are issues there as well. Attachment 91397 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8518180 I don't have a chromium-win build. I'll probably land this late at night and fire-fight the bot, unless someone would like to volunteer to weed out these issues on windows. Figuring out how to do a try job with both depots was harder than I thought it would be but here you go: http://build.chromium.org/p/tryserver.chromium/builders/win/builds/28759 http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/23951 http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/24454 Tryjobs all failed to apply the patch. Try specifying -r to git/gcl try. (In reply to comment #8) > Tryjobs all failed to apply the patch. Try specifying -r to git/gcl try. I'm not sure if that would help because I think that specifies chromium's revision and the problem is webkit's revision (maybe if I ran it from the subdir and then I wouldn't have to update DEPS?) Anyway, I modified the patch so that it won't conflict with the other patch that is on the webkit side (http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/23959/steps/update/logs/patch) and I think this will work now: http://build.chromium.org/p/tryserver.chromium/builders/win/builds/28765 http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/23959 http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/24460 Those try results don't really make sense. It's reporting errors in functions that have #define LOOSE_OWN_PTR at the top. (In reply to comment #10) > Those try results don't really make sense. It's reporting errors in functions that have #define LOOSE_OWN_PTR at the top. oh that has to do with how I finally got the patch to apply: http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/23959/steps/update/logs/patch The reason I had to do this was a bit involved. (I was patching before your first change to OwnPtr.h but rolling to after it -- I didn't think of this issue or I could have handled it differently.) In any case, the windows results are helpful. :) Ok. I've fixed the error that the Windows try bot uncovered. The Mac trybot also uncovered an error that I don't see in my upstream Chromium build. Perhaps the feature is only enabled downstream? In any case, I fixed it as well. I'm going to try landing this patch with my fingers crossed. I expect disaster. Comment on attachment 91397 [details] Patch Clearing flags on attachment: 91397 Committed r85167: <http://trac.webkit.org/changeset/85167> All reviewed patches have been landed. Closing bug. Looks like it built cleanly on chromium-win. Thanks Mr. Levin! |