CPUProfiler: It is not clear why we are using Handle<Object> for scriptId. Lets flip it into Smi/in…

CPUProfiler: It is not clear why we are using Handle<Object> for scriptId. Lets flip it into Smi/int.

By the nature it is integer. So we can work with it as with Smi internaly and use int in the external API.

BUG=none
TEST=existing tests
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15327

[yurys, 2013-06-25 11:57:38]

https://codereview.chromium.org/17600006/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/17600006/diff/1/include/v8.h#newcode1047
include/v8.h:1047: V8_DEPRECATED(Local<Value> Id());
Won't this break Blink compilation?

https://codereview.chromium.org/17600006/diff/1/include/v8.h#newcode2358
include/v8.h:2358: int ScriptId() const;
Same question.

https://codereview.chromium.org/17600006/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/17600006/diff/1/src/api.cc#newcode4355
src/api.cc:4355: return i::Script::kNoScriptId;
This should be a public constant.

https://codereview.chromium.org/17600006/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/17600006/diff/1/src/heap.h#newcode178
src/heap.h:178: V(Smi, last_script_id, LastScriptId)                            
            \
Why not make it simple int property and probably move into Factory?

https://codereview.chromium.org/17600006/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/17600006/diff/1/test/cctest/test-api.cc#newco...
test/cctest/test-api.cc:16657: CHECK_EQ(script->GetId(), foo->ScriptId());
We should keep old tests as well until the API is deleted.

[Jakob Kummerow, 2013-06-25 11:58:39]

https://codereview.chromium.org/17600006/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/17600006/diff/1/src/api.cc#newcode4355
src/api.cc:4355: return i::Script::kNoScriptId;
nit: {} please

https://codereview.chromium.org/17600006/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/17600006/diff/1/src/heap.h#newcode1444
src/heap.h:1444: inline void SetLastScriptId(int last_script_id);
I don't think we need this any more, since AFAICT you removed the only call site
in factory.cc.

https://codereview.chromium.org/17600006/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/17600006/diff/1/src/objects.h#newcode5823
src/objects.h:5823: static const int kNoScriptId = 0;
Please move this somewhere else (e.g. to the beginning of the class definition,
by the enums), as it has nothing to do with the object layout description.
Since this only has a single use in api.cc, it would also be fine with me to
just inline the value (maybe as a function-local constant to be more descriptive
than a plain '0' literal).

[loislo, 2013-06-25 13:04:53]

comments addressed

https://codereview.chromium.org/17600006/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/17600006/diff/1/include/v8.h#newcode1047
include/v8.h:1047: V8_DEPRECATED(Local<Value> Id());
On 2013/06/25 11:57:38, Yury Semikhatsky wrote:
> Won't this break Blink compilation?

Done.

https://codereview.chromium.org/17600006/diff/1/include/v8.h#newcode2358
include/v8.h:2358: int ScriptId() const;
On 2013/06/25 11:57:38, Yury Semikhatsky wrote:
> Same question.

Done.

https://codereview.chromium.org/17600006/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/17600006/diff/1/src/api.cc#newcode4355
src/api.cc:4355: return i::Script::kNoScriptId;
On 2013/06/25 11:58:39, Jakob wrote:
> nit: {} please

Done.

https://codereview.chromium.org/17600006/diff/1/src/api.cc#newcode4355
src/api.cc:4355: return i::Script::kNoScriptId;
On 2013/06/25 11:57:38, Yury Semikhatsky wrote:
> This should be a public constant.

Done.

https://codereview.chromium.org/17600006/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/17600006/diff/1/src/heap.h#newcode178
src/heap.h:178: V(Smi, last_script_id, LastScriptId)                            
            \
On 2013/06/25 11:57:38, Yury Semikhatsky wrote:
> Why not make it simple int property and probably move into Factory?

factory is a memberless class. So it cannot be used for storing last_script_id.

https://codereview.chromium.org/17600006/diff/1/src/heap.h#newcode1444
src/heap.h:1444: inline void SetLastScriptId(int last_script_id);
On 2013/06/25 11:58:39, Jakob wrote:
> I don't think we need this any more, since AFAICT you removed the only call
site
> in factory.cc.

Done.

https://codereview.chromium.org/17600006/diff/1/src/heap.h#newcode1444
src/heap.h:1444: inline void SetLastScriptId(int last_script_id);
On 2013/06/25 11:58:39, Jakob wrote:
> I don't think we need this any more, since AFAICT you removed the only call
site
> in factory.cc.

Done.

https://codereview.chromium.org/17600006/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/17600006/diff/1/src/objects.h#newcode5823
src/objects.h:5823: static const int kNoScriptId = 0;
On 2013/06/25 11:58:39, Jakob wrote:
> Please move this somewhere else (e.g. to the beginning of the class
definition,
> by the enums), as it has nothing to do with the object layout description.
> Since this only has a single use in api.cc, it would also be fine with me to
> just inline the value (maybe as a function-local constant to be more
descriptive
> than a plain '0' literal).

It was moved to v8::Script.

[Jakob Kummerow, 2013-06-25 14:32:13]

LGTM with a comment.

Please avoid rebasing between patch sets as it screws up the diff view.

https://codereview.chromium.org/17600006/diff/2004/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/17600006/diff/2004/include/v8.h#newcode2363
include/v8.h:2363: V8_DEPRECATED(Handle<Value> GetScriptId() const);
I guess you meant to remove the V8_DEPRECATED annotation from the function
declaration.

[loislo, 2013-06-25 14:58:05]

Committed patchset #7 manually as r15327 (presubmit successful).