Closed Bug 997219 Opened 6 years ago Closed 3 years ago

Object Inspector doesn't show the value of properties with whitespace name

Categories

(DevTools :: Object Inspector, defect)

defect
Not set

Tracking

(firefox51 verified)

VERIFIED FIXED
Firefox 51
Tracking Status
firefox51 --- verified

People

(Reporter: obrufau, Assigned: Oriol)

References

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140415030203

Steps to reproduce:

1. Open Web Console
2. Run `({' ': 123})`. Any combination of spaces, tabs and newlines can be used.
3. Click the returned object


Actual results:

Object Inspector shows the whitespace property (collapsed), but doesn't show its value.


Expected results:

It should show its value.
Component: Untriaged → Developer Tools: Object Inspector
OS: Windows XP → All
Hardware: x86 → All
See Also: → 996691
I haven't looked at this bug yet, but I'm pretty sure bug 881480 will fix it if we end up doing that first. There must be a simpler fix though.
Depends on: 881480
I also noticed this issue.

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=ee5ca214e87c

Probably caused by bug 808370.

But before that, the debugger was already affected.
Assignee: nobody → oriol-bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
The problem is that VariablesView trims the property names.
https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/devtools/client/shared/widgets/VariablesView.jsm#1260

And at the end, if the string is the empty one, it returns before displaying the value.
https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/devtools/client/shared/widgets/VariablesView.jsm#2451

So the solution seems trimming property name when displaying it, but leaving it untrimmed in _nameString.

Not sure who should review, so choosing :past because he commented.
Attachment #8779568 - Flags: review?(past)
Comment on attachment 8779568 [details] [diff] [review]
object-inspector-whitespace-property.patch

Review of attachment 8779568 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting to Brian.
Attachment #8779568 - Flags: review?(past) → review?(bgrinstead)
Comment on attachment 8779568 [details] [diff] [review]
object-inspector-whitespace-property.patch

I'm unavailable for a couple of days - forwarding to Nick
Attachment #8779568 - Flags: review?(bgrinstead) → review?(nfitzgerald)
Comment on attachment 8779568 [details] [diff] [review]
object-inspector-whitespace-property.patch

Review of attachment 8779568 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but we should have a tiny test so that this doesn't regress in the future.
Attachment #8779568 - Flags: review?(nfitzgerald)
Adding test
Attachment #8779568 - Attachment is obsolete: true
Attachment #8779844 - Flags: review?(nfitzgerald)
Comment on attachment 8779844 [details] [diff] [review]
object-inspector-whitespace-property-v2.patch

Review of attachment 8779844 [details] [diff] [review]:
-----------------------------------------------------------------

Great test! Thanks a lot!

r=me with the comments below fixed up.

::: devtools/client/debugger/test/mochitest/browser_dbg_variables-view-08.js
@@ +29,5 @@
> +
> +  let obj;
> +  [...scope].forEach(function([name, value]) {
> +    if(name === "obj") obj = value;
> +  });

Nit: for/of loop is more idiomatic than a forEach here. Also, devtools JS code always uses {} even if the condition is just a single statement.

  let obj;
  for (let [name, value] of scope) {
    if (name === "obj") {
      obj = value;
    }
  }

@@ +42,5 @@
> +  let count = values.length;
> +
> +  for (let [property, value] of obj) {
> +    let index = values.indexOf(property);
> +    if(index >= 0) {

Nit: space between "if" and "(" here and throughout

@@ +54,5 @@
> +    }
> +  }
> +  is(count, 0, "There are " + count + " missing properties");
> +
> +  debugger;

Was this left here from debugging? This shouldn't be here.
Attachment #8779844 - Flags: review?(nfitzgerald) → review+
Attached patch This is a mistake. Ignore it. (obsolete) — Splinter Review
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #8)
> Was this left here from debugging? This shouldn't be here.

This was left here when I copied from browser_dbg_variables-view-07.js, which was left there when I copied from browser_dbg_variables-view-06.js. It seemed strange, but I thought it was necessary for the tests.

I removed it from these too while I was at it.
Attachment #8779844 - Attachment is obsolete: true
Attachment #8779917 - Flags: review?(nfitzgerald)
Sorry, I attached the wrong patch.
Attachment #8779917 - Attachment is obsolete: true
Attachment #8779917 - Flags: review?(nfitzgerald)
Attachment #8779918 - Flags: review?(nfitzgerald)
(In reply to Oriol from comment #9)
> Created attachment 8779917 [details] [diff] [review]
> revoked-proxy-crash-v3.patch
> 
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #8)
> > Was this left here from debugging? This shouldn't be here.
> 
> This was left here when I copied from browser_dbg_variables-view-07.js,
> which was left there when I copied from browser_dbg_variables-view-06.js. It
> seemed strange, but I thought it was necessary for the tests.

The `debugger` statement in the test case is certainly necessary; the `debugger` statement(s?) in test orchestration files shouldn't be there.

> I removed it from these too while I was at it.

Awesome, thank you very much.
Comment on attachment 8779918 [details] [diff] [review]
object-inspector-whitespace-property-v3.patch

Review of attachment 8779918 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Attachment #8779918 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
Attachment #8779917 - Attachment description: revoked-proxy-crash-v3.patch → This is a mistake. Ignore it.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/eb1e3117f53c
When inspecting an object, do not trim property names except when displaying them. r=fitzgen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb1e3117f53c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this Bug on Nightly 31.0a1 (2014-04-16)  on Windows 10, 64 Bit!

The bug's fix is now verified on latest  Nightly 51.0a1

Nightly  51.0a1:
Build ID	   : 20160817030202
User Agent 	   : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

 [bugday-20160817]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.