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

Issue 265593002: Add v8::Message::GetScriptOrigin() with tests (Closed)

Created:
6 years, 7 months ago by kozyatinskiy1
Modified:
6 years, 6 months ago
Reviewers:
vsevik, yurys, Yang
CC:
v8-dev, Paweł Hajdan Jr.
Base URL:
git://github.com/v8/v8.git@master
Visibility:
Public.

Description

Added Message::GetScripOrigin. Replaced Message::GetResourceName with GetScriptOrigin().ResourceName(). Now, GetScriptOrigin().ResourceName() function returns the resource name or sourceURL (from //# sourceURL=) for the script from where the function causing the error originates. Method GetScriptResourceName() deprecated. Use GetScriptOrigin()->ResourceName() instead. Function used in Blink: https://codereview.chromium.org/260513004/ R=yangguo@chromium.org, yurys@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=21893

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 7

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Total comments: 1

Patch Set 7 : #

Patch Set 8 : #

Total comments: 2

Patch Set 9 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -25 lines) Patch
M include/v8.h View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M samples/lineprocessor.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M samples/shell.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M src/api.cc View 1 2 3 4 5 6 7 8 1 chunk +19 lines, -10 lines 0 comments Download
M src/d8.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M test/cctest/test-api.cc View 1 2 3 4 5 6 7 11 chunks +61 lines, -12 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
vsevik
https://codereview.chromium.org/265593002/diff/40001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/265593002/diff/40001/include/v8.h#newcode1134 include/v8.h:1134: * Returns the resource name for the script from ...
6 years, 7 months ago (2014-04-30 10:18:40 UTC) #1
kozyatinskiy1
https://codereview.chromium.org/265593002/diff/40001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/265593002/diff/40001/test/cctest/test-api.cc#newcode17691 test/cctest/test-api.cc:17691: "//# sourceURL=foo2.js\";\n" On 2014/04/30 10:18:40, vsevik wrote: > could ...
6 years, 7 months ago (2014-04-30 15:46:42 UTC) #2
vsevik
Yury, could you please take a look?
6 years, 7 months ago (2014-05-05 07:21:47 UTC) #3
yurys
https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h File include/v8.h (right): https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h#newcode1137 include/v8.h:1137: Handle<Value> GetScriptResourceNameOrSourceURL() const; Would it make sense to expose ...
6 years, 7 months ago (2014-05-05 07:30:40 UTC) #4
kozyatinskiy1
On 2014/05/05 07:30:40, yurys wrote: > https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h > File include/v8.h (right): > > https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h#newcode1137 > ...
6 years, 7 months ago (2014-05-15 08:24:58 UTC) #5
yurys
On 2014/05/15 08:24:58, kozyatinskiy wrote: > On 2014/05/05 07:30:40, yurys wrote: > > https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h > ...
6 years, 7 months ago (2014-05-15 12:45:52 UTC) #6
vsevik
> Yeah, you are right. We discussed this with Vsevolod and it seems that what ...
6 years, 6 months ago (2014-06-04 14:28:16 UTC) #7
yurys
On 2014/06/04 14:28:16, vsevik wrote: > > Yeah, you are right. We discussed this with ...
6 years, 6 months ago (2014-06-05 08:53:03 UTC) #8
yurys
Yang, could you do OWNER review?
6 years, 6 months ago (2014-06-05 09:11:20 UTC) #9
Yang
On 2014/06/05 09:11:20, yurys wrote: > Yang, could you do OWNER review? Now that I ...
6 years, 6 months ago (2014-06-05 10:40:43 UTC) #10
vsevik
> Now that I looked closer at this API, I see (if I'm not mistaken) ...
6 years, 6 months ago (2014-06-05 11:44:03 UTC) #11
Yang
On 2014/06/05 11:44:03, vsevik wrote: > > Now that I looked closer at this API, ...
6 years, 6 months ago (2014-06-05 11:52:37 UTC) #12
kozyatinskiy1
On 2014/06/05 11:52:37, Yang wrote: > On 2014/06/05 11:44:03, vsevik wrote: > > > Now ...
6 years, 6 months ago (2014-06-11 16:19:11 UTC) #13
Yang
On 2014/06/11 16:19:11, kozyatinskiy wrote: > On 2014/06/05 11:52:37, Yang wrote: > > On 2014/06/05 ...
6 years, 6 months ago (2014-06-12 07:28:07 UTC) #14
kozyatinskiy1
Please, don't commit this patch without next: 1. https://codereview.chromium.org/260513004/ - replaced GetScriptResourceName in blink. 2. ...
6 years, 6 months ago (2014-06-16 09:23:18 UTC) #15
kozyatinskiy1
On 2014/06/16 09:23:18, kozyatinskiy wrote: > Please, don't commit this patch without next: > 1. ...
6 years, 6 months ago (2014-06-16 11:26:13 UTC) #16
yurys
lgtm https://codereview.chromium.org/265593002/diff/180001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/265593002/diff/180001/src/api.cc#newcode1953 src/api.cc:1953: isolate)); style: wrong indentation. https://codereview.chromium.org/265593002/diff/180001/src/api.cc#newcode1954 src/api.cc:1954: i::Handle<i::Script> script(i::Script::cast(scriptValue->value())); ...
6 years, 6 months ago (2014-06-16 12:40:53 UTC) #17
kozyatinskiy1
On 2014/06/16 12:40:53, yurys wrote: > lgtm > > https://codereview.chromium.org/265593002/diff/180001/src/api.cc > File src/api.cc (right): > ...
6 years, 6 months ago (2014-06-16 16:10:08 UTC) #18
yurys
6 years, 6 months ago (2014-06-20 07:44:15 UTC) #19
Message was sent while issue was closed.
Committed patchset #9 manually as r21893 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698