Issue 5034 - Part 3: Create plain JsValue instead of JsValuePtr

Issue 5034 - Part 3: Create plain JsValue instead of JsValuePtr

[hub, 2017-04-19 17:57:22]

There is also a Part 4 comming to change Evaluate() to return a plain JsValue.

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp#new...
src/JsValue.cpp:44: value.reset(new
v8::Persistent<v8::Value>(src.jsEngine->GetIsolate(), *src.value));
Here I wonder if we shouldn't change JsValue::value declaration to be a
shared_ptr<> instead of a unique_ptr<> and thus avoid creating
v8::Persistent<v8::Value>.

[sergei, 2017-04-19 18:56:52]

very good

https://codereview.adblockplus.org/29417605/diff/29417606/include/AdblockPlus...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/29417605/diff/29417606/include/AdblockPlus...
include/AdblockPlus/JsValue.h:64: JsValue& operator=(const JsValue& src);
I think it would be useful to take the comment from Clone method and add it for
the assign operator and for copy-ctr, though for the former it might should be
modified a bit.

https://codereview.adblockplus.org/29417605/diff/29417606/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/src/GlobalJsObject....
src/GlobalJsObject.cpp:76: auto value = jsEngine.NewObject();
This change is a bit controversial, but OK.

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp#new...
src/JsValue.cpp:44: value.reset(new
v8::Persistent<v8::Value>(src.jsEngine->GetIsolate(), *src.value));
On 2017/04/19 17:57:22, hub wrote:
> Here I wonder if we shouldn't change JsValue::value declaration to be a
> shared_ptr<> instead of a unique_ptr<> and thus avoid creating
> v8::Persistent<v8::Value>.

I would not like to change it to std::shared_ptr, at least so far.

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp#new...
src/JsValue.cpp:59: value->Dispose();
actually, the lock (JsContext) should be acquired before calling of Dispose. I'm
afraid there is a bug in the destructor because it's not acquiring the lock.

https://codereview.adblockplus.org/29417605/diff/29417606/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/FilterEngine.c...
test/FilterEngine.cpp:232: std::make_pair("first_run_subscription_auto_select",
jsEngine->NewValue(false)));
here and below
what about using of emplace method to simplify the syntax?

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp#n...
test/JsEngine.cpp:102: }
Actually, it would be good to test in addition that after calling of a copy-ctr
they both refer to the same JS object.

https://codereview.adblockplus.org/29417605/diff/29417606/test/Prefs.cpp
File test/Prefs.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/Prefs.cpp#newc...
test/Prefs.cpp:163:
preconfiguredPrefs.insert(std::make_pair("disable_auto_updates",
jsEngine->NewValue(false)));
emplace in this file could also a bit reduce the code.

[hub, 2017-04-19 21:56:49]

Updated patch.

I haven't figured out how check values are the same. So it is not in this patch
update. Otherwise took care of the rest.

https://codereview.adblockplus.org/29417605/diff/29417606/include/AdblockPlus...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/29417605/diff/29417606/include/AdblockPlus...
include/AdblockPlus/JsValue.h:64: JsValue& operator=(const JsValue& src);
On 2017/04/19 18:56:52, sergei wrote:
> I think it would be useful to take the comment from Clone method and add it
for
> the assign operator and for copy-ctr, though for the former it might should be
> modified a bit.

I didn't think the assignment operator overload needed to be commented.

https://codereview.adblockplus.org/29417605/diff/29417606/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/src/GlobalJsObject....
src/GlobalJsObject.cpp:76: auto value = jsEngine.NewObject();
On 2017/04/19 18:56:52, sergei wrote:
> This change is a bit controversial, but OK.

It is required for lifetime/scope reasons. Otherwise the compiler complain
"reference to temporary"

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp#new...
src/JsValue.cpp:44: value.reset(new
v8::Persistent<v8::Value>(src.jsEngine->GetIsolate(), *src.value));
On 2017/04/19 18:56:52, sergei wrote:
> On 2017/04/19 17:57:22, hub wrote:
> > Here I wonder if we shouldn't change JsValue::value declaration to be a
> > shared_ptr<> instead of a unique_ptr<> and thus avoid creating
> > v8::Persistent<v8::Value>.
> 
> I would not like to change it to std::shared_ptr, at least so far.

ok.

https://codereview.adblockplus.org/29417605/diff/29417606/src/JsValue.cpp#new...
src/JsValue.cpp:59: value->Dispose();
On 2017/04/19 18:56:52, sergei wrote:
> actually, the lock (JsContext) should be acquired before calling of Dispose.
I'm
> afraid there is a bug in the destructor because it's not acquiring the lock.

ah ok. Will fix this and the destructor.

https://codereview.adblockplus.org/29417605/diff/29417606/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/FilterEngine.c...
test/FilterEngine.cpp:232: std::make_pair("first_run_subscription_auto_select",
jsEngine->NewValue(false)));
On 2017/04/19 18:56:52, sergei wrote:
> here and below
> what about using of emplace method to simplify the syntax?

good idea.

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp#n...
test/JsEngine.cpp:102: }
On 2017/04/19 18:56:52, sergei wrote:
> Actually, it would be good to test in addition that after calling of a
copy-ctr
> they both refer to the same JS object.

Not sure how to do that. This is neither v8::Value::StrictEquals() nor
v8::Value::Equals(). Not even sure it is v8::Value::SameValue() but this API
isn't in the version we are using.

https://codereview.adblockplus.org/29417605/diff/29417606/test/Prefs.cpp
File test/Prefs.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/Prefs.cpp#newc...
test/Prefs.cpp:163:
preconfiguredPrefs.insert(std::make_pair("disable_auto_updates",
jsEngine->NewValue(false)));
On 2017/04/19 18:56:52, sergei wrote:
> emplace in this file could also a bit reduce the code.

Done.

[sergei, 2017-04-20 07:42:53]

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp#n...
test/JsEngine.cpp:102: }
On 2017/04/19 21:56:49, hub wrote:
> On 2017/04/19 18:56:52, sergei wrote:
> > Actually, it would be good to test in addition that after calling of a
> copy-ctr
> > they both refer to the same JS object.
> 
> Not sure how to do that. This is neither v8::Value::StrictEquals() nor
> v8::Value::Equals(). Not even sure it is v8::Value::SameValue() but this API
> isn't in the version we are using.

I thought about something like

auto obj = jsEngine->NewObject();
auto sameObj = obj;
EXPECT_TRUE(jsEngine->Evaluate("function(a, b){return a == b;}").Call(obj,
sameObj));

My concern was that while copying of JsValue there is no creation of a new
object. In terms of JS, it does not call something like `sameObj =
Object.assign({}, obj);` nor `sameObj = new String(obj)` when obj is a string,
etc.

https://codereview.adblockplus.org/29417605/diff/29417658/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417658/src/JsValue.cpp#new...
src/JsValue.cpp:51: const JsContext context(jsEngine);
I think it deserves a separate commit. Do you mind to do it separately?

[hub, 2017-04-20 12:41:01]

updated patch with these.

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417606/test/JsEngine.cpp#n...
test/JsEngine.cpp:102: }
On 2017/04/20 07:42:52, sergei wrote:
> On 2017/04/19 21:56:49, hub wrote:
> > On 2017/04/19 18:56:52, sergei wrote:
> > > Actually, it would be good to test in addition that after calling of a
> > copy-ctr
> > > they both refer to the same JS object.
> > 
> > Not sure how to do that. This is neither v8::Value::StrictEquals() nor
> > v8::Value::Equals(). Not even sure it is v8::Value::SameValue() but this API
> > isn't in the version we are using.
> 
> I thought about something like
> 
> auto obj = jsEngine->NewObject();
> auto sameObj = obj;
> EXPECT_TRUE(jsEngine->Evaluate("function(a, b){return a == b;}").Call(obj,
> sameObj));
> 
> My concern was that while copying of JsValue there is no creation of a new
> object. In terms of JS, it does not call something like `sameObj =
> Object.assign({}, obj);` nor `sameObj = new String(obj)` when obj is a string,
> etc.

ok. this works. and I tested the failure too :-)

https://codereview.adblockplus.org/29417605/diff/29417658/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29417658/src/JsValue.cpp#new...
src/JsValue.cpp:51: const JsContext context(jsEngine);
On 2017/04/20 07:42:52, sergei wrote:
> I think it deserves a separate commit. Do you mind to do it separately?

will do.

[sergei, 2017-04-20 12:45:08]

LGTM

https://codereview.adblockplus.org/29417605/diff/29418572/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29418572/test/JsEngine.cpp#n...
test/JsEngine.cpp:68: bool IsSame(const AdblockPlus::JsEnginePtr& jsEngine,
Can be just `JsEngine&` (without pointer).

https://codereview.adblockplus.org/29417605/diff/29418572/test/JsEngine.cpp#n...
test/JsEngine.cpp:90: ASSERT_TRUE(IsSame(jsEngine, value, value2));
I think we should rather try to use EXPECT_ when it's possible. Though he it can
be considered as consistent with already existing code.

[hub, 2017-04-20 13:01:46]

updated patch

https://codereview.adblockplus.org/29417605/diff/29418572/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29417605/diff/29418572/test/JsEngine.cpp#n...
test/JsEngine.cpp:68: bool IsSame(const AdblockPlus::JsEnginePtr& jsEngine,
On 2017/04/20 12:45:08, sergei wrote:
> Can be just `JsEngine&` (without pointer).

ok

https://codereview.adblockplus.org/29417605/diff/29418572/test/JsEngine.cpp#n...
test/JsEngine.cpp:90: ASSERT_TRUE(IsSame(jsEngine, value, value2));
On 2017/04/20 12:45:08, sergei wrote:
> I think we should rather try to use EXPECT_ when it's possible. Though he it
can
> be considered as consistent with already existing code.

I used ASSERT_ for consistency. EXPECT_ is fine too.

[sergei, 2017-04-20 13:03:15]

LGTM