Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6584950149087232: Issue 1280 - Update v8 (Closed)

Created:
Oct. 24, 2014, 12:42 p.m. by sergei
Modified:
Nov. 4, 2014, 2:46 p.m.
Visibility:
Public.

Description

IMPORTANT: I've not tested it for Android. * it does not work on MSVS2010, such fixes will be in another commit * update to the most closest revision of maxthon. Despite they said it's 3.20.16, the closest version is 062a08f5e129ea0fdc0376a6cd51de7570294682 * adapt code to work with the current v8 version. V8ValueHolder - adapt for the new v8::Persistent syntax - add typedef typename v8::Persistent<T> V8Persistent; - remove copy-ctr, because it's not used, but requires support - remove V8ValueHolder(v8::Isolate* isolate, v8::Persistent<T> value) because it's not used and currently it's not possible to copy v8::Persistent. - adapt V8ValueHolder::reset methods - remove type conversion operators because they are not supported by current v8::Persistent JsValue: - pass JsValuePtr as const JsValuePtr&. It's a performance optimization, avoids unnecessary interlocked calls. - add v8::Local<v8::Value> JsValue::unwrapValue() const; It helps to retrieve v8::Local from v8::Persistent because the syntax has been changed. - adapt to the new syntax. JsContext, JsEngine, FileSystemJsObject - adapt code, nothing important

Patch Set 1 #

Patch Set 2 : use msvs2012, because firstly it should go into our branch #

Total comments: 17

Patch Set 3 : fix style #

Patch Set 4 : get rid of auto and fix friends of JsValue #

Total comments: 9

Patch Set 5 : get rid of auto and fix friends of JsValue #

Patch Set 6 : get rid of auto and fix friends of JsValue #

Total comments: 19

Patch Set 7 : get rid of unrelated stuff #

Patch Set 8 : revert not critical chanegs in JsValue ctr #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -63 lines) Patch
M .hgsubstate View 1 chunk +1 line, -1 line 0 comments Download
M common.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M createsolution.bat View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M include/AdblockPlus/JsValue.h View 1 2 3 4 5 6 7 4 chunks +6 lines, -4 lines 0 comments Download
M include/AdblockPlus/V8ValueHolder.h View 1 2 3 4 5 6 3 chunks +11 lines, -27 lines 0 comments Download
M libadblockplus.gyp View 1 chunk +2 lines, -1 line 0 comments Download
M src/JsContext.cpp View 1 chunk +1 line, -1 line 0 comments Download
M src/JsEngine.cpp View 1 2 3 4 5 6 1 chunk +4 lines, -2 lines 0 comments Download
M src/JsValue.cpp View 1 2 3 4 5 6 7 7 chunks +33 lines, -25 lines 0 comments Download

Messages

Total messages: 17
sergei
Tested on MSVS2010. It also compiles and works well with Maxthon's version of v8 (http://dl.maxthon.com/adp/v8.zip).
Oct. 24, 2014, 12:45 p.m. (2014-10-24 12:45:51 UTC) #1
sergei
> Felix: > So, what I think we can do now is first update to ...
Oct. 27, 2014, 10:40 a.m. (2014-10-27 10:40:41 UTC) #2
Felix Dahlke
Just did a first pass at this, still planning to take a closer look. But ...
Oct. 31, 2014, 5:31 a.m. (2014-10-31 05:31:57 UTC) #3
sergei
http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/createsolution.bat File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/createsolution.bat#newcode7 createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G ...
Oct. 31, 2014, 11:47 a.m. (2014-10-31 11:47:00 UTC) #4
Felix Dahlke
http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/createsolution.bat File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/createsolution.bat#newcode7 createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G ...
Oct. 31, 2014, 3:07 p.m. (2014-10-31 15:07:51 UTC) #5
sergei
http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/createsolution.bat File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/createsolution.bat#newcode7 createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G ...
Oct. 31, 2014, 4:13 p.m. (2014-10-31 16:13:34 UTC) #6
Felix Dahlke
Alright, took a proper close look this time, looking good! Just some Friday afternoon nit ...
Oct. 31, 2014, 4:30 p.m. (2014-10-31 16:30:15 UTC) #7
sergei
http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/include/AdblockPlus/JsValue.h File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/include/AdblockPlus/JsValue.h#newcode59 include/AdblockPlus/JsValue.h:59: JsValue(const JsValuePtr& value); On 2014/10/31 16:30:15, Felix H. Dahlke ...
Oct. 31, 2014, 5:14 p.m. (2014-10-31 17:14:17 UTC) #8
Felix Dahlke
http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/include/AdblockPlus/JsValue.h File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/include/AdblockPlus/JsValue.h#newcode59 include/AdblockPlus/JsValue.h:59: JsValue(const JsValuePtr& value); On 2014/10/31 17:14:17, sergei wrote: > ...
Nov. 1, 2014, 4:45 a.m. (2014-11-01 04:45:40 UTC) #9
Wladimir Palant
I'd strongly prefer you to remove all unnecessary changes from this review. While these changes ...
Nov. 1, 2014, 10:30 p.m. (2014-11-01 22:30:25 UTC) #10
Felix Dahlke
http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/DefaultWebRequestWinInet.cpp File src/DefaultWebRequestWinInet.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/DefaultWebRequestWinInet.cpp#newcode175 src/DefaultWebRequestWinInet.cpp:175: res = WinHttpQueryHeaders(hRequest, WINHTTP_QUERY_STATUS_CODE, WINHTTP_HEADER_NAME_BY_INDEX, WINHTTP_NO_OUTPUT_BUFFER , &statusLen, WINHTTP_NO_HEADER_INDEX); ...
Nov. 2, 2014, 3:55 a.m. (2014-11-02 03:55:20 UTC) #11
Felix Dahlke
Just noticed that the review is private - it can be public alright. The issue ...
Nov. 3, 2014, 5:03 a.m. (2014-11-03 05:03:26 UTC) #12
sergei
I see that some style stuff like 80 characters width, spaces, new lines and indentation ...
Nov. 3, 2014, 12:58 p.m. (2014-11-03 12:58:19 UTC) #13
Felix Dahlke
On 2014/11/03 12:58:19, sergei wrote: > I see that some style stuff like 80 characters ...
Nov. 3, 2014, 3:45 p.m. (2014-11-03 15:45:23 UTC) #14
Felix Dahlke
LGTM, assuming that we indeed need to create a new local handle for context before ...
Nov. 3, 2014, 3:46 p.m. (2014-11-03 15:46:23 UTC) #15
sergei
On 2014/11/03 15:46:23, Felix H. Dahlke wrote: > LGTM, assuming that we indeed need to ...
Nov. 3, 2014, 4:10 p.m. (2014-11-03 16:10:33 UTC) #16
Wladimir Palant
Nov. 3, 2014, 6:06 p.m. (2014-11-03 18:06:33 UTC) #17
LGTM

http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/...
File src/JsEngine.cpp (right):

http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/...
src/JsEngine.cpp:60: JsValuePtr(new JsValue(result,
v8::Local<v8::Context>::New(result->isolate, result->context)->Global())));
I see, so v8::Persistent cannot be used as a handle any more. Then this change
is fine of course.

Powered by Google App Engine
This is Rietveld