Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(108)

Issue 8228025: Implement WebKitPlatformSupport::currentThread (Closed)

Created:
9 years, 2 months ago by piman
Modified:
9 years, 2 months ago
CC:
chromium-reviews, darin-cc_chromium.org
Visibility:
Public.

Description

Implement WebKitPlatformSupport::currentThread BUG=None TEST=Webkit compositor in Chrome. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105206

Patch Set 1 #

Total comments: 2

Patch Set 2 : git try #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -12 lines) Patch
M webkit/glue/webkitplatformsupport_impl.h View 4 chunks +4 lines, -0 lines 0 comments Download
M webkit/glue/webkitplatformsupport_impl.cc View 3 chunks +25 lines, -1 line 0 comments Download
M webkit/glue/webthread_impl.h View 1 2 chunks +14 lines, -6 lines 1 comment Download
M webkit/glue/webthread_impl.cc View 1 2 chunks +17 lines, -5 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
piman
This is the chrome side for https://bugs.webkit.org/show_bug.cgi?id=69048
9 years, 2 months ago (2011-10-12 00:38:23 UTC) #1
darin (slow to review)
LGTM http://codereview.chromium.org/8228025/diff/1/webkit/glue/webthread_impl.cc File webkit/glue/webthread_impl.cc (right): http://codereview.chromium.org/8228025/diff/1/webkit/glue/webthread_impl.cc#newcode58 webkit/glue/webthread_impl.cc:58: #ifdef WEBTHREAD_HAS_LONGLONG_CHANGE i think this #ifdef was just ...
9 years, 2 months ago (2011-10-12 04:42:20 UTC) #2
piman
http://codereview.chromium.org/8228025/diff/1/webkit/glue/webthread_impl.cc File webkit/glue/webthread_impl.cc (right): http://codereview.chromium.org/8228025/diff/1/webkit/glue/webthread_impl.cc#newcode58 webkit/glue/webthread_impl.cc:58: #ifdef WEBTHREAD_HAS_LONGLONG_CHANGE On 2011/10/12 04:42:20, darin wrote: > i ...
9 years, 2 months ago (2011-10-12 19:53:52 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/piman@chromium.org/8228025/4001
9 years, 2 months ago (2011-10-12 19:54:48 UTC) #4
darin (slow to review)
LGTM
9 years, 2 months ago (2011-10-12 20:54:04 UTC) #5
commit-bot: I haz the power
Change committed as 105206
9 years, 2 months ago (2011-10-12 23:47:24 UTC) #6
jamesr
http://codereview.chromium.org/8228025/diff/4001/webkit/glue/webthread_impl.h File webkit/glue/webthread_impl.h (right): http://codereview.chromium.org/8228025/diff/4001/webkit/glue/webthread_impl.h#newcode19 webkit/glue/webthread_impl.h:19: virtual void postDelayedTask(Task* task, long long delay_ms) OVERRIDE; i ...
9 years, 2 months ago (2011-10-18 02:26:16 UTC) #7
piman
On 2011/10/18 02:26:16, jamesr wrote: > http://codereview.chromium.org/8228025/diff/4001/webkit/glue/webthread_impl.h > File webkit/glue/webthread_impl.h (right): > > http://codereview.chromium.org/8228025/diff/4001/webkit/glue/webthread_impl.h#newcode19 > ...
9 years, 2 months ago (2011-10-18 02:49:00 UTC) #8
darin (slow to review)
9 years, 2 months ago (2011-10-18 22:59:29 UTC) #9
On Mon, Oct 17, 2011 at 7:49 PM, <piman@chromium.org> wrote:

> On 2011/10/18 02:26:16, jamesr wrote:
>
>> http://codereview.chromium.**org/8228025/diff/4001/webkit/**
>>
glue/webthread_impl.h<http://codereview.chromium.org/8228025/diff/4001/webkit/glue/webthread_impl.h>
>> File webkit/glue/webthread_impl.h (right):
>>
>
>
> http://codereview.chromium.**org/8228025/diff/4001/webkit/**
>
glue/webthread_impl.h#**newcode19<http://codereview.chromium.org/8228025/diff/4001/webkit/glue/webthread_impl.h#newcode19>
>
>> webkit/glue/webthread_impl.h:**19: virtual void postDelayedTask(Task*
>> task, long
>> long delay_ms) OVERRIDE;
>> i thought we were avoiding using OVERRIDE in implementations of WebKit
>> APIs so
>> that rolling wouldn't be too painful. has that changed?
>>
>
> I didn't know that. The plan was to make sure we wouldn't accidentally
> break
> stuff while rolling. If the consensus is that we don't like OVERRIDE in
> this
> case, then I'm happy to remove it in a follow up CL.
>
>
Yes, OVERRIDE can make API changes more painful.  I agree that it can also
help you
catch mistakes.  I'm not entirely sure which is better, but I'm currently
feeling the pain of
trying to change a method override declared with OVERRIDE :-P

-Darin



>
http://codereview.chromium.**org/8228025/<http://codereview.chromium.org/8228...
>

Powered by Google App Engine
This is Rietveld 408576698