Chromium Code Reviews
Help | Chromium Project | Sign in
(9)

Issue 168533002: Cleanup: Forward declare v8::Isolate in V8Initializer. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by tfarina
Modified:
1 year, 1 month ago
Reviewers:
haraken, abarth
CC:
blink-reviews, Nils Barth (inactive), kojih, arv+blink, jsbell+bindings_chromium.org, sof, abarth, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, Inactive
Visibility:
Public.

Description

Cleanup: Forward declare v8::Isolate in V8Initializer. We can forward declare it because we are not calling any function of Isolate in the header, so the forward declaration is enough to make a pointer declaration and thus we don't need to include v8.h in the header. BUG=None TEST=None R=abarth@chromium.org

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -1 line) Patch
M Source/bindings/v8/V8Initializer.h View 1 chunk +3 lines, -1 line 0 comments Download
M Source/bindings/v8/V8Initializer.cpp View 1 chunk +1 line, -0 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 10 (0 generated)
tfarina
1 year, 1 month ago (2014-02-15 15:38:12 UTC) #1
abarth
not lgtm This CL doesn't solve any problem. It's perfectly normal for code in bindings/v8 ...
1 year, 1 month ago (2014-02-15 19:07:14 UTC) #2
tfarina
On 2014/02/15 19:07:14, abarth wrote: > not lgtm > > This CL doesn't solve any ...
1 year, 1 month ago (2014-02-16 14:10:53 UTC) #3
tfarina
Adam, please, could you read the first section of http://www.chromium.org/developers/coding-style/cpp-dos-and-donts? I think that covers the ...
1 year, 1 month ago (2014-02-16 14:12:06 UTC) #4
haraken
On 2014/02/16 14:12:06, tfarina wrote: > Adam, please, could you read the first section of ...
1 year, 1 month ago (2014-02-16 14:35:45 UTC) #5
tfarina
On 2014/02/16 14:35:45, haraken wrote: > On 2014/02/16 14:12:06, tfarina wrote: > > Adam, please, ...
1 year, 1 month ago (2014-02-16 14:43:57 UTC) #6
tfarina
On 2014/02/16 14:35:45, haraken wrote: > On 2014/02/16 14:12:06, tfarina wrote: > > Adam, please, ...
1 year, 1 month ago (2014-02-16 14:43:57 UTC) #7
tfarina
OK, I guess it is fine, since v8.h does not include other headers. Closing this. ...
1 year, 1 month ago (2014-02-16 14:44:41 UTC) #8
tfarina
On Sun, Feb 16, 2014 at 11:35 AM, <haraken@chromium.org> wrote: > On 2014/02/16 14:12:06, tfarina ...
1 year, 1 month ago (2014-02-16 14:48:42 UTC) #9
abarth
1 year, 1 month ago (2014-02-18 01:18:23 UTC) #10
On Sun, Feb 16, 2014 at 6:48 AM, Thiago Farina <tfarina@chromium.org> wrote:

> On Sun, Feb 16, 2014 at 11:35 AM, <haraken@chromium.org> wrote:
>
>> On 2014/02/16 14:12:06, tfarina wrote:
>>
>>> Adam, please, could you read the first section of
>>> http://www.chromium.org/developers/coding-style/cpp-dos-and-donts? I
>>> think
>>>
>> that
>>
>>> covers the good practice here.
>>>
>>
>> not lgtm2.
>>
>> In a particular case of v8.h, there is no benefit in replacing #include
>> with
>> forward declarations. As Adam mentioned, it's normal for v8 bindings to
>> include
>> v8.h to avoid writing a lot of forward declarations.
>>
>> Writing (a lot of) forward declarations is still better than including
> (unnecessary) headers in other headers. The main reason is compilation
> time, and to be honest WebKit/Blink has the worst compilation time among
> the projects used on Chromium, and since I have a slow machine that matters
> to me. It certainly does not matter when you have a z620 or a cluster to
> build your binary under 2 minutes.
>

I'm in favor of working to improve compilation time.  If you have data
about how much we can improve compilation time in this way, I'd be
interested to see it.

Adam

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ecdb341-tainted