Bug 37293 – [WebKit2] Add Platform directory and files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Created attachment 52898 [details]
Patch

Start the dump of WebKit2.

[Adam Barth]

Is there a design doc for the structure of WebKit2?  It's unclear to me how this relates to other ports.

[WebKit Review Bot]

Attachment 52898 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
 the same line  [whitespace/newline] [4]
WebKit2/Platform/CoreIPC/Attachment.h:58:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/Platform/CoreIPC/Attachment.h:61:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/Platform/CoreIPC/Attachment.h:62:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/Platform/CoreIPC/Attachment.h:63:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/Platform/CoreIPC/Attachment.h:64:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/Platform/win/RunLoopWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/win/RunLoopWin.cpp:54:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit2/Platform/win/WorkQueueWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/CoreIPC/Attachment.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/CoreIPC/Attachment.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/Platform/CoreIPC/Connection.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/CoreIPC/Connection.cpp:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Platform/CoreIPC/Connection.cpp:144:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit2/Platform/WorkQueue.h:37:  "wtf/HashMap.h" already included at WebKit2/Platform/WorkQueue.h:31  [build/include] [4]
WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/CoreIPC/ArgumentDecoder.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/Platform/CoreIPC/ArgumentDecoder.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/Platform/CoreIPC/win/ConnectionWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/CoreIPC/win/ConnectionWin.cpp:96:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit2/Platform/CoreIPC/win/ConnectionWin.cpp:149:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 42 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

(In reply to comment #1)
> Is there a design doc for the structure of WebKit2?  It's unclear to me how
> this relates to other ports.

It's a new take on WebKit port design, so WebKit2 kinda seems reasonable.

[Adam Barth]

I know.  I was just giving Sam a hard time.  :)

Glad we have a meeting coming up.  Should be fun to talk about.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> I know.  I was just giving Sam a hard time.  :)
> 
> Glad we have a meeting coming up.  Should be fun to talk about.

I also think we should land in buildable chunks! :P

[Adam Barth]

More seriously, we have some experience with this stuff.  Are you interested in a real review, or is the plan to land the code in the tree, presuming it was reviewed internally by WebKit Reviewers (tm)?

[Anders Carlsson]

(In reply to comment #5)
> (In reply to comment #4)
> > I know.  I was just giving Sam a hard time.  :)
> > 
> > Glad we have a meeting coming up.  Should be fun to talk about.
> 
> I also think we should land in buildable chunks! :P

This chunk is buildable! :)

[Sam Weinig]

(In reply to comment #6)
> More seriously, we have some experience with this stuff.  Are you interested in
> a real review, or is the plan to land the code in the tree, presuming it was
> reviewed internally by WebKit Reviewers (tm)?

Any feedback would be appreciated, but since we have the ok to land this (and have done internal review) we would like to land it and work iteratively in the Open Source from now on.  Side note: this shouldn't break anyone since no one builds it.

[Adam Barth]

Okiedokes.  I can file bugs about anything I notice as the patches go by.  You'd make me a happy man if you fixed the stylebot nits as you go, especially "Code inside a namespace should not be indented" because that one is a PITA to clean up incrementally.

[Darin Adler]

Comment on attachment 52898 [details]
Patch

> +    std::queue<WorkItem*> workItemQueue;

There's already using namespace std, so just queue.

> +        std::auto_ptr<WorkItem> item(workItemQueue.front());

And just auto_ptr.

> +void RunLoop::scheduleWork(std::auto_ptr<WorkItem> item)

And here.

I'll review more later.

[Anders Carlsson]

Comment on attachment 52898 [details]
Patch

r=me.

(There are some minor style issues which we plan to fix once we land).

[Adam Treat]

(In reply to comment #11)
> (From update of attachment 52898 [details])
> r=me.
> 
> (There are some minor style issues which we plan to fix once we land).

This is a big no no for every other port introduction.  It does not look good that you are landing this with style problems and without any community discussion before you've announced it today.

[Sam Weinig]

Landed in r57307.