Issue 1550 - Get rid of V8ValueHolder.h

- make JsValue movable
- remove V8ValueHolder.h
- use pointer (std::unique_ptr<v8::Persistent<v8::Context>>) instead of member to avoid including v8.h in the public library header.

[Wladimir Palant, 2014-11-11 21:06:03]

This looks fine to me but I think that Felix has a better understanding of the
implications here.

[Felix Dahlke, 2015-02-05 05:18:58]

Considering that we have to stick to C++03 in libadblockplus for the time being,
we can't merge this as-is right now. But we could still get rid of V8ValueHolder
in a similar way.

However, this is very close to how things were before V8ValueHolder was
introduced in the first place. We were using std::auto_ptr instead, which
transfers ownership on copy, and thus has _roundabout_ the same semantics as
std::move on std::unique_ptr.

I'm not entirely sure _why_ V8ValueHolder was introduced frankly, it looks like
it's there to ensure Dispose() is called. If we can indeed just do this in the
JsValue destructor, we can get rid of V8ValueHolder.

So as a first step, I suggest we keep V8ValueHolder and just move the Dispose()
calls to the JsValue destructor, ensuring this actually does the trick and
doesn't cause any crashes or memory leaks.

Then we can replace V8ValueHolder by std::auto_ptr, std::tr1::shared_ptr or even
std::unique_ptr, if we wait until we can use C++11 in libadblockplus :P

[sergei, 2016-01-22 09:28:20]

rebase

[Oleksandr, 2016-01-22 12:11:15]

https://codereview.adblockplus.org/5598762307158016/diff/29334267/include/Adb...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334267/include/Adb...
include/AdblockPlus/JsValue.h:133: std::unique_ptr<v8::Persistent<v8::Value>>
value;
Aren't we leaking the value here? The idea behind V8ValueHolder was that we have
to dispose v8::Persistent handles ourselves, since they are not disposed in the
destructor (https://developers.google.com/v8/embed?hl=en). I don't see where
this value is disposed with this change.

[sergei, 2016-01-22 13:17:16]

On 2016/01/22 12:11:15, Oleksandr wrote:
>
https://codereview.adblockplus.org/5598762307158016/diff/29334267/include/Adb...
> File include/AdblockPlus/JsValue.h (right):
> 
>
https://codereview.adblockplus.org/5598762307158016/diff/29334267/include/Adb...
> include/AdblockPlus/JsValue.h:133: std::unique_ptr<v8::Persistent<v8::Value>>
> value;
> Aren't we leaking the value here? The idea behind V8ValueHolder was that we
have
> to dispose v8::Persistent handles ourselves, since they are not disposed in
the
> destructor (https://developers.google.com/v8/embed?hl=en). I don't see where
> this value is disposed with this change.

Good catch! It has been lost (see
https://codereview.adblockplus.org/5598762307158016/diff/5629499534213120/src...)
during working on https://codereview.adblockplus.org/6193234183192576/ and then
splitting it into several smaller reviews.
Fixed.

[Oleksandr, 2016-01-22 14:02:37]

LGTM with one nit.

https://codereview.adblockplus.org/5598762307158016/diff/29334338/include/Adb...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334338/include/Adb...
include/AdblockPlus/Notification.h:58: * @param jsValue `JsValuePtr`
notification JavaScript object.
The comment is leftover from JsValuePtr days

[sergei, 2016-01-22 14:34:55]

https://codereview.adblockplus.org/5598762307158016/diff/29334338/include/Adb...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334338/include/Adb...
include/AdblockPlus/Notification.h:58: * @param jsValue `JsValuePtr`
notification JavaScript object.
On 2016/01/22 14:02:37, Oleksandr wrote:
> The comment is leftover from JsValuePtr days

Thanks, fixed

[Eric, 2016-01-26 18:37:41]

I'm missing something, I think. What version of v8 does this work with? It's not
in the commit message, nor anywhere else. If there was a breaking API change in
v8 that the present change set accounts for, that needs to be documented.

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
src/FilterEngine.cpp:230: return FilterPtr(new
Filter(std::move(*func->Call(params))));
Why is 'move' necessary here? The value is already temporary.

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/JsValue...
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/JsValue...
src/JsValue.cpp:42: value->Dispose();
The previous code had "Dispose(isolate)". Was there a breaking API change in v8
that this present change set presupposes?

[sergei, 2016-01-27 10:21:15]

On 2016/01/26 18:37:41, Eric wrote:
> I'm missing something, I think. What version of v8 does this work with? It's
not
> in the commit message, nor anywhere else. If there was a breaking API change
in
> v8 that the present change set accounts for, that needs to be documented.
It works with the current version of v8, it should be clear from the description
because the newer version of v8 cannot be used with any of those visual studios.

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
src/FilterEngine.cpp:230: return FilterPtr(new
Filter(std::move(*func->Call(params))));
On 2016/01/26 18:37:40, Eric wrote:
> Why is 'move' necessary here? The value is already temporary.

Compiler generates an error here because it's not temporary. `Call` returns
std::shared_ptr<AdblockPlus::JsValue>, dereferencing of it gives a reference to
the held object.

Strictly speaking, `Call` should return rather `JsValue` (which is temporary and
JsValue&&) instead of std::shared_ptr but it introduces a lot of non-relevant
changes which IMO should be made in another commit.

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/JsValue...
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/JsValue...
src/JsValue.cpp:42: value->Dispose();
On 2016/01/26 18:37:40, Eric wrote:
> The previous code had "Dispose(isolate)". Was there a breaking API change in
v8
> that this present change set presupposes?

The implementation of Dispose(isolate) is the following one:

// TODO(dcarney): deprecate
V8_INLINE(void Dispose(Isolate* isolate)) { Dispose(); }

FYI: In a newer version of v8 the method Dispose will go away, there will be
Reset() which is called from the destructor ~UniquePersistent().

[sergei, 2016-01-27 11:23:03]

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
src/FilterEngine.cpp:230: return FilterPtr(new
Filter(std::move(*func->Call(params))));
On 2016/01/27 10:21:14, sergei wrote:
> On 2016/01/26 18:37:40, Eric wrote:
> > Why is 'move' necessary here? The value is already temporary.
> 
> Compiler generates an error here because it's not temporary. `Call` returns
> std::shared_ptr<AdblockPlus::JsValue>, dereferencing of it gives a reference
to
> the held object.
> 
> Strictly speaking, `Call` should return rather `JsValue` (which is temporary
and
> JsValue&&) instead of std::shared_ptr but it introduces a lot of non-relevant
> changes which IMO should be made in another commit.

FYI: https://codereview.adblockplus.org/29334678/

[Eric, 2016-02-08 19:50:06]

I need version numbers before I can continue on this review. Apparently its for
some version of v8 that's not what I have already in adblockplusie.

On 2016/01/27 10:21:15, sergei wrote:
> It works with the current version of v8, it should be clear from the
description
> because the newer version of v8 cannot be used with any of those visual
studios.

It's completely opaque.

"current version of v8" can means any of these five things to me:
1) The version of v8 in the current tip of libadblockplus.
2) The version of v8 in the version of libadblockplus that's in the current tip
of adblockplusie
3) Same as 2 but for Android
4) The version of v8 in the most recently shipped version of Chrome
5) The current version of v8 in it's own repository

I have a blanket clarification. Whenever I ask you "what version of v8" in any
context, now or in the future, the answer I expect consists of a *version
number*. That's a perfectly unambiguous answer.

Therefore, I'll ask you again: What is the version of v8 that this present
change set is supposed to work with? It would be useful to know the lowest such
version number, if the change seems to be permanent, or the range of valid
version numbers if they've already obsoleted it. But at the very least an answer
should include at least one valid version number.

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
src/FilterEngine.cpp:230: return FilterPtr(new
Filter(std::move(*func->Call(params))));
On 2016/01/27 10:21:14, sergei wrote:
> Compiler generates an error here because it's not temporary. `Call` returns
> std::shared_ptr<AdblockPlus::JsValue>, dereferencing of it gives a reference
to
> the held object.

OK.

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterE...
src/FilterEngine.cpp:230: return FilterPtr(new
Filter(std::move(*func->Call(params))));
On 2016/01/27 11:23:02, sergei wrote:
> FYI: https://codereview.adblockplus.org/29334678/

That change set belongs here. You're adding a bunch of 'std::move' calls here
and then immediately taking them out again. That's should really be an indicator
that the changes are closely related.

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/JsValue...
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/JsValue...
src/JsValue.cpp:42: value->Dispose();
On 2016/01/27 10:21:14, sergei wrote:
> The implementation of Dispose(isolate) is the following one:
> 
> // TODO(dcarney): deprecate
> V8_INLINE(void Dispose(Isolate* isolate)) { Dispose(); }

Clear as mud.

A version number would a much better answer. Was there a breaking API change? If
so, what was the version number where it first appeared?

> FYI: In a newer version of v8 the method Dispose will go away, there will be
> Reset() which is called from the destructor ~UniquePersistent().

What version number is that? Has it been released, or planned for the future?

[sergei, 2016-02-29 16:59:05]

On 2016/02/08 19:50:06, Eric wrote:
> I need version numbers before I can continue on this review.

This codereview is based on
> 2) The version of v8 in the version of libadblockplus that's in the current
tip

[sergei, 2016-04-05 12:06:48]

On 2016/02/29 16:59:05, sergei wrote:
> On 2016/02/08 19:50:06, Eric wrote:
> > I need version numbers before I can continue on this review.
> 
> This codereview is based on
> > 2) The version of v8 in the version of libadblockplus that's in the current
> tip

We have changed the policy, now all code reviews are based on master bookmark if
there is nothing about it in the codereview description.

[sergei, 2016-05-23 10:12:29]

rebased.
I would like to ask you to check it again just in case.
I'm going to commit it.
It compiles for android and all tests pass on linux and windows.

[Oleksandr, 2016-05-23 10:49:55]

LGTM. Looks like this is just a rebase with no extra changes.

[Oleksandr, 2016-05-23 10:49:55]

LGTM. Looks like this is just a rebase with no extra changes.

[sergei, 2016-05-23 10:52:56]

On 2016/05/23 10:49:55, Oleksandr wrote:
> LGTM. Looks like this is just a rebase with no extra changes.

Yes, basically there are neither required changes nor any files also affected by
other commits, on which it's rebased. Anyway I had compiled it and run all tests
to be sure that the everything still works (e.g. there are neither changes in
the behaviour nor in the interface).