807924 - Trying to inspect objects in Scratchpad sometimes results in type errors

Status: RESOLVED WORKSFORME

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

STR:

Write this line in a Scratchpad
>> Object.create(null)
..and Inspect

Error: TypeError: can't convert null to primitive type
Source File: chrome://browser/content/scratchpad.js
Line: 483

..or Display

Error: TypeError: can't convert Object to string
Source File: chrome://browser/content/scratchpad.js
Line: 560

Write this instead:
>> Object.create(null).constructor
or
>> Object.create(null).prototype
and Inspect

Error: Error: First argument must have an objectActor or an object property!
Source File: resource://gre/modules/PropertyPanel.jsm
Line: 114

Comment 1

[Brandon Benvie [:benvie]]

I'll fix this in the process of converting the Scratchpad to use VariablesView.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: WIP1

First crack at this. With bug 808369 landing, fixing this just required fixing `Scratchpad.display`. This patch:

* adds a new localization to 'scratchpad.properties' for `stringConversionFailed`
* wraps the conversion to string in `Scratchpad.writeAsComment` in a try-catch
* displays the "Conversion to string failed." error in the catch
* adds a test to confirm this functions as expected

Comment 4

[Brandon Benvie [:benvie]]

Attachment: WIP2

And of course I forgot to add the new test file.

Comment 5

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=fc5c3b6f98f5

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 743371 [details] [diff] [review]
WIP2

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +550,5 @@
> +    try {
> +      str = aValue + "";
> +    }
> +    catch (ex) {
> +      str = this.strings.GetStringFromName("stringConversionFailed");

should we return ex.message along with this as well? Though I guess ex.message is likely null. I feel like the exception should return *something*. Maybe preface the str with "Exception: " as we do for other error messages.

::: browser/locales/en-US/chrome/browser/devtools/scratchpad.properties
@@ +89,5 @@
>  fileNoLongerExists.notification=This file no longer exists.
> +
> +# LOCALIZATION NOTE (stringConversionFailed): Happens when a value cannot be
> +# converted to a string for inspection.
> +stringConversionFailed=Cannot convert value to string.
\ No newline at end of file

I think this should be pretty obviously an error.

Comment 7

[Rob Campbell [:rc] (:robcee)]

are you still working on this Brandon?

Comment 8

[Brandon Benvie [:benvie]]

I've been debating how to proceed on this, since I realized the work on bug 825039 kind of subsumes this. I suppose it might be best just to get this finished up and landed and then revisit.

Comment 9

[Brandon Benvie [:benvie]]

Ok yeah, I'm rolling this into bug 825039.