Summary: | Web Inspector: deleting an array property causes it to appear as `undefined × 1` in the console | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mathias Bynens <mathias> | ||||||||||||||||
Component: | Web Inspector | Assignee: | sam <dsam2912> | ||||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||||
Severity: | Normal | CC: | ap, burg, dglazkov, dsam2912, graouts, joepeck, jonowells, mathias, mattbaker, ndkrempel, nvasilyev, paulirish, pfeldman, timothy, webkit-bug-importer, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
URL: | data:text/html,<script>var%20array%20%3D%20%5B0%2C%201%2C%202%2C%203%5D%3B%20delete%20array%5B0%5D%3B%20console.log(array)%3B<%2Fscript> | ||||||||||||||||||
Attachments: |
|
Description
Mathias Bynens
2012-03-28 08:13:00 PDT
Another example: `Array(2)` logs `[undefined × 2]`, but `[undefined, undefined]` logs `[undefined, undefined]`. Created attachment 138780 [details]
Patch
Comment on attachment 138780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138780&action=review > Source/WebCore/inspector/front-end/ConsoleMessage.js:341 > + if (!element || element.textContent === "undefined") This change does not address the bug. You should instead modify appendUndefined to not render "x 1" in case it is actually "1". The code was not handling the undefined as the first value. This also needs a test (see LayoutTests/inspector/console/console-format.html). (In reply to comment #3) > (From update of attachment 138780 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138780&action=review > > > Source/WebCore/inspector/front-end/ConsoleMessage.js:341 > > + if (!element || element.textContent === "undefined") > > This change does not address the bug. You should instead modify appendUndefined to not render "x 1" in case it is actually "1". The code was not handling the undefined as the first value. This also needs a test (see LayoutTests/inspector/console/console-format.html). Thanks for review Pavel. I have though of the same thing first. But then though of other cases as well while cross checking. Following are some of the cases and their o/p with checking for render "x 1" in case it is actually "1" in appendUndefined. 1).var array = [0, 1, 2, 3]; delete array[0]; array; [undefined, 1, 2, 3] 2).var array = [undefined, undefined]; array; [undefined, undefined] --- I guess this should be handled as undefined*2. It would happen as the undefined values gets missed in while going through array elements and its corresponding properties in the loop just below appendUndefined(). Since there is no delete here, all properties value are in order and there will be no cumulation for undefined. 3).var array = [undefined, undefined, 1, 2, 3]; delete array[2]; delete array[3] ;array; [undefined, undefined, undefined × 2, 3] --- Same here. But I guess check for "1" should be in place as "undefined" is better than "undefined x 1". Kindly suggest. Here is why delete and simple undefined work differently: > var a = [undefined, undefined, 3]; Object.keys(a); ["0", "1", "2"] > var a = [1, 2, 3]; delete a[0]; delete a[1]; Object.keys(a); ["2"] Anyways, in addition to the fix in appendUndefined, you should then change the loop in the _printArray to ignore values that have type undefined: for (var i = 0; i < properties.length; ++i) { var property = properties[i]; var name = property.name; if (!isNaN(name) >>>>>>>>>>&& property.value.type !== "undefined"<<<<<<<<<<<) elements[name] = this._formatAsArrayEntry(property.value); } Created attachment 138815 [details]
Patch
(In reply to comment #5) > Here is why delete and simple undefined work differently: > > > var a = [undefined, undefined, 3]; Object.keys(a); > ["0", "1", "2"] > > > var a = [1, 2, 3]; delete a[0]; delete a[1]; Object.keys(a); > ["2"] > > > Anyways, in addition to the fix in appendUndefined, you should then change the loop in the _printArray to ignore values that have type undefined: > > for (var i = 0; i < properties.length; ++i) { > var property = properties[i]; > var name = property.name; > if (!isNaN(name) >>>>>>>>>>&& property.value.type !== "undefined"<<<<<<<<<<<) > elements[name] = this._formatAsArrayEntry(property.value); > } ohh, thanks Pavel!, have uploaded the patch. Comment on attachment 138815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138815&action=review One small nit and it is good to go. You don't need to change localizedStrings here. (resetting r?). > Source/WebCore/inspector/front-end/ConsoleMessage.js:337 > + span.textContent = WebInspector.UIString("undefined"); You don't need to localize "undefined" - it is a JavaScript term. The reason it was localized previously was "x %d" fragment. Created attachment 138819 [details]
Patch
(In reply to comment #8) > (From update of attachment 138815 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138815&action=review > > One small nit and it is good to go. You don't need to change localizedStrings here. (resetting r?). > > > Source/WebCore/inspector/front-end/ConsoleMessage.js:337 > > + span.textContent = WebInspector.UIString("undefined"); > > You don't need to localize "undefined" - it is a JavaScript term. The reason it was localized previously was "x %d" fragment. okk, have removed it from localizedStrings. Is it ok ? Comment on attachment 138819 [details] Patch Rejecting attachment 138819 [details] from commit-queue. New failing tests: inspector/console/console-format.html Full output: http://queues.webkit.org/results/12520608 Created attachment 138839 [details]
Archive of layout-test-results from ec2-cq-03
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 138819 [details] Patch Attachment 138819 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12528690 New failing tests: inspector/console/console-format.html Created attachment 138872 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 151639 [details]
Patch
(In reply to comment #15) > Created an attachment (id=151639) [details] > Patch Hi Pavel, I have uploaded the patch again (content remains the same) as I guess it could not be committed earlier due bot error. Comment on attachment 151639 [details] Patch Attachment 151639 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13180381 New failing tests: inspector/console/console-format.html Created attachment 151654 [details]
Archive of layout-test-results from gce-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 151639 [details]
Patch
Please make sure you rebaseline Layouttests/platform/chromium/inspector/console-fromat-expexted.txt prior to landing.
This doesn't seem like a bug to me. I see the '[undefined × 1]' output as a feature, which lets you distinguish between sparse arrays and arrays filled with the value 'undefined'. If you're going to remove the '× 1' part, then perhaps you should also: (a) Replace 'undefined' with 'missing', so that: Array(1) => [missing] Array(2) => [missing × 2] (ideally with 'missing' in italics, but not essential.) (b) Also group genuine 'undefined' values, so that: [undefined] => [undefined] [undefined, undefined] => [undefined × 2] I see (a) as more important than (b), and also an easy change. Should we distinguish missing and undefined? Close or keep? Joe's rewrite of array previews has addressed the ambiguity between undefined elements and holes. Closing. |