Preserve insertion order for expanded object properties in console [devtools/devtools-frontend : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
this one requires the 3 way dance -> http://crrev.com/c/7646648 (silences errors)
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 1 comment![]( "Open in Gerrit")
Thanks for taking this on!
This does not yet cover all cases. I posted a counter-example at https://crbug.com/423838364#comment10.
I believe more counter-examples can probably be found when comparing the logic in `ObjectPropertiesSection.compareProperties` with the logic in `RemoteObjectPreviewFormatter`
Related details
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Thanks for taking this on!
This does not yet cover all cases. I posted a counter-example at https://crbug.com/423838364#comment10.
I believe more counter-examples can probably be found when comparing the logic in `ObjectPropertiesSection.compareProperties` with the logic in `RemoteObjectPreviewFormatter`
Thanks removed underscore-based reordering in compareProperties and added regression tests for underscore-prefixed keys, mixed function/data properties, and enumerable/non-enumerable buckets; do you know any other places, or do you have ideas how to find other affected places (guess even if we miss a spot; it would not end the world?)
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 2 comments![]( "Open in Gerrit")
Helmut JanuschkaThanks for taking this on!
This does not yet cover all cases. I posted a counter-example at https://crbug.com/423838364#comment10.
I believe more counter-examples can probably be found when comparing the logic in `ObjectPropertiesSection.compareProperties` with the logic in `RemoteObjectPreviewFormatter`
Thanks removed underscore-based reordering in compareProperties and added regression tests for underscore-prefixed keys, mixed function/data properties, and enumerable/non-enumerable buckets; do you know any other places, or do you have ideas how to find other affected places (guess even if we miss a spot; it would not end the world?)
I was able to create another counter-example, this time using a symbol.
As long as the sorting for the expanded view in `compareProperties` and the sorting for the collapsed view in `RemoteObjectPreviewFormatter` don't follow the same logic, there can be discrepancies.
What would happen if `compareProperties` was completely removed/not called? I see 3 call sites but I haven't investigated what each of them do exactly. Maybe @pfaffe who touched this code recently has some more context.
Philip, do you have relevant knowledge you can share?
Related details
- Helmut Januschka
- Philip Pfaffe
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 2 comments![]( "Open in Gerrit")
Helmut JanuschkaThanks for taking this on!
This does not yet cover all cases. I posted a counter-example at https://crbug.com/423838364#comment10.
I believe more counter-examples can probably be found when comparing the logic in `ObjectPropertiesSection.compareProperties` with the logic in `RemoteObjectPreviewFormatter`
Wolfgang BeyerThanks removed underscore-based reordering in compareProperties and added regression tests for underscore-prefixed keys, mixed function/data properties, and enumerable/non-enumerable buckets; do you know any other places, or do you have ideas how to find other affected places (guess even if we miss a spot; it would not end the world?)
I was able to create another counter-example, this time using a symbol.
As long as the sorting for the expanded view in `compareProperties` and the sorting for the collapsed view in `RemoteObjectPreviewFormatter` don't follow the same logic, there can be discrepancies.
What would happen if `compareProperties` was completely removed/not called? I see 3 call sites but I haven't investigated what each of them do exactly. Maybe @pfaffe who touched this code recently has some more context.
Any option we choose here will make some people unhappy I'm afraid. Especially if we change this to address a small-ish UX inconsistency. Should we maybe make this configurable instead?
Except in the wasm case, I don't think I'd ever worry about the insertion order. I certainly rely on the alphabetical order though, in order to find specific properties I'm looking for. How about this approach:
- Stop sorting wasm properties by default.
- Keep sorting JS properties by default.
- Have a context menu entry to toggle.
Bug: 423838364Related details
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
Helmut JanuschkaThanks for taking this on!
This does not yet cover all cases. I posted a counter-example at https://crbug.com/423838364#comment10.
I believe more counter-examples can probably be found when comparing the logic in `ObjectPropertiesSection.compareProperties` with the logic in `RemoteObjectPreviewFormatter`
Wolfgang BeyerThanks removed underscore-based reordering in compareProperties and added regression tests for underscore-prefixed keys, mixed function/data properties, and enumerable/non-enumerable buckets; do you know any other places, or do you have ideas how to find other affected places (guess even if we miss a spot; it would not end the world?)
Philip PfaffeI was able to create another counter-example, this time using a symbol.
As long as the sorting for the expanded view in `compareProperties` and the sorting for the collapsed view in `RemoteObjectPreviewFormatter` don't follow the same logic, there can be discrepancies.
What would happen if `compareProperties` was completely removed/not called? I see 3 call sites but I haven't investigated what each of them do exactly. Maybe @pfaffe who touched this code recently has some more context.
Any option we choose here will make some people unhappy I'm afraid. Especially if we change this to address a small-ish UX inconsistency. Should we maybe make this configurable instead?
Except in the wasm case, I don't think I'd ever worry about the insertion order. I certainly rely on the alphabetical order though, in order to find specific properties I'm looking for. How about this approach:
- Stop sorting wasm properties by default.
- Keep sorting JS properties by default.
- Have a context menu entry to toggle.
thanks philip, see newest PS it has the context menu entry and you suggested applied
Does this also fix crbug.com/40833016?
- Philip Pfaffe
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 4 comments![]( "Open in Gerrit")
const sortPropertiesAlphabetically = obj.subtype !== 'wasmvalue' && obj.subtype !== 'webassemblymemory';We should check this inside of ObjectPropertiesSection so that it applies to all objects. Also, I'm not sure we should force the sorting of webassemblymemory objects, WDYT?
#sortPropertiesAlphabetically: boolean;I think we should not store this in a dedicated property and instead write changes through to the options. And that means that we need to copy the options instead of storing the object.
Alternatively, we need to always pass fresh objects to the ObjectTreeNodeBase constructor.
if (sortPropertiesAlphabetically) {What about the underscores?
['useCapture', 'false'],If we're still sorting by default, what are these results changing?
Related details
- Helmut Januschka
- Wolfgang Beyer
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
const sortPropertiesAlphabetically = obj.subtype !== 'wasmvalue' && obj.subtype !== 'webassemblymemory';We should check this inside of ObjectPropertiesSection so that it applies to all objects. Also, I'm not sure we should force the sorting of webassemblymemory objects, WDYT?
moved to ObjectPropertiesSection. and well i think it is not worth the extra code to force any special case on wasm, reverted.
#sortPropertiesAlphabetically: boolean;I think we should not store this in a dedicated property and instead write changes through to the options. And that means that we need to copy the options instead of storing the object.
Alternatively, we need to always pass fresh objects to the ObjectTreeNodeBase constructor.
removed the dedicated sort field and now write the value through `options.sortPropertiesAlphabetically`
if (sortPropertiesAlphabetically) {Helmut JanuschkaWhat about the underscores?
good catch, restored the underscore.
['useCapture', 'false'],If we're still sorting by default, what are these results changing?
- Philip Pfaffe
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |