Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1185443004:
extensions: Use V8 Maybe APIs in ModuleSystem (Closed)
|
Created:
5 years, 6 months ago by bashi Modified:
5 years, 6 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionextensions: Use V8 Maybe APIs in ModuleSystem
Replacing DEPRECATE_SOON APIs.
BUG=479065
Committed: https://crrev.com/6a4854fa7f9c3156a6621f22e10567c7d7c7c0d1
Cr-Commit-Position: refs/heads/master@{#335181}
Patch Set 1 : Add include #
Total comments: 32
Patch Set 2 : #
Total comments: 15
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 15 (5 generated)
Patchset #1 (id:1) has been deleted
bashi@chromium.org changed reviewers: + kalman@chromium.org
PTAL? A separate CL of https://codereview.chromium.org/1167423002/
Lots of comments to try to hammer out the future of these v8 bindings. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:208: return; If this isn't a string it means that JS called require(something) where something isn't a string. That's almost certainly a Chrome programmer error, so I'd rather that not be a silent failure. It would be nice to throw a JS exception then (or just LOG(FATAL); they amount to the the same thing) but there exists a bug where a third party did manage to call require(), so perhaps that isn't wise. Well, the worst they can do is crash themselves, I suppose. Something to think about. How about: if (!args[0]->IsString()) { NOTREACHED() << "require() called with a non-string argument"; return; } to at least catch Chrome programmer error to some degree. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:275: if (module_name.size() >= v8::String::kMaxLength || It would be nice to build a version of ToV8String that takes these checks into account, otherwise it'll probably regress. Possibly another version of ToV8String that takes a pointer to a handle and returns a bool? v8::Local<v8::Context> v8_module_name, v8_method_name; if (!ToV8String(GetIsolate(), module_name, &v8_module_name) || !ToV8String(GetIsolate(), method_name, &v8_method_name)) { return ...; } Overloading ToV8String in that way is probably a bad idea, so I'd either get rid of the handle-returning version, or have a name like ToV8StringUnsafe maybe? https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:278: v8::Local<v8::Primitive>(v8::Undefined(GetIsolate()))); Both the condition and body of this function are multiple lines, I think the body should be wrapped in {}. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:312: result = v8::Local<v8::Primitive>(v8::Undefined(GetIsolate())); Arghhhh so much verbosity. Why do methods like v8::Undefined and v8::Object::Get not automatically return a v8::Local rather than a v8::Handle? https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:384: v8::Local<v8::Value> v8_name; This should be the one called v8_module_name (with name --> module_name), versus what's currently called |v8_module_name| instead called |v8_k_module_name| or just inlined. It should always be safe to cast to a v8 value, it would be crazy not to be. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:502: if (internal_name.size() >= v8::String::kMaxLength) Generally I think many of these failures should have a NOTREACHED or LOG(FATAL) in their body, so as to not hide mysterious failures. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:737: resolver_local->Resolve(GetIsolate()->GetCurrentContext(), value); Can this be context() rather than GetCurrentContext()? https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system.h (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.h:54: ExceptionHandler(ScriptContext* context) : context_(context) {} explicit https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.h:59: ScriptContext* context_; Declare member variables after methods, and comment what the lifetime relationship is between it and its owner. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.h:155: static void LazyFieldGetter(v8::Local<v8::Name> property, Quick question: should these have been "const Local<Name>&" rather than "Local<Name>"? I can fix it up later if so, I'm just curious. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system_test.cc (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system_test.cc:30: FailsOnException() : ModuleSystem::ExceptionHandler(NULL) {} nullptr. In fact you might as well s/NULL/nullptr/ this file. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system_unittest.cc (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system_unittest.cc:38: : ModuleSystem::ExceptionHandler(NULL), handled_exception_(false) {} Same s/NULL/nullptr/. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... File extensions/renderer/v8_maybe_helpers.h (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:6: #define EXTENSIONS_RENDERER_V8_MAYBE_HELPERS_H_ I think just "v8_helpers" is fine, maybe extensions will want other v8 helpers that aren't necessarily to do with maybe. Also, it's fewer characters. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:11: I'm a little bit put off by a function as generic named as "SetProperty" being directly exposed in the global extensions namespace. Can we put this in a "v8_helpers" namespace, and then do "using v8_helpers" in places they're heavily used? https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:16: v8::NewStringType string_type = v8::NewStringType::kNormal) { You may also want to CHECK the string length here? https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:21: inline bool CheckV8Call(v8::Maybe<bool> maybe) { Put in an "internal" namespace? I don't see a utility of this function beyond the scope of this file, it's very specific.
Patchset #2 (id:40001) has been deleted
Thank you for review and sorry for the delay. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:208: return; On 2015/06/12 16:51:36, kalman wrote: > If this isn't a string it means that JS called require(something) where > something isn't a string. That's almost certainly a Chrome programmer error, so > I'd rather that not be a silent failure. It would be nice to throw a JS > exception then (or just LOG(FATAL); they amount to the the same thing) but there > exists a bug where a third party did manage to call require(), so perhaps that > isn't wise. > > Well, the worst they can do is crash themselves, I suppose. Something to think > about. > > How about: > > if (!args[0]->IsString()) { > NOTREACHED() << "require() called with a non-string argument"; > return; > } > > to at least catch Chrome programmer error to some degree. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:275: if (module_name.size() >= v8::String::kMaxLength || On 2015/06/12 16:51:36, kalman wrote: > It would be nice to build a version of ToV8String that takes these checks into > account, otherwise it'll probably regress. Possibly another version of > ToV8String that takes a pointer to a handle and returns a bool? > > v8::Local<v8::Context> v8_module_name, v8_method_name; > if (!ToV8String(GetIsolate(), module_name, &v8_module_name) || > !ToV8String(GetIsolate(), method_name, &v8_method_name)) { > return ...; > } Added. > > Overloading ToV8String in that way is probably a bad idea, so I'd either get rid > of the handle-returning version, or have a name like ToV8StringUnsafe maybe? Renamed ToV8String() to ToV8StringUnsafe(). https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:278: v8::Local<v8::Primitive>(v8::Undefined(GetIsolate()))); On 2015/06/12 16:51:36, kalman wrote: > Both the condition and body of this function are multiple lines, I think the > body should be wrapped in {}. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:312: result = v8::Local<v8::Primitive>(v8::Undefined(GetIsolate())); On 2015/06/12 16:51:36, kalman wrote: > Arghhhh so much verbosity. Why do methods like v8::Undefined and v8::Object::Get > not automatically return a v8::Local rather than a v8::Handle? Not sure, but maybe a historical reason? https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:384: v8::Local<v8::Value> v8_name; On 2015/06/12 16:51:36, kalman wrote: > This should be the one called v8_module_name (with name --> module_name), versus > what's currently called |v8_module_name| instead called |v8_k_module_name| or > just inlined. It should always be safe to cast to a v8 value, it would be crazy > not to be. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:502: if (internal_name.size() >= v8::String::kMaxLength) On 2015/06/12 16:51:36, kalman wrote: > Generally I think many of these failures should have a NOTREACHED or LOG(FATAL) > in their body, so as to not hide mysterious failures. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.cc:737: resolver_local->Resolve(GetIsolate()->GetCurrentContext(), value); On 2015/06/12 16:51:36, kalman wrote: > Can this be context() rather than GetCurrentContext()? Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system.h (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.h:54: ExceptionHandler(ScriptContext* context) : context_(context) {} On 2015/06/12 16:51:37, kalman wrote: > explicit Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.h:59: ScriptContext* context_; On 2015/06/12 16:51:36, kalman wrote: > Declare member variables after methods, and comment what the lifetime > relationship is between it and its owner. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system.h:155: static void LazyFieldGetter(v8::Local<v8::Name> property, On 2015/06/12 16:51:36, kalman wrote: > Quick question: should these have been "const Local<Name>&" rather than > "Local<Name>"? I can fix it up later if so, I'm just curious. IIUC, V8 handles are passed by values so it should be "v8::Local<v8::Name>". https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system_test.cc (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system_test.cc:30: FailsOnException() : ModuleSystem::ExceptionHandler(NULL) {} On 2015/06/12 16:51:37, kalman wrote: > nullptr. In fact you might as well s/NULL/nullptr/ this file. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... File extensions/renderer/module_system_unittest.cc (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/mod... extensions/renderer/module_system_unittest.cc:38: : ModuleSystem::ExceptionHandler(NULL), handled_exception_(false) {} On 2015/06/12 16:51:37, kalman wrote: > Same s/NULL/nullptr/. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... File extensions/renderer/v8_maybe_helpers.h (right): https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:6: #define EXTENSIONS_RENDERER_V8_MAYBE_HELPERS_H_ On 2015/06/12 16:51:37, kalman wrote: > I think just "v8_helpers" is fine, maybe extensions will want other v8 helpers > that aren't necessarily to do with maybe. Also, it's fewer characters. Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:11: On 2015/06/12 16:51:37, kalman wrote: > I'm a little bit put off by a function as generic named as "SetProperty" being > directly exposed in the global extensions namespace. Can we put this in a > "v8_helpers" namespace, and then do "using v8_helpers" in places they're heavily > used? Done. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:16: v8::NewStringType string_type = v8::NewStringType::kNormal) { On 2015/06/12 16:51:37, kalman wrote: > You may also want to CHECK the string length here? Added as DCHECK(). ToLocalChecked() has CHECK() so we don't need it, but I agree that having an assertion here is helpful. https://codereview.chromium.org/1185443004/diff/20001/extensions/renderer/v8_... extensions/renderer/v8_maybe_helpers.h:21: inline bool CheckV8Call(v8::Maybe<bool> maybe) { On 2015/06/12 16:51:37, kalman wrote: > Put in an "internal" namespace? I don't see a utility of this function beyond > the scope of this file, it's very specific. I'd prefer putting this in the current namespace. It is used in module_system.cc and I would use it in follow-up CLs when I replace V8 APIs in other files.
Thanks. Still getting to terms with this. Sorry about having a non-trivial 2nd round of review. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... extensions/renderer/module_system.cc:92: CreateExceptionString(try_catch) + "{" + stack_trace + "}"); Is this formatting change necessary? Either way, run "git cl format" before submitting. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... extensions/renderer/module_system.cc:284: v8::Local<v8::Primitive>(v8::Undefined(GetIsolate()))); It actually looks like Handle and Local are the same thing now: https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&sq... so this shouldn't need the additional cast to a v8::Local<v8::Primitive>. Unless there is something special about passing it to Escape? Same would go for everywhere else, of course. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... extensions/renderer/module_system.cc:391: if (!parameters->Get(context, v8_k_module_name).ToLocal(&v8_module_name)) { What do you think about adding a "GetProperty" method to v8_helpers, in a similar way to how you have a "SetProperty"? You could have a string-key version of it and so on. It would save a bunch of boilerplate in this file. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... File extensions/renderer/module_system.h (right): https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... extensions/renderer/module_system.h:60: std::string CreateExceptionString(const v8::TryCatch& try_catch); Usually there is a blank line at least between function definitions and variables, if not every item in a class declaration. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... File extensions/renderer/v8_helpers.h (right): https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:13: namespace v8_helpers { Comments in this class, and its methods, would be nice. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:20: DCHECK(strlen(str) < v8::String::kMaxLength); Seems like it should be <= really, "max length" should be inclusive? https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:32: inline bool CheckV8Call(v8::Maybe<bool> maybe) { CheckV8Call is not a good name, it sounds like this is some sort of function call. It also sounds like it does a CHECK. The comment would be along the lines of "Returns true if |maybe| is both a value, and that value is true." You could call it "IsTrue"? https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:38: v8::Local<v8::Value> key, Another suggestion: provide a version of SetProperty which takes a std::string or const char* key, since many callers seem to be calling SetProperty(..., ToV8StringUnsafe(key), ...).
https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... extensions/renderer/module_system.cc:92: CreateExceptionString(try_catch) + "{" + stack_trace + "}"); On 2015/06/16 20:52:24, kalman wrote: > Is this formatting change necessary? Either way, run "git cl format" before > submitting. Reverted. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... extensions/renderer/module_system.cc:284: v8::Local<v8::Primitive>(v8::Undefined(GetIsolate()))); On 2015/06/16 20:52:24, kalman wrote: > It actually looks like Handle and Local are the same thing now: > > https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&sq... > > so this shouldn't need the additional cast to a v8::Local<v8::Primitive>. Unless > there is something special about passing it to Escape? > > Same would go for everywhere else, of course. Done. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/mod... extensions/renderer/module_system.cc:391: if (!parameters->Get(context, v8_k_module_name).ToLocal(&v8_module_name)) { On 2015/06/16 20:52:24, kalman wrote: > What do you think about adding a "GetProperty" method to v8_helpers, in a > similar way to how you have a "SetProperty"? You could have a string-key version > of it and so on. It would save a bunch of boilerplate in this file. Done. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... File extensions/renderer/v8_helpers.h (right): https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:13: namespace v8_helpers { On 2015/06/16 20:52:25, kalman wrote: > Comments in this class, and its methods, would be nice. Done. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:20: DCHECK(strlen(str) < v8::String::kMaxLength); On 2015/06/16 20:52:25, kalman wrote: > Seems like it should be <= really, "max length" should be inclusive? Yes. Thanks for the catch. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:32: inline bool CheckV8Call(v8::Maybe<bool> maybe) { On 2015/06/16 20:52:25, kalman wrote: > CheckV8Call is not a good name, it sounds like this is some sort of function > call. It also sounds like it does a CHECK. > > The comment would be along the lines of "Returns true if |maybe| is both a > value, and that value is true." You could call it "IsTrue"? Done. https://codereview.chromium.org/1185443004/diff/60001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:38: v8::Local<v8::Value> key, On 2015/06/16 20:52:25, kalman wrote: > Another suggestion: provide a version of SetProperty which takes a std::string > or const char* key, since many callers seem to be calling SetProperty(..., > ToV8StringUnsafe(key), ...). Done.
lgtm https://codereview.chromium.org/1185443004/diff/80001/extensions/renderer/v8_... File extensions/renderer/v8_helpers.h (right): https://codereview.chromium.org/1185443004/diff/80001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:85: // GetPropertyUnsafe() family Wraps v8::Object::Get(). Thye crash when an They
Thanks! https://codereview.chromium.org/1185443004/diff/80001/extensions/renderer/v8_... File extensions/renderer/v8_helpers.h (right): https://codereview.chromium.org/1185443004/diff/80001/extensions/renderer/v8_... extensions/renderer/v8_helpers.h:85: // GetPropertyUnsafe() family Wraps v8::Object::Get(). Thye crash when an On 2015/06/17 18:25:15, kalman wrote: > They Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1185443004/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185443004/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a4854fa7f9c3156a6621f22e10567c7d7c7c0d1 Cr-Commit-Position: refs/heads/master@{#335181} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
