Issue 6584950149087232: Issue 1280 - Update v8 (Closed)
DescriptionIMPORTANT: 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 #
MessagesTotal messages: 17
Tested on MSVS2010. It also compiles and works well with Maxthon's version of v8 (http://dl.maxthon.com/adp/v8.zip).
> Felix: > So, what I think we can do now is first update to that version, then create a Maxthon branch for libadblockplus and update to a newer version, switch to C++11 etc. ourselves. I would like to precise it a bit. I think that in the scope of this change we should first update to the closest version of v8 to the Maxthon's version, they are different, but the changes are valid for both versions. Then create a branch for Maxthon, now there should be only one commit which changes createsolution.bat to generate solution projects for MSVS2010. In our branch it should be MSVS2012 (tested, it works) because we use it now. Then we can update to the latest v8 version. It requires visual studio 2013 (https://code.google.com/p/v8/wiki/BuildingWithGYP#Visual_Studio), so before we also have to update gyp, so it can generate projects for MSVS2013. > Sergei: > I will keep it private for the nearest future, there is a link to Maxthon's v8, we might put something else in the comments, I'm afraid it can be sensitive. Also I'm not sure whether we want to have any company name in the commit message, I think it should be neutral. If somebody thinks that we should put it into issue tracker or somewhere else, please let me know.
Just did a first pass at this, still planning to take a closer look. But I've gotta run now, and this way you already have some comments you can work with. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G msvs_version=2012 -Dtarget_arch=x64 -Dhost_arch=x64 libadblockplus.gyp We generally don't align code in columns, so I'd vote against the spaces after x64. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... include/AdblockPlus/JsValue.h:64: struct CtrArg So the idea is that only friend classes can access this and thus only they can create new JsValue objects? I'm not sure if I like it, but I would definitely like to tackle this in a separate issue and focus on updating V8 and supporting Visual Studio 2010 here. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... include/AdblockPlus/JsValue.h:140: v8::Local<v8::Value> unwrapValue() const; All functions should start with an upper case letter. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... src/JsValue.cpp:35: : jsEngine(value->jsEngine) Why change the initialiser list here? It's unrelated, and now inconsistent. Same for the new empty line below. It's hard to figure out what was really changed by the commit if there are unrelated style changes. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... src/JsValue.cpp:60: auto value = unwrapValue(); We do not use C++11 in libadblockplus yet.
http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G msvs_version=2012 -Dtarget_arch=x64 -Dhost_arch=x64 libadblockplus.gyp On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > We generally don't align code in columns, so I'd vote against the spaces after > x64. I've removed such spaces. Could you explain the advantage of it? I like to align code in columns because for me it improves the readability and simplifies error checking. The used parameters as well as the difference are immediately visible, it's easier to edit them, one can use multicursor input to add or to remove parameters. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... include/AdblockPlus/JsValue.h:64: struct CtrArg Here it's merely another form of `friend class JsEngine;`, so it was here already. But I feel I need to explain the trick used here. Despite mentioned above the proper access to the members, the main requirement to use it is such functions like std::make_shared http://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared . Let's consider an example: class NotUsableWithFactoryFunctions { friend class Client; protected: // support inheritance, otherwise private NotUsableWithFactoryFunctions(){} }; class Client { void SomeMethod() { std::make_unique<NotUsableWithFactoryFunctions>(); // compilation error because `std::make_unique` does not have access to the constructor of `NotUsableWithFactoryFunctions`. } }; Adding such functions as friends of `NotUsableWithFactoryFunctions` makes no sense because in that case everybody can instantiate them. The following class allows `Client` to construct an instance of `ClientFriendly` using `std::make_unique<ClientFriendly>(ClientFriendly::PrivateArg{});` class ClientFriendly { friend class Client; protected: // support inheritance, otherwise private struct PrivateArg{}; public: explicit NotUsableWithFactoryFunctions(PrivateArg){} }; BTW, the same trick sometimes is used when one need to expose a pointer to the member but does not want the member to be callable from outside. class Button { protected: struct PrivateSignalArg{}; public: // signals void pushed(PrivateSignalArg); }; Button button; //button.pushed(); // cannot be called by anybody except `Button` and its derived. SomeModel model; model.connect(&Button::pushed, &model, &SomeModel::onButtonPushed); // OK May be the most famous example of it is in Qt5 http://woboq.com/blog/how-qt-signals-slots-work-part2-qt5.html # Protected, Public, or Private Signals. JIC, from http://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared This function is typically used to replace the construction std::shared_ptr<T>(new T(args...)) of a shared pointer from the raw pointer returned by a call to new. In contrast to that expression, std::make_shared<T> typically allocates memory for the T object and for the std::shared_ptr's control block with a single memory allocation (this is a non-binding requirement in the Standard), where std::shared_ptr<T>(new T(args...)) performs at least two memory allocations. Moreover, code such as f(std::shared_ptr<int>(new int(42)), g()) can cause a memory leak if g throws an exception because g() may be called after new int(42) and before the constructor of shared_ptr<int>. This doesn't occur in f(std::make_shared<int>(42), g()), since two function calls are never interleaved. Anyway it's just a good practice to use make_shared and make_unique instead of manual calls `new`. But it should not be blind, http://herbsutter.com/gotw/_102/ . http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... include/AdblockPlus/JsValue.h:140: v8::Local<v8::Value> unwrapValue() const; On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > All functions should start with an upper case letter. fixed http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... src/JsValue.cpp:35: : jsEngine(value->jsEngine) On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > Why change the initialiser list here? It's unrelated, and now inconsistent. Same > for the new empty line below. It's hard to figure out what was really changed by > the commit if there are unrelated style changes. Sorry, fixed. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... src/JsValue.cpp:60: auto value = unwrapValue(); On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > We do not use C++11 in libadblockplus yet. Is it a real problem or only artificial restriction? What are the exact versions of compilers we use to build libadblockplus? BTW, the recent v8 requires C++11, so does it make sense to get rid of auto here?
http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G msvs_version=2012 -Dtarget_arch=x64 -Dhost_arch=x64 libadblockplus.gyp On 2014/10/31 11:47:01, sergei wrote: > On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > > We generally don't align code in columns, so I'd vote against the spaces after > > x64. > > I've removed such spaces. > Could you explain the advantage of it? > I like to align code in columns because for me it improves the readability and > simplifies error checking. The used parameters as well as the difference are > immediately visible, it's easier to edit them, one can use multicursor input to > add or to remove parameters. I don't have a strong opinion actually. The only coding style topic really worth arguing about in my mind is consistency, and indenting on column is not something we do anywhere. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... include/AdblockPlus/JsValue.h:64: struct CtrArg On 2014/10/31 11:47:01, sergei wrote: > Here it's merely another form of `friend class JsEngine;`, so it was here > already. But I feel I need to explain the trick used here. > Despite mentioned above the proper access to the members, the main requirement > to use it is such functions like std::make_shared > http://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared . > Let's consider an example: > class NotUsableWithFactoryFunctions > { > friend class Client; > protected: // support inheritance, otherwise private > NotUsableWithFactoryFunctions(){} > }; > > class Client > { > void SomeMethod() > { > std::make_unique<NotUsableWithFactoryFunctions>(); // compilation error > because `std::make_unique` does not have access to the constructor of > `NotUsableWithFactoryFunctions`. > } > }; > > Adding such functions as friends of `NotUsableWithFactoryFunctions` makes no > sense because in that case everybody can instantiate them. > The following class allows `Client` to construct an instance of `ClientFriendly` > using `std::make_unique<ClientFriendly>(ClientFriendly::PrivateArg{});` > class ClientFriendly > { > friend class Client; > protected: // support inheritance, otherwise private > struct PrivateArg{}; > public: > explicit NotUsableWithFactoryFunctions(PrivateArg){} > }; > > BTW, the same trick sometimes is used when one need to expose a pointer to the > member but does not want the member to be callable from outside. > class Button > { > protected: > struct PrivateSignalArg{}; > public: > // signals > void pushed(PrivateSignalArg); > }; > > Button button; > //button.pushed(); // cannot be called by anybody except `Button` and its > derived. > SomeModel model; > model.connect(&Button::pushed, &model, &SomeModel::onButtonPushed); // OK > > May be the most famous example of it is in Qt5 > http://woboq.com/blog/how-qt-signals-slots-work-part2-qt5.html # Protected, > Public, or Private Signals. > > > JIC, from http://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared > This function is typically used to replace the construction > std::shared_ptr<T>(new T(args...)) of a shared pointer from the raw pointer > returned by a call to new. In contrast to that expression, std::make_shared<T> > typically allocates memory for the T object and for the std::shared_ptr's > control block with a single memory allocation (this is a non-binding requirement > in the Standard), where std::shared_ptr<T>(new T(args...)) performs at least two > memory allocations. > Moreover, code such as f(std::shared_ptr<int>(new int(42)), g()) can cause a > memory leak if g throws an exception because g() may be called after new int(42) > and before the constructor of shared_ptr<int>. This doesn't occur in > f(std::make_shared<int>(42), g()), since two function calls are never > interleaved. > > Anyway it's just a good practice to use make_shared and make_unique instead of > manual calls `new`. But it should not be blind, http://herbsutter.com/gotw/_102/ > . At first glance it seems like a rather complex hack around a missing language feature to me. But I'd like to dig into this (and how Qt5 is using it) some more before discussing it. Anyway, we really should handle this in a dedicated issue/review. I think I see the point, but I still think we shouldn't mix this up with issue 1280. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... src/JsValue.cpp:60: auto value = unwrapValue(); On 2014/10/31 11:47:01, sergei wrote: > On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > > We do not use C++11 in libadblockplus yet. > > Is it a real problem or only artificial restriction? What are the exact versions > of compilers we use to build libadblockplus? > > BTW, the recent v8 requires C++11, so does it make sense to get rid of auto > here? More of an artificial restriction in that sense. The plan is to move to C++11 in libadblockplus actually: https://issues.adblockplus.org/ticket/768 However, libadblockplus is a library, and we want to support a reasonably wide range of platforms and compilers. Hence using a new C++11 feature needs some review and testing. I'm all for that, but we shouldn't mix it in with an important issue. And IMHO it would make more sense to get rid of TR1 for good before we start to use anything from C++11.
http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G msvs_version=2012 -Dtarget_arch=x64 -Dhost_arch=x64 libadblockplus.gyp On 2014/10/31 15:07:51, Felix H. Dahlke wrote: > On 2014/10/31 11:47:01, sergei wrote: > > On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > > > We generally don't align code in columns, so I'd vote against the spaces > after > > > x64. > > > > I've removed such spaces. > > Could you explain the advantage of it? > > I like to align code in columns because for me it improves the readability and > > simplifies error checking. The used parameters as well as the difference are > > immediately visible, it's easier to edit them, one can use multicursor input > to > > add or to remove parameters. > > I don't have a strong opinion actually. The only coding style topic really worth > arguing about in my mind is consistency, and indenting on column is not > something we do anywhere. BTW, don't mix indenting and alignment, here are tabs for indentation http://2.bp.blogspot.com/-FMTWlpzeT-Y/UK94k7hZuBI/AAAAAAAAASI/VI1HzcucYT4/s16... http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/incl... include/AdblockPlus/JsValue.h:64: struct CtrArg On 2014/10/31 15:07:51, Felix H. Dahlke wrote: > On 2014/10/31 11:47:01, sergei wrote: > > Here it's merely another form of `friend class JsEngine;`, so it was here > > already. But I feel I need to explain the trick used here. > > Despite mentioned above the proper access to the members, the main requirement > > to use it is such functions like std::make_shared > > http://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared . > > Let's consider an example: > > class NotUsableWithFactoryFunctions > > { > > friend class Client; > > protected: // support inheritance, otherwise private > > NotUsableWithFactoryFunctions(){} > > }; > > > > class Client > > { > > void SomeMethod() > > { > > std::make_unique<NotUsableWithFactoryFunctions>(); // compilation error > > because `std::make_unique` does not have access to the constructor of > > `NotUsableWithFactoryFunctions`. > > } > > }; > > > > Adding such functions as friends of `NotUsableWithFactoryFunctions` makes no > > sense because in that case everybody can instantiate them. > > The following class allows `Client` to construct an instance of > `ClientFriendly` > > using `std::make_unique<ClientFriendly>(ClientFriendly::PrivateArg{});` > > class ClientFriendly > > { > > friend class Client; > > protected: // support inheritance, otherwise private > > struct PrivateArg{}; > > public: > > explicit NotUsableWithFactoryFunctions(PrivateArg){} > > }; > > > > BTW, the same trick sometimes is used when one need to expose a pointer to the > > member but does not want the member to be callable from outside. > > class Button > > { > > protected: > > struct PrivateSignalArg{}; > > public: > > // signals > > void pushed(PrivateSignalArg); > > }; > > > > Button button; > > //button.pushed(); // cannot be called by anybody except `Button` and its > > derived. > > SomeModel model; > > model.connect(&Button::pushed, &model, &SomeModel::onButtonPushed); // OK > > > > May be the most famous example of it is in Qt5 > > http://woboq.com/blog/how-qt-signals-slots-work-part2-qt5.html # Protected, > > Public, or Private Signals. > > > > > > JIC, from http://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared > > This function is typically used to replace the construction > > std::shared_ptr<T>(new T(args...)) of a shared pointer from the raw pointer > > returned by a call to new. In contrast to that expression, std::make_shared<T> > > typically allocates memory for the T object and for the std::shared_ptr's > > control block with a single memory allocation (this is a non-binding > requirement > > in the Standard), where std::shared_ptr<T>(new T(args...)) performs at least > two > > memory allocations. > > Moreover, code such as f(std::shared_ptr<int>(new int(42)), g()) can cause a > > memory leak if g throws an exception because g() may be called after new > int(42) > > and before the constructor of shared_ptr<int>. This doesn't occur in > > f(std::make_shared<int>(42), g()), since two function calls are never > > interleaved. > > > > Anyway it's just a good practice to use make_shared and make_unique instead of > > manual calls `new`. But it should not be blind, > http://herbsutter.com/gotw/_102/ > > . > > At first glance it seems like a rather complex hack around a missing language > feature to me. But I'd like to dig into this (and how Qt5 is using it) some more > before discussing it. > > Anyway, we really should handle this in a dedicated issue/review. > > > I think I see the point, but I still think we shouldn't mix this up with issue > 1280. reverted. JIC, it's mainly for std::make_{shared,unique} http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/src/... src/JsValue.cpp:60: auto value = unwrapValue(); On 2014/10/31 15:07:51, Felix H. Dahlke wrote: > On 2014/10/31 11:47:01, sergei wrote: > > On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > > > We do not use C++11 in libadblockplus yet. > > > > Is it a real problem or only artificial restriction? What are the exact > versions > > of compilers we use to build libadblockplus? > > > > BTW, the recent v8 requires C++11, so does it make sense to get rid of auto > > here? > > More of an artificial restriction in that sense. The plan is to move to C++11 in > libadblockplus actually: https://issues.adblockplus.org/ticket/768 > > However, libadblockplus is a library, and we want to support a reasonably wide > range of platforms and compilers. Hence using a new C++11 feature needs some > review and testing. I'm all for that, but we shouldn't mix it in with an > important issue. And IMHO it would make more sense to get rid of TR1 for good > before we start to use anything from C++11. fixed
Alright, took a proper close look this time, looking good! Just some Friday afternoon nit picking. http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... File createsolution.bat (right): http://codereview.adblockplus.org/6584950149087232/diff/5649050225344512/crea... createsolution.bat:7: python msvs_gyp_wrapper.py --depth=build\x64 -f msvs -I common.gypi --generator-output=build\x64 -G msvs_version=2012 -Dtarget_arch=x64 -Dhost_arch=x64 libadblockplus.gyp On 2014/10/31 16:13:35, sergei wrote: > On 2014/10/31 15:07:51, Felix H. Dahlke wrote: > > On 2014/10/31 11:47:01, sergei wrote: > > > On 2014/10/31 05:31:57, Felix H. Dahlke wrote: > > > > We generally don't align code in columns, so I'd vote against the spaces > > after > > > > x64. > > > > > > I've removed such spaces. > > > Could you explain the advantage of it? > > > I like to align code in columns because for me it improves the readability > and > > > simplifies error checking. The used parameters as well as the difference are > > > immediately visible, it's easier to edit them, one can use multicursor input > > to > > > add or to remove parameters. > > > > I don't have a strong opinion actually. The only coding style topic really > worth > > arguing about in my mind is consistency, and indenting on column is not > > something we do anywhere. > > BTW, don't mix indenting and alignment, here are tabs for indentation > http://2.bp.blogspot.com/-FMTWlpzeT-Y/UK94k7hZuBI/AAAAAAAAASI/VI1HzcucYT4/s16... Yeah, aligning it is. http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... include/AdblockPlus/JsValue.h:59: JsValue(const JsValuePtr& value); There's already a protected block below, why not move that there? http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... include/AdblockPlus/JsValue.h:129: private: Same here, why add another private block when there's one below? http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... File include/AdblockPlus/V8ValueHolder.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... include/AdblockPlus/V8ValueHolder.h:57: void reset(v8::Isolate* isolate = nullptr, std::auto_ptr<V8Persistent> value = std::auto_ptr<V8Persistent>(new V8Persistent())) nullptr is also C++11, I figure we'd better stick to 0 until we switch. http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/src/... src/JsValue.cpp:207: const JsContext context(jsEngine); Why did you move this up? Doesn't it make more sense to keep the definition closer to it's usage here and check for the error first?
http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... include/AdblockPlus/JsValue.h:59: JsValue(const JsValuePtr& value); On 2014/10/31 16:30:15, Felix H. Dahlke wrote: > There's already a protected block below, why not move that there? Sure, I can move. Do we have something in some style guide regarding it? Like firstly public, protected and private methods then variables with the same visibility order? http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... File include/AdblockPlus/V8ValueHolder.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... include/AdblockPlus/V8ValueHolder.h:57: void reset(v8::Isolate* isolate = nullptr, std::auto_ptr<V8Persistent> value = std::auto_ptr<V8Persistent>(new V8Persistent())) On 2014/10/31 16:30:15, Felix H. Dahlke wrote: > nullptr is also C++11, I figure we'd better stick to 0 until we switch. fixed http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/src/... src/JsValue.cpp:207: const JsContext context(jsEngine); On 2014/10/31 16:30:15, Felix H. Dahlke wrote: > Why did you move this up? Doesn't it make more sense to keep the definition > closer to it's usage here and check for the error first? Moved back, it was done during the intermediate debugging state.
http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... include/AdblockPlus/JsValue.h:59: JsValue(const JsValuePtr& value); On 2014/10/31 17:14:17, sergei wrote: > On 2014/10/31 16:30:15, Felix H. Dahlke wrote: > > There's already a protected block below, why not move that there? > > Sure, I can move. Do we have something in some style guide regarding it? Like > firstly public, protected and private methods then variables with the same > visibility order? No, wasn't necessary to have that in the style so far, Mozilla doesn't have it either. It's more about consistency with the surrounding code: JsValue.h (and any other header in libadblockplus at least) only have one public/protected/private block each.
I'd strongly prefer you to remove all unnecessary changes from this review. While these changes might be good ones, they are making this review unnecessarily complicated - they are much better reviewed separately, without time pressure. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... include/AdblockPlus/JsValue.h:134: private: Nit: Why not merge the two private sections? http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... File include/AdblockPlus/V8ValueHolder.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... include/AdblockPlus/V8ValueHolder.h:55: reset(isolate, std::auto_ptr<V8Persistent>(new V8Persistent(isolate, value))); Nit: two spaces for indentation please. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/DefaultWebRequestWinInet.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... 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); Nit: no whitespace before comma please. Either way, I don't see how the changes in this file are related. 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()))); Why do you need to create a new handle for the context? The isolate is guaranteed to be the same (see two lines above), why not access the global object via the original handle? Nit: the line break is there to keep the lines within 80 characters. Your additions make this line too long again, meaning that you need to add a line break somewhere. Or even better: create a temporary variable for your context clone. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/JsValue.cpp:26: using namespace AdblockPlus; This is an unrelated change that makes this unnecessarily hard to review. Please always separate such cosmetic changes into their own commit/review - they are trivial to review then, the reviewer knows that it's all merely search&replace and doesn't need to search for the "real" changes in there. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/JsValue.cpp:214: thisPtr = JsValuePtr(new JsValue(jsEngine, localContext->Global())); As before, I don't understand the necessity of creating a new context handle here. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/Utils.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/Utils.cpp:49: v8::String::NewStringType::kInternalizedString, str.length()); I doubt that using internalized strings makes sense here - chances are that we have a string that will only be used once and never again, and we will waste resources unnecessarily on internalizing it. The default is kNormalString. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/Utils.cpp:102: } The changes to this function are again completely unrelated.
http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/DefaultWebRequestWinInet.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... 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); On 2014/11/01 22:30:25, Wladimir Palant wrote: > Nit: no whitespace before comma please. > > Either way, I don't see how the changes in this file are related. Missed the white space, agreed. As for this change, it's explained in the description: DefaultWebRequestWinInet - std::basic_string::operator[i] has an assert in MSVS2010 which verifies the size of string, in our case the size is zero and we should not call operator[]. The call of WinHttpQueryHeaders is adapted. Since this is, how I see it, not the only change for supporting Visual Studio 2010, I accepted that we mix these two issues: V8 update and Visual Studio 2010. I fully agree that it'd be much cleaner to do it in separate issues/commits. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/Utils.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/Utils.cpp:49: v8::String::NewStringType::kInternalizedString, str.length()); On 2014/11/01 22:30:25, Wladimir Palant wrote: > I doubt that using internalized strings makes sense here - chances are that we > have a string that will only be used once and never again, and we will waste > resources unnecessarily on internalizing it. The default is kNormalString. Somehow thought kInternalizedString was the default and this was just an API change for the V8 update. I agree that: a) We should not mix this up with issue 1280 if it's not necessary for the V8 update. b) String interning doesn't seem to make sense for us as Wladimir said, we're dealing with a huge number of unique strings. http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/Utils.cpp:102: } On 2014/11/01 22:30:25, Wladimir Palant wrote: > The changes to this function are again completely unrelated. I actually presumed switching to UrlCanonicalizeW was somehow necessary for the V8 update, but as the description says, it's just unrelated. Let's please not mix up dozens of unrelated changes in one commit, particularly not for a P2 issue that could cause tons of regressions already, being a dependency update.
Just noticed that the review is private - it can be public alright. The issue is non-confidential.
I see that some style stuff like 80 characters width, spaces, new lines and indentation happens in almost all reviews. As well as some violations had somehow passed through review already. It's not urgent but if we really care about it I would like you to keep it mind and may be in some future we can start to use some kind of cpplint. Now it only takes time of the reviewers and pollutes comments. http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5643440998055936/incl... include/AdblockPlus/JsValue.h:59: JsValue(const JsValuePtr& value); On 2014/11/01 04:45:40, Felix H. Dahlke wrote: > On 2014/10/31 17:14:17, sergei wrote: > > On 2014/10/31 16:30:15, Felix H. Dahlke wrote: > > > There's already a protected block below, why not move that there? > > > > Sure, I can move. Do we have something in some style guide regarding it? Like > > firstly public, protected and private methods then variables with the same > > visibility order? > > No, wasn't necessary to have that in the style so far, Mozilla doesn't have it > either. It's more about consistency with the surrounding code: JsValue.h (and > any other header in libadblockplus at least) only have one > public/protected/private block each. Merged http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... File include/AdblockPlus/JsValue.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... include/AdblockPlus/JsValue.h:134: private: On 2014/11/01 22:30:25, Wladimir Palant wrote: > Nit: Why not merge the two private sections? merged http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... File include/AdblockPlus/V8ValueHolder.h (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/incl... include/AdblockPlus/V8ValueHolder.h:55: reset(isolate, std::auto_ptr<V8Persistent>(new V8Persistent(isolate, value))); On 2014/11/01 22:30:25, Wladimir Palant wrote: > Nit: two spaces for indentation please. fixed http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/DefaultWebRequestWinInet.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... 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); reverted 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()))); On 2014/11/01 22:30:25, Wladimir Palant wrote: > Why do you need to create a new handle for the context? The isolate is > guaranteed to be the same (see two lines above), why not access the global > object via the original handle? Not sure that I correctly understand what you mean. Could you write a line of expected code? If you mean something like `result->context->Global()` then it's not impossible with the new version of v8. The API of v8::Persistent has changed with the updating of v8. There is only one way to obtain a handle from persistent, which is to create the handle passing persistent into handle constructor. > > Nit: the line break is there to keep the lines within 80 characters. Your > additions make this line too long again, meaning that you need to add a line > break somewhere. Or even better: create a temporary variable for your context > clone. fixed http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/JsValue.cpp:214: thisPtr = JsValuePtr(new JsValue(jsEngine, localContext->Global())); On 2014/11/01 22:30:25, Wladimir Palant wrote: > As before, I don't understand the necessity of creating a new context handle > here. Discussion is here http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... File src/Utils.cpp (right): http://codereview.adblockplus.org/6584950149087232/diff/5137785908363264/src/... src/Utils.cpp:49: v8::String::NewStringType::kInternalizedString, str.length()); On 2014/11/02 03:55:20, Felix H. Dahlke wrote: > On 2014/11/01 22:30:25, Wladimir Palant wrote: > > I doubt that using internalized strings makes sense here - chances are that we > > have a string that will only be used once and never again, and we will waste > > resources unnecessarily on internalizing it. The default is kNormalString. > > Somehow thought kInternalizedString was the default and this was just an API > change for the V8 update. I agree that: > > a) We should not mix this up with issue 1280 if it's not necessary for the V8 > update. > b) String interning doesn't seem to make sense for us as Wladimir said, we're > dealing with a huge number of unique strings. Thanks, I see what it means, anyway it's not important for that update, so I've reverted it back.
On 2014/11/03 12:58:19, sergei wrote: > I see that some style stuff like 80 characters width, spaces, new lines and > indentation happens in almost all reviews. As well as some violations had > somehow passed through review already. It's not urgent but if we really care > about it I would like you to keep it mind and may be in some future we can start > to use some kind of cpplint. Now it only takes time of the reviewers and > pollutes comments. For some reason, following a style guide and keeping code consistent seems to be a persistent problem for people working on our C++ projects - not an issue anywhere else really. I still have some hopes that we don't need to resort to a mandatory linter to tackle this, let's give it a bit more time. Regarding the 80 column rule: We're not strict about enforcing it, it's highly inconsistent anyway. But when editing existing code that adheres to it, it should not be violated.
LGTM, assuming that we indeed need to create a new local handle for context before in order to retrieve the global object. 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()))); On 2014/11/03 12:58:19, sergei wrote: > On 2014/11/01 22:30:25, Wladimir Palant wrote: > > Why do you need to create a new handle for the context? The isolate is > > guaranteed to be the same (see two lines above), why not access the global > > object via the original handle? > Not sure that I correctly understand what you mean. Could you write a line of > expected code? If you mean something like `result->context->Global()` then it's > not impossible with the new version of v8. The API of v8::Persistent has changed > with the updating of v8. There is only one way to obtain a handle from > persistent, which is to create the handle passing persistent into handle > constructor. > > > > > Nit: the line break is there to keep the lines within 80 characters. Your > > additions make this line too long again, meaning that you need to add a line > > break somewhere. Or even better: create a temporary variable for your context > > clone. > fixed I see. Looked through the docs a bit, unfortunately I don't have the time to dig into this more right now, so I've got to ask: How I see it, v8::Persistent still inherits operator->() from v8::Handle - why can't we use it for calling Global() anymore? What was the API change?
On 2014/11/03 15:46:23, Felix H. Dahlke wrote: > LGTM, assuming that we indeed need to create a new local handle for context > before in order to retrieve the global object. > > 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()))); > On 2014/11/03 12:58:19, sergei wrote: > > On 2014/11/01 22:30:25, Wladimir Palant wrote: > > > Why do you need to create a new handle for the context? The isolate is > > > guaranteed to be the same (see two lines above), why not access the global > > > object via the original handle? > > Not sure that I correctly understand what you mean. Could you write a line of > > expected code? If you mean something like `result->context->Global()` then > it's > > not impossible with the new version of v8. The API of v8::Persistent has > changed > > with the updating of v8. There is only one way to obtain a handle from > > persistent, which is to create the handle passing persistent into handle > > constructor. > > > > > > > > Nit: the line break is there to keep the lines within 80 characters. Your > > > additions make this line too long again, meaning that you need to add a line > > > break somewhere. Or even better: create a temporary variable for your > context > > > clone. > > fixed > > I see. Looked through the docs a bit, unfortunately I don't have the time to dig > into this more right now, so I've got to ask: How I see it, v8::Persistent still > inherits operator->() from v8::Handle - why can't we use it for calling Global() > anymore? What was the API change? I see in https://code.google.com/p/v8/source/browse/trunk/include/v8.h?r=16196#498 ``` template <class T> class Persistent // NOLINT #ifdef V8_USE_UNSAFE_HANDLES : public Handle<T> { #else ``` but V8_USE_UNSAFE_HANDLES is not defined, so there is no inheritance. Just in case, in the latest version the inheritance option is completely removed (https://code.google.com/p/v8/source/browse/trunk/include/v8.h#453), so we will have to do it that way anyway. What we can do only in JsEngine::New is something like ``` v8::Local<v8::Context> v8Context = v8::Context::New(result->isolate); result->context.reset(result->isolate, v8Context); v8::Local<v8::Object> globalContext = v8Context->Global(); ```
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. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
