Bug 59394 - XMLDocumentParserLibxml2 should play nice with strict OwnPtrs
Summary: XMLDocumentParserLibxml2 should play nice with strict OwnPtrs
Status: RESOLVED FIXED
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
URL:
Keywords:
: 60676 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-04-25 16:33 PDT by Adam Barth
Modified: 2011-05-12 14:57 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.21 KB, patch)
2011-04-25 16:34 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2011-04-27 04:07 PDT, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
Patch (18.23 KB, patch)
2011-04-29 11:17 PDT, Maciej Stachowiak
darin: review+
Details | Formatted Diff | Diff
Patch (16.98 KB, patch)
2011-05-12 14:39 PDT, Patrick R. Gansterer
commit-queue: commit-queue-
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-25 16:33:48 PDT
ValidationMessage and XMLDocumentParserLibxml2 should play nice with strict OwnPtrs
Comment 1 Adam Barth 2011-04-25 16:34:42 PDT
Created attachment 91006 [details]
Patch
Comment 2 Adam Barth 2011-04-25 16:35:53 PDT
Committed r84835: <http://trac.webkit.org/changeset/84835>
Comment 3 WebKit Review Bot 2011-04-25 16:41:40 PDT
http://trac.webkit.org/changeset/84835 might have broken Chromium Linux Release
Comment 4 James Robinson 2011-04-25 16:46:14 PDT
Reverted r84835 for reason:

Breaks compile because Deque<OwnPtr> doesn't work

Committed r84843: <http://trac.webkit.org/changeset/84843>
Comment 5 Maciej Stachowiak 2011-04-27 04:07:58 PDT
Created attachment 91264 [details]
Patch
Comment 6 WebKit Review Bot 2011-04-27 04:11:57 PDT
Attachment 91264 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8509937
Comment 7 Maciej Stachowiak 2011-04-27 04:29:53 PDT
I guess some of the EWS bots don't like it. I can't guess what would cause that linker error.
Comment 8 WebKit Review Bot 2011-04-27 04:32:19 PDT
Attachment 91264 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8509940
Comment 9 Build Bot 2011-04-27 04:36:15 PDT
Attachment 91264 [details] did not build on win:
Build output: http://queues.webkit.org/results/8513291
Comment 10 Maciej Stachowiak 2011-04-27 04:38:16 PDT
OK, now I think I see why it might be a deque problem and why it only shows up in the linker.
Comment 11 Maciej Stachowiak 2011-04-27 04:42:24 PDT
It's Deque::takeFirst that has the problem and it can easily by avoided by just using first() and removeFirst() separately. It would be hard to avoid otherwise short of specializing for OwnPtr.
Comment 12 WebKit Review Bot 2011-04-27 05:03:27 PDT
Attachment 91264 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8514190
Comment 13 Adam Barth 2011-04-27 14:17:25 PDT
(In reply to comment #11)
> It's Deque::takeFirst that has the problem and it can easily by avoided by just using first() and removeFirst() separately. It would be hard to avoid otherwise short of specializing for OwnPtr.

Ah, ok.  I can pick this up again then.  Thanks!
Comment 14 Maciej Stachowiak 2011-04-29 11:17:07 PDT
Created attachment 91707 [details]
Patch
Comment 15 WebKit Review Bot 2011-04-29 11:20:31 PDT
Attachment 91707 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/PassTraits.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/Deque.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Darin Adler 2011-04-29 11:54:15 PDT
Comment on attachment 91707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91707&action=review

>> Source/JavaScriptCore/wtf/Deque.h:37
>> +#include "PassTraits.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

What the stylebot said ;-)

> Source/JavaScriptCore/wtf/Deque.h:55
> +        typedef PassTraits<T> Pass;

Not 100% sure the name “Pass” is clear enough name for this. If it wasn’t for the name conflict, I’d suggest calling it PassTraits.

>> Source/JavaScriptCore/wtf/PassTraits.h:41
>> +    template<typename T> struct PassTraits {
> 
> Code inside a namespace should not be indented.  [whitespace/indent] [4]

Would be better to follow the style guide here.

> Source/JavaScriptCore/wtf/PassTraits.h:44
> +        static PassType transfer(Type& t) { return t; }

Is “t” the best name for this argument? Maybe “argument”? Or “object”?
Comment 17 Patrick R. Gansterer 2011-05-12 14:39:22 PDT
Created attachment 93346 [details]
Patch

@mjs: I fixed the style issues of you patch. Can you verify the patch and set cq+?
Comment 18 Patrick R. Gansterer 2011-05-12 14:40:13 PDT
*** Bug 60676 has been marked as a duplicate of this bug. ***
Comment 19 WebKit Commit Bot 2011-05-12 14:45:48 PDT
Comment on attachment 93346 [details]
Patch

Rejecting attachment 93346 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
----------
|Index: Source/JavaScriptCore/wtf/PassTraits.h
|index 76c4769..3462734 100644
|--- Source/JavaScriptCore/wtf/PassTraits.h
|+++ Source/JavaScriptCore/wtf/PassTraits.h
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/dom/XMLDocumentParserLibxml2.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8689820
Comment 20 Patrick R. Gansterer 2011-05-12 14:57:23 PDT
Committed r86383: <http://trac.webkit.org/changeset/86383>