Bug 59666 - Enable strict OwnPtrs for Chromium
Summary: Enable strict OwnPtrs for Chromium
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
Depends on: 59664
Blocks: 59428
  Show dependency treegraph
Reported: 2011-04-27 18:24 PDT by Adam Barth
Modified: 2011-04-28 00:10 PDT (History)
4 users (show)

See Also:

Patch (971 bytes, patch)
2011-04-27 18:24 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-04-27 18:24:16 PDT
Enable strict OwnPtrs for Chromium
Comment 1 Adam Barth 2011-04-27 18:24:31 PDT
Created attachment 91397 [details]
Comment 2 Adam Barth 2011-04-27 18:25:03 PDT
Should build cleanly on Mac, but jamesr says it doesn't build cleanly on Linux.
Comment 3 David Levin 2011-04-27 18:26:19 PDT
Comment on attachment 91397 [details]

Pending clean build of course.
Comment 4 James Robinson 2011-04-27 18:27:06 PDT
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.
Comment 5 WebKit Review Bot 2011-04-27 18:48:05 PDT
Attachment 91397 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8518180
Comment 6 Adam Barth 2011-04-27 18:55:22 PDT
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.
Comment 8 James Robinson 2011-04-27 19:39:26 PDT
Tryjobs all failed to apply the patch.  Try specifying -r to  git/gcl try.
Comment 9 David Levin 2011-04-27 19:47:23 PDT
(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:
Comment 10 Adam Barth 2011-04-27 23:04:28 PDT
Those try results don't really make sense.  It's reporting errors in functions that have #define LOOSE_OWN_PTR at the top.
Comment 11 David Levin 2011-04-27 23:15:41 PDT
(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.)
Comment 12 Adam Barth 2011-04-27 23:21:20 PDT
In any case, the windows results are helpful.  :)
Comment 13 Adam Barth 2011-04-27 23:31:34 PDT
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 14 Adam Barth 2011-04-27 23:35:48 PDT
Comment on attachment 91397 [details]

Clearing flags on attachment: 91397

Committed r85167: <http://trac.webkit.org/changeset/85167>
Comment 15 Adam Barth 2011-04-27 23:35:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Adam Barth 2011-04-28 00:10:08 PDT
Looks like it built cleanly on chromium-win.  Thanks Mr. Levin!