Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2164763005:
Library custom formatters (Closed)
|
Created:
4 years, 5 months ago by bmilligan Modified:
4 years, 5 months ago CC:
dev-compiler+reviews_dartlang.org, priscillalee Base URL:
https://github.com/dart-lang/dev_compiler.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionLibrary custom formatters
R=alanknight@google.com, jacobr@google.com, jmesserly@google.com
Committed: https://github.com/dart-lang/dev_compiler/commit/98581cc03b91bcabca7473db0fbdf5f194860c42
Patch Set 1 #
Total comments: 47
Patch Set 2 : Pub upgrade #
Total comments: 44
Patch Set 3 : Generic type recognition and cleanup #Patch Set 4 : Delete dart_sdk.js~ #Patch Set 5 : Removal of generic argument parenthesis #
Total comments: 8
Patch Set 6 : CL changes #Patch Set 7 : CL cleanup #Patch Set 8 : Custom formatter branch #Patch Set 9 : Revert tests #Patch Set 10 : Revert pubspec.lock #Patch Set 11 : Presubmit checks test file updates #Patch Set 12 : Revert test files #
Messages
Total messages: 19 (3 generated)
bmilligan@google.com changed reviewers: + alanknight@google.com, jacobr@google.com, jmesserly@google.com
John, take a look at the changes I've made excluding debugger.dart. Jacob and Alan can review debugger.dart. Thanks! https://codereview.chromium.org/2164763005/diff/1/lib/runtime/dart_library.js File lib/runtime/dart_library.js (right): https://codereview.chromium.org/2164763005/diff/1/lib/runtime/dart_library.js... lib/runtime/dart_library.js:19: const dartLibraryName = Symbol("dartLibraryName"); John - Not sure where this Symbol should go. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:749: String preview(object) => 'Named Constructor'; I'm working on getting this to display the signature of the constructor, but that process is more in depth. I thought I would submit the custom formatters now to get the ball rolling and add that feature later.
I didn't make it all the way through the file before I had to leave but here are some comments to get you started. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (left): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:442: new NameValuePair(name: '[[class]]', value: new ClassMetadata(object))); btw the reason I have to create a ClassMetadata object here is that object is an actual instance object and doesn't have some symbol on the object indicating it is a ClassMetadata object. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:146: iterable.skip(start).take(length).toList().asMap(); nice! https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:173: class Library { do you need to wrap the libraries? I would expect the symbols added will let you just tell immediately that an object is a library or a named constructor? https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:352: // precedence. good catch :) https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:485: Object getPropertyValue(Object object, String name) { call this safeGetProperty and make it a top level member. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:518: if (symbolName(symbol) == 'dartLibraryName') { you will be able to match the actual symbol instead of just the symbols name now that you have the unique symbol instance stored in the rtti library. You will also be able to simplify and not have to walk the proto chain as a single getProperty call will be fine. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:530: var libraryNameArray = libraryName.split('/'); I think libraryNameParts would be cleaner https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:531: if (libraryNameArray.length > 1) { comment why you are stripping the last entry https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:532: libraryNameArray[libraryNameArray.length - 1] = ''; libraryNameParts.last = '' https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:541: name = name.replaceAll('__', '/') + '.dart'; prefer string interoplation. So name = "${name.replaceAll('__', '/')}.dart";
Description was changed from ========== Library custom formatters Merge branch 'master' of https://github.com/dart-lang/dev_compiler return type fix Nit fixes Updated IterableSpan Custom formatter popup booleans Merge branch 'master' of https://github.com/dart-lang/dev_compiler Private name in loaded LibraryLoader Debugger library import with name Merge branch 'master' of https://github.com/dart-lang/dev_compiler Merge branch 'master' of https://github.com/dart-lang/dev_compiler setter for testing maxspanlength setter Merge branch 'master' of https://github.com/dart-lang/dev_compiler Merge branch 'master' of https://github.com/dart-lang/dev_compiler comments and 10001 case naming and logic changes logic updates and variable renaming Merge branch 'master' of https://github.com/dart-lang/dev_compiler Array formatting customized to look like JS BUG= ========== to ========== Library custom formatters ==========
the DDC changes LGTM https://codereview.chromium.org/2164763005/diff/1/test/codegen_expected/path/... File test/codegen_expected/path/path.js (left): https://codereview.chromium.org/2164763005/diff/1/test/codegen_expected/path/... test/codegen_expected/path/path.js:3: ], function load__path(exports, dart_sdk) { it may be worth trying "pub upgrade". I think this diffs in generated files are based on some package differences. But it could be the checked in one is the older one, in which case it's good to update. Not sure :)
https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:564: if (JSNative.getProperty(value, 'name') == 'makeGenericType') { this is fragile. is there not an existing symbol that indicates that a type is generic? If not, we should add one. Check with John on this. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:652: ret.addAll((new IterableSpan(0, object.length, object)).children()); remove unneeded extra set of parens. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:661: class ClassMetadataFormatter extends ObjectFormatter { why is this now an extends instead of implements relationship? generally prefer implements instead of extends unless there is a strong reason to use inheritance. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:664: _getType(object) { is the return type of this method Type? https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:701: if (name == 'length' || name == 'name' || name == 'prototype') continue; why are we removing length and name? I would add a TODO to perform more principled checks to filter out spurious members. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:706: name = entry.name + '.' + name; I think it is cleaner to write name = '${entry.name}.${name}'; https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:718: name == r'$identityHash') { $identityHash is a little scary. Add a TODO to not hard code that one. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:726: if (symbolName(symbol) == 'sig') { should not be using symbolName. You should be able to get the actual 'sig' symbol. That will avoid the enumeration of the symbols and be much more efficient. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:749: String preview(object) => 'Named Constructor'; On 2016/07/21 01:59:27, bmilligan wrote: > I'm working on getting this to display the signature of the constructor, but > that process is more in depth. I thought I would submit the custom formatters > now to get the ball rolling and add that feature later. Sounds good.
https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:564: if (JSNative.getProperty(value, 'name') == 'makeGenericType') { this is fragile. is there not an existing symbol that indicates that a type is generic? If not, we should add one. Check with John on this. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:652: ret.addAll((new IterableSpan(0, object.length, object)).children()); remove unneeded extra set of parens. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:661: class ClassMetadataFormatter extends ObjectFormatter { why is this now an extends instead of implements relationship? generally prefer implements instead of extends unless there is a strong reason to use inheritance. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:701: if (name == 'length' || name == 'name' || name == 'prototype') continue; why are we removing length and name? I would add a TODO to perform more principled checks to filter out spurious members. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:718: name == r'$identityHash') { $identityHash is a little scary. Add a TODO to not hard code that one. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:726: if (symbolName(symbol) == 'sig') { should not be using symbolName. You should be able to get the actual 'sig' symbol. That will avoid the enumeration of the symbols and be much more efficient. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:749: String preview(object) => 'Named Constructor'; On 2016/07/21 01:59:27, bmilligan wrote: > I'm working on getting this to display the signature of the constructor, but > that process is more in depth. I thought I would submit the custom formatters > now to get the ball rolling and add that feature later. Sounds good.
https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:564: if (JSNative.getProperty(value, 'name') == 'makeGenericType') { On 2016/07/21 16:19:00, Jacob wrote: > this is fragile. is there not an existing symbol that indicates that a type is > generic? If not, we should add one. Check with John on this. yeah there should be, I know we track type args on there. Probably can look at how it's printed by the typeName code.
Also, I assume the backup file dart_sdk.js~ shouldn't be there. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:485: Object getPropertyValue(Object object, String name) { It's better to avoid "get" as a prefix (even though Java and JavaScript both do it all the time), and "value" doesn't really tell us anything more. See go/longnamesarelong I'd probably call these property and protoChain. I could see getProperty or propertyNamed, since property by itself isn't very specific. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:496: /// to avoid properties that are Dart methods. What does the comment mean about being careful to avoid properties that are Dart methods? I don't see anything that looks like that. Or does it mean avoiding the possibility of a Dart method named __proto__? https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:497: List<Object> getProtoChain(var current) { Don't put "var" on a method parameter, either leave it blank or put Object or explicitly make it dynamic. Probably blank is fine in this case. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:502: protoChain.add(current); Naming an input parameter "current" feels odd. Also, perhaps as a hangover from ancient programming languages, assigning to an input parameter makes me uncomfortable. So I'd probably give it a different name and assign it to a separately declared current. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:509: /// Formatter for module Dart Library objects. What's the difference between a module dart library and a dart library? https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:516: for (current in protoChain) { Using a for loop variable that's declared and initialized elsewhere makes me even more uncomfortable. It works(I think) it just feels uncommon enough that someone reading the code could easily miss it. You could concatenate the collections, or make getProtoChain also return the original object if this is its main use. Another approach would be to split out a separate _acceptExactly(object) (or some better name meaning exactly that object, not checking prototypes). Then this can be something like return _acceptExactly(object) || getProtoChain(current).any((proto) => _acceptExactly(proto)); which also avoids computing the proto chain if we accept the object directly. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:519: libraryName = JSNative.getProperty(current, symbol); We now have a function for this, so we should use it. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:530: var libraryNameArray = libraryName.split('/'); Better to just call this "libraryNames", makes the expressions a little more readable, and equally obvious that it's a collection. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:538: var properties = new LinkedHashSet<NameValuePair>(); These are linked because we want to preserve the JS order? I think devtools normally displays things alphabetically, so maybe it would be better to use a regular set and sort. Or maybe the JS order is already alphabetical? Not sure. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:540: var value = JSNative.getProperty(object, name); Shouldn't this, and similarly below be calling our wrapper function that catches exceptions? https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:541: name = name.replaceAll('__', '/') + '.dart'; A comment for the replacement would be good. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:558: var properties = new LinkedHashSet<NameValuePair>(); Should this just be called "children"? https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:674: var ret = getTypeName(type); var typeName? https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:676: var typeNames = implements().map((type) => getTypeName(type)); If you're just calling a one argument function/method in a map, where the argument is the loop variable, you can just use the function as a tear-off. implements().map(getTypeName); https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:677: return ret + ' implements ${typeNames.join(", ")}'; Really nitpicking it's slightly better to use an interpolation for the whole business rather than a concatenation. So "$ret implements ..." https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:691: var ret = <NameValuePair>[]; Also better as "children" https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:701: if (name == 'length' || name == 'name' || name == 'prototype') continue; This would be better in a separate list of properties we don't show. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:717: name == 'new' || Also better in a separate list. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:724: var constructor = getPropertyValue(prototype, 'constructor'); Also, I would think, call our wrapper for this. I presume we'd want to catch any exceptions here. There's probably a general issue there of ensuring robustness in our debugger if we get an exception inside our own formatters, but that's more of a polish item that we should note and deal with later.
https://codereview.chromium.org/2164763005/diff/1/test/codegen_expected/path/... File test/codegen_expected/path/path.js (left): https://codereview.chromium.org/2164763005/diff/1/test/codegen_expected/path/... test/codegen_expected/path/path.js:3: ], function load__path(exports, dart_sdk) { On 2016/07/21 13:19:57, John Messerly wrote: > it may be worth trying "pub upgrade". I think this diffs in generated files are > based on some package differences. But it could be the checked in one is the > older one, in which case it's good to update. Not sure :) I ran pub upgrade and git pull. Does it still look off? Maybe it is an older checked in version. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (left): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:442: new NameValuePair(name: '[[class]]', value: new ClassMetadata(object))); On 2016/07/21 02:10:31, Jacob wrote: > btw the reason I have to create a ClassMetadata object here is that > object is an actual instance object and doesn't have some symbol on the object > indicating it is a ClassMetadata object. Acknowledged. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:146: iterable.skip(start).take(length).toList().asMap(); On 2016/07/21 02:10:31, Jacob wrote: > nice! Acknowledged. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:173: class Library { On 2016/07/21 02:10:32, Jacob wrote: > do you need to wrap the libraries? I would expect the symbols added will let you > just tell immediately that an object is a library or a named constructor? The symbol for the libraries is on the library module, not the libraries themselves. The library module is identified that way, but there's no tag on its children libraries. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:352: // precedence. On 2016/07/21 02:10:31, Jacob wrote: > good catch :) Acknowledged. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:485: Object getPropertyValue(Object object, String name) { On 2016/07/21 02:10:31, Jacob wrote: > call this > safeGetProperty > and make it a top level member. Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:518: if (symbolName(symbol) == 'dartLibraryName') { On 2016/07/21 02:10:31, Jacob wrote: > you will be able to match the actual symbol instead of just the symbols name now > that you have the unique symbol instance stored in the rtti library. > > You will also be able to simplify and not have to walk the proto chain as a > single getProperty call will be fine. Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:530: var libraryNameArray = libraryName.split('/'); On 2016/07/21 02:10:31, Jacob wrote: > I think > libraryNameParts would be cleaner Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:531: if (libraryNameArray.length > 1) { On 2016/07/21 02:10:31, Jacob wrote: > comment why you are stripping the last entry Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:532: libraryNameArray[libraryNameArray.length - 1] = ''; On 2016/07/21 02:10:31, Jacob wrote: > libraryNameParts.last = '' '.last' cannot be used as a setter, which is why I used the indexing. The API says .last is read-only. :/ https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:541: name = name.replaceAll('__', '/') + '.dart'; On 2016/07/21 02:10:31, Jacob wrote: > prefer string interoplation. So > name = "${name.replaceAll('__', '/')}.dart"; Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:564: if (JSNative.getProperty(value, 'name') == 'makeGenericType') { On 2016/07/21 16:19:00, Jacob wrote: > this is fragile. is there not an existing symbol that indicates that a type is > generic? If not, we should add one. Check with John on this. Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:564: if (JSNative.getProperty(value, 'name') == 'makeGenericType') { On 2016/07/21 16:20:25, John Messerly wrote: > On 2016/07/21 16:19:00, Jacob wrote: > > this is fragile. is there not an existing symbol that indicates that a type is > > generic? If not, we should add one. Check with John on this. > > yeah there should be, I know we track type args on there. Probably can look at > how it's printed by the typeName code. Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:652: ret.addAll((new IterableSpan(0, object.length, object)).children()); On 2016/07/21 16:18:59, Jacob wrote: > remove unneeded extra set of parens. Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:661: class ClassMetadataFormatter extends ObjectFormatter { On 2016/07/21 16:18:59, Jacob wrote: > why is this now an extends instead of implements relationship? > generally prefer implements instead of extends unless there is a strong reason > to use inheritance. There was a function I had in ObjectFormatter that I used here. That function is now top level, which I should have done originally. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:664: _getType(object) { On 2016/07/21 16:18:58, Jacob wrote: > is the return type of this method > Type? rtti.dart says getReifiedType "returns the runtime representation of the type of obj. This is different from the user-visible Type object returned by calling 'runtimeType' on some Dart object". There are no return types defined in rtti, so the best I can assume is that it returns an Object? https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:701: if (name == 'length' || name == 'name' || name == 'prototype') continue; On 2016/07/21 16:19:00, Jacob wrote: > why are we removing length and name? > I would add a TODO to perform more principled checks to filter out spurious > members. Length is always = 0, and name is redundant as it's already displayed in the parent node. I thought they were unnecessary properties to display, but could delete those filters. I should give you credit for that TODO. It's spurious of me to pretend I use the word spurious. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:706: name = entry.name + '.' + name; On 2016/07/21 16:18:58, Jacob wrote: > I think it is cleaner to write > name = '${entry.name}.${name}'; Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:718: name == r'$identityHash') { On 2016/07/21 16:18:59, Jacob wrote: > $identityHash is a little scary. Add a TODO to not hard code that one. Done. Is it because the name is fragile? https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:726: if (symbolName(symbol) == 'sig') { On 2016/07/21 16:18:59, Jacob wrote: > should not be using symbolName. You should be able to get the actual 'sig' > symbol. > That will avoid the enumeration of the symbols and be much more efficient. Done. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:749: String preview(object) => 'Named Constructor'; On 2016/07/21 16:18:59, Jacob wrote: > On 2016/07/21 01:59:27, bmilligan wrote: > > I'm working on getting this to display the signature of the constructor, but > > that process is more in depth. I thought I would submit the custom formatters > > now to get the ball rolling and add that feature later. > > Sounds good. Acknowledged. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:485: Object getPropertyValue(Object object, String name) { On 2016/07/21 17:44:48, Alan Knight wrote: > It's better to avoid "get" as a prefix (even though Java and JavaScript both do > it all the time), and "value" doesn't really tell us anything more. See > go/longnamesarelong > > I'd probably call these property and protoChain. I could see getProperty or > propertyNamed, since property by itself isn't very specific. Jacob suggested safeGetProperty. Are you referring to the object being renamed as protoChain? https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:496: /// to avoid properties that are Dart methods. On 2016/07/21 17:44:49, Alan Knight wrote: > What does the comment mean about being careful to avoid properties that are Dart > methods? I don't see anything that looks like that. Or does it mean avoiding the > possibility of a Dart method named __proto__? This function existed previously, I just moved it to the Object Formatter class. I'm not sure who wrote that comment or what it means. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:497: List<Object> getProtoChain(var current) { On 2016/07/21 17:44:49, Alan Knight wrote: > Don't put "var" on a method parameter, either leave it blank or put Object or > explicitly make it dynamic. Probably blank is fine in this case. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:502: protoChain.add(current); On 2016/07/21 17:44:49, Alan Knight wrote: > Naming an input parameter "current" feels odd. > > Also, perhaps as a hangover from ancient programming languages, assigning to an > input parameter makes me uncomfortable. So I'd probably give it a different name > and assign it to a separately declared current. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:509: /// Formatter for module Dart Library objects. On 2016/07/21 17:44:49, Alan Knight wrote: > What's the difference between a module dart library and a dart library? The dart library module is an object that contains several dart libraries. For example, 'third_party/dart/async/' is a library module that contains dart libraries such as async.dart, result.dart, src/async_memoizer.dart, src/cancelable_operation.dart, etc. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:516: for (current in protoChain) { On 2016/07/21 17:44:49, Alan Knight wrote: > Using a for loop variable that's declared and initialized elsewhere makes me > even more uncomfortable. It works(I think) it just feels uncommon enough that > someone reading the code could easily miss it. > > You could concatenate the collections, or make getProtoChain also return the > original object if this is its main use. Another approach would be to split out > a separate _acceptExactly(object) (or some better name meaning exactly that > object, not checking prototypes). Then this can be something like > > return _acceptExactly(object) || getProtoChain(current).any((proto) => > _acceptExactly(proto)); > > which also avoids computing the proto chain if we accept the object directly. Yeah, I switched this part to be identifiable by symbols, so now nothing I wrote references the proto chain. Now its only use is in the Object Formatter in code that I assume Jacob wrote by the TODO's. I could ask him about it to clean it up. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:519: libraryName = JSNative.getProperty(current, symbol); On 2016/07/21 17:44:49, Alan Knight wrote: > We now have a function for this, so we should use it. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:530: var libraryNameArray = libraryName.split('/'); On 2016/07/21 17:44:49, Alan Knight wrote: > Better to just call this "libraryNames", makes the expressions a little more > readable, and equally obvious that it's a collection. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:538: var properties = new LinkedHashSet<NameValuePair>(); On 2016/07/21 17:44:49, Alan Knight wrote: > These are linked because we want to preserve the JS order? I think devtools > normally displays things alphabetically, so maybe it would be better to use a > regular set and sort. Or maybe the JS order is already alphabetical? Not sure. The JS order is already alphabetical. It's nice to keep it in alphabetical JS order so that classes appear together, metadata appears first, functions are together, etc. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:540: var value = JSNative.getProperty(object, name); On 2016/07/21 17:44:49, Alan Knight wrote: > Shouldn't this, and similarly below be calling our wrapper function that catches > exceptions? Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:541: name = name.replaceAll('__', '/') + '.dart'; On 2016/07/21 17:44:49, Alan Knight wrote: > A comment for the replacement would be good. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:558: var properties = new LinkedHashSet<NameValuePair>(); On 2016/07/21 17:44:49, Alan Knight wrote: > Should this just be called "children"? Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:674: var ret = getTypeName(type); On 2016/07/21 17:44:48, Alan Knight wrote: > var typeName? Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:676: var typeNames = implements().map((type) => getTypeName(type)); On 2016/07/21 17:44:49, Alan Knight wrote: > If you're just calling a one argument function/method in a map, where the > argument is the loop variable, you can just use the function as a tear-off. > > implements().map(getTypeName); Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:677: return ret + ' implements ${typeNames.join(", ")}'; On 2016/07/21 17:44:49, Alan Knight wrote: > Really nitpicking it's slightly better to use an interpolation for the whole > business rather than a concatenation. So > > "$ret implements ..." Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:691: var ret = <NameValuePair>[]; On 2016/07/21 17:44:49, Alan Knight wrote: > Also better as "children" Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:701: if (name == 'length' || name == 'name' || name == 'prototype') continue; On 2016/07/21 17:44:49, Alan Knight wrote: > This would be better in a separate list of properties we don't show. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:717: name == 'new' || On 2016/07/21 17:44:49, Alan Knight wrote: > Also better in a separate list. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:724: var constructor = getPropertyValue(prototype, 'constructor'); On 2016/07/21 17:44:49, Alan Knight wrote: > Also, I would think, call our wrapper for this. I presume we'd want to catch any > exceptions here. > > There's probably a general issue there of ensuring robustness in our debugger if > we get an exception inside our own formatters, but that's more of a polish item > that we should note and deal with later. Done.
lgtm A few minor comments to clean up before you submit. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:173: class Library { On 2016/07/22 18:25:18, bmilligan wrote: > On 2016/07/21 02:10:32, Jacob wrote: > > do you need to wrap the libraries? I would expect the symbols added will let > you > > just tell immediately that an object is a library or a named constructor? > > The symbol for the libraries is on the library module, not the libraries > themselves. The library module is identified that way, but there's no tag on its > children libraries. Acknowledged. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:532: libraryNameArray[libraryNameArray.length - 1] = ''; On 2016/07/22 18:25:18, bmilligan wrote: > On 2016/07/21 02:10:31, Jacob wrote: > > libraryNameParts.last = '' > > '.last' cannot be used as a setter, which is why I used the indexing. The API > says .last is read-only. :/ Acknowledged. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:664: _getType(object) { On 2016/07/22 18:25:18, bmilligan wrote: > On 2016/07/21 16:18:58, Jacob wrote: > > is the return type of this method > > Type? > > rtti.dart says getReifiedType "returns the runtime representation of the type of > obj. This is different from the user-visible Type object returned by calling > 'runtimeType' on some Dart object". There are no return types defined in rtti, > so the best I can assume is that it returns an Object? Acknowledged. https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:718: name == r'$identityHash') { On 2016/07/22 18:25:18, bmilligan wrote: > On 2016/07/21 16:18:59, Jacob wrote: > > $identityHash is a little scary. Add a TODO to not hard code that one. > > Done. Is it because the name is fragile? yeah. someone might well change it to $identity or a symbol instead of naming $identityHash. https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:193: return name == null nit: dart now has a ?? operator that can simplify this sort of code https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:209: property = JSNative.getProperty(protoChain, name); nit: cleaner to just return within the try and within the catch rather than having a "property" local variable. https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:571: .split(' =>')[0] [0] --> .first https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:572: .replaceAll('(', '') can combine these two call to replaceAll
https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:718: name == r'$identityHash') { On 2016/07/22 19:17:50, Jacob wrote: > On 2016/07/22 18:25:18, bmilligan wrote: > > On 2016/07/21 16:18:59, Jacob wrote: > > > $identityHash is a little scary. Add a TODO to not hard code that one. > > > > Done. Is it because the name is fragile? > > yeah. someone might well change it to $identity or a symbol instead of naming > $identityHash. Acknowledged. https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:193: return name == null On 2016/07/22 19:17:50, Jacob wrote: > nit: dart now has a ?? operator that can simplify this sort of code So cool! https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:209: property = JSNative.getProperty(protoChain, name); On 2016/07/22 19:17:50, Jacob wrote: > nit: cleaner to just return within the try and within the catch > rather than having a > "property" > local variable. Done. https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:571: .split(' =>')[0] On 2016/07/22 19:17:50, Jacob wrote: > [0] --> .first Done. https://codereview.chromium.org/2164763005/diff/80001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:572: .replaceAll('(', '') On 2016/07/22 19:17:50, Jacob wrote: > can combine these two call to replaceAll Done.
lgtm https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:485: Object getPropertyValue(Object object, String name) { On 2016/07/22 18:25:19, bmilligan wrote: > On 2016/07/21 17:44:48, Alan Knight wrote: > > It's better to avoid "get" as a prefix (even though Java and JavaScript both > do > > it all the time), and "value" doesn't really tell us anything more. See > > go/longnamesarelong > > > > I'd probably call these property and protoChain. I could see getProperty or > > propertyNamed, since property by itself isn't very specific. > > Jacob suggested safeGetProperty. Are you referring to the object being renamed > as protoChain? safeGetProperty seems reasonable. I was referring to the getProtoChain method below, which could just be called protoChain. But I think it's disappeared now anyway. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:496: /// to avoid properties that are Dart methods. On 2016/07/22 18:25:18, bmilligan wrote: > On 2016/07/21 17:44:49, Alan Knight wrote: > > What does the comment mean about being careful to avoid properties that are > Dart > > methods? I don't see anything that looks like that. Or does it mean avoiding > the > > possibility of a Dart method named __proto__? > > This function existed previously, I just moved it to the Object Formatter class. > I'm not sure who wrote that comment or what it means. I'm suspicious it got moved to the wrong place somewhere in the past, and referred to one of those places where we had checks for particular properties to avoid. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:509: /// Formatter for module Dart Library objects. On 2016/07/22 18:25:18, bmilligan wrote: > On 2016/07/21 17:44:49, Alan Knight wrote: > > What's the difference between a module dart library and a dart library? > > The dart library module is an object that contains several dart libraries. For > example, 'third_party/dart/async/' is a library module that contains dart > libraries such as async.dart, result.dart, src/async_memoizer.dart, > src/cancelable_operation.dart, etc. Acknowledged. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:538: var properties = new LinkedHashSet<NameValuePair>(); On 2016/07/22 18:25:19, bmilligan wrote: > On 2016/07/21 17:44:49, Alan Knight wrote: > > These are linked because we want to preserve the JS order? I think devtools > > normally displays things alphabetically, so maybe it would be better to use a > > regular set and sort. Or maybe the JS order is already alphabetical? Not sure. > > The JS order is already alphabetical. It's nice to keep it in alphabetical JS > order so that classes appear together, metadata appears first, functions are > together, etc. Acknowledged.
https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:485: Object getPropertyValue(Object object, String name) { On 2016/07/22 20:28:41, Alan Knight wrote: > On 2016/07/22 18:25:19, bmilligan wrote: > > On 2016/07/21 17:44:48, Alan Knight wrote: > > > It's better to avoid "get" as a prefix (even though Java and JavaScript both > > do > > > it all the time), and "value" doesn't really tell us anything more. See > > > go/longnamesarelong > > > > > > I'd probably call these property and protoChain. I could see getProperty or > > > propertyNamed, since property by itself isn't very specific. > > > > Jacob suggested safeGetProperty. Are you referring to the object being renamed > > as protoChain? > > safeGetProperty seems reasonable. > > I was referring to the getProtoChain method below, which could just be called > protoChain. But I think it's disappeared now anyway. Done. https://codereview.chromium.org/2164763005/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:496: /// to avoid properties that are Dart methods. On 2016/07/22 20:28:41, Alan Knight wrote: > On 2016/07/22 18:25:18, bmilligan wrote: > > On 2016/07/21 17:44:49, Alan Knight wrote: > > > What does the comment mean about being careful to avoid properties that are > > Dart > > > methods? I don't see anything that looks like that. Or does it mean avoiding > > the > > > possibility of a Dart method named __proto__? > > > > This function existed previously, I just moved it to the Object Formatter > class. > > I'm not sure who wrote that comment or what it means. > > I'm suspicious it got moved to the wrong place somewhere in the past, and > referred to one of those places where we had checks for particular properties to > avoid. Done.
Description was changed from ========== Library custom formatters ========== to ========== Library custom formatters R=alanknight@google.com, jacobr@google.com, jmesserly@google.com Committed: https://github.com/dart-lang/dev_compiler/commit/98581cc03b91bcabca7473db0fbd... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) manually as 98581cc03b91bcabca7473db0fbdf5f194860c42 (presubmit successful).
Message was sent while issue was closed.
\o/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
