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

Issue 6193234183192576: Issue 1197 - change local copy of v8 (to 4.3.15) to work with Visual Studio 2013 (Closed)

Created:
June 11, 2015, 1:19 p.m. by sergei
Modified:
July 5, 2017, 10:54 a.m.
CC:
Felix Dahlke
Visibility:
Public.

Description

# it cannot be linked for android. - add gyp 'host_arch' variable which is now required - generate MSVS solution and projects for MSVS2013 - change to work with new signature of callback functions - add helper AdblockPlus::Utils::ThrowException to throw js exceptions (it's used in callbacks). - change to take into account renaming of v8::Persistent to v8::UniquePersistent - add initializing of v8 platform and link with the static lib v8_libplatform. - fix IdleNotification because 'v8::V8::IdleNotification' is not available anymore, we have to use 'isolate->IdleNotification(1000)' - pass isolate argument when it's necessary # depends on # https://codereview.adblockplus.org/6233220328718336/ - Issue 1197- fix isolate management # https://codereview.adblockplus.org/5145227803230208/ - Issue 1197 - adapt gyp variables # https://codereview.adblockplus.org/5117069997637632/ - Issue 1548 - manually disable usage of v8_random_seed in gyp

Patch Set 1 #

Patch Set 2 : update #

Patch Set 3 : rebase and update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -96 lines) Patch
M Makefile View 1 1 chunk +1 line, -1 line 0 comments Download
M createsolution.bat View 1 chunk +2 lines, -2 lines 0 comments Download
M dependencies View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M include/AdblockPlus/JsEngine.h View 1 2 5 chunks +6 lines, -5 lines 0 comments Download
M include/AdblockPlus/JsValue.h View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M libadblockplus.gyp View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M src/ConsoleJsObject.cpp View 1 2 3 chunks +14 lines, -16 lines 0 comments Download
M src/FileSystemJsObject.cpp View 1 2 2 chunks +19 lines, -36 lines 0 comments Download
M src/GlobalJsObject.cpp View 1 2 2 chunks +4 lines, -7 lines 0 comments Download
M src/JsEngine.cpp View 1 2 8 chunks +17 lines, -11 lines 0 comments Download
M src/JsValue.cpp View 1 2 3 chunks +2 lines, -7 lines 0 comments Download
M src/Utils.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/Utils.cpp View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M src/WebRequestJsObject.cpp View 1 2 2 chunks +2 lines, -5 lines 0 comments Download

Messages

Total messages: 5
sergei
Felix or Wladimir, could someone of you update our v8 repository and gyp repository. Pay ...
June 11, 2015, 1:29 p.m. (2015-06-11 13:29:08 UTC) #1
sergei
Nov. 16, 2015, 4:55 p.m. (2015-11-16 16:55:11 UTC) #2
Eric
Since I've been added as a reviewer, a few things are obvious just from the ...
Jan. 25, 2016, 2:07 p.m. (2016-01-25 14:07:16 UTC) #3
sergei
I will rebase and update this review after getting of LGTM for the dependencies. On ...
Jan. 25, 2016, 3:16 p.m. (2016-01-25 15:16:11 UTC) #4
Eric
Jan. 25, 2016, 4:02 p.m. (2016-01-25 16:02:49 UTC) #5
On 2016/01/25 15:16:11, sergei wrote:
> #1197 is libadblockplus ticket, it does not reuse IE ticket, doesn't it?

I had forgotten the details myself.

#1197 is the libadblockplus ticket to get v8 to compile with C++11 (somehow).
The decision is to do that by upgrading v8 to a newer version.

#1087 is the ABP-IE ticket to upgrade from VS 2012 to VS 2013.

> It's very important to say that we are updating v8 not only to stay current
but
> also to have it working with Visual Studio 2013. It's the reason it's not
> updated to the latest version. It took a quite long time to find those
spicific
> commit to which we can update it now.

Please distinguish between "what we're doing" and "why we're doing it". "What
we're doing" belongs in the title. "Why we're doing it" belongs in the
documentation (the commit message in this case).

> For IE specifically it will be in a review for IE repository for the change
with
> update of libadblockplus.
[...]
> It should be indeed in a separate commit, however the change
> 2012->2013 in README.md should be made here.

All of the IE-specific changes belong in change set labelled #1087.

FYI, I've just updated #1087 to require VS 2015.

> It won't be possible to compile it in C++-03 mode because v8 requries some
C++11
> feature.

I've seen a number of comments from Felix in the past about staying in C++03
(for reasons I didn't know). I'm perfectly pleased not to stay in the past.

Powered by Google App Engine
This is Rietveld