Closed Bug 988826 Opened 6 years ago Closed 5 years ago

Use LongStringActor for exceptionMessage in the evaluateJS response packet

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: msucan, Assigned: akshendra521994, Mentored)

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

No description provided.
Mentor: mihai.sucan
Whiteboard: [mentor=msucan][lang=js] → [lang=js]
Flags: needinfo?(mihai.sucan)
I'm not sure what the needinfo was for, but the purpose of this bug is to use a LongStringActor in the console backend for transmitting the exceptionMessage as it can get quite long. You just need to use _createStringGrip for this and you can crib from other uses of this function in the same file:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webconsole.js#788
Mentor: mihai.sucan → past
Flags: needinfo?(mihai.sucan)
Attached patch 988826_LongStringActor.patch (obsolete) — Splinter Review
Attachment #8515499 - Flags: review?(past)
Comment on attachment 8515499 [details] [diff] [review]
988826_LongStringActor.patch

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

Nice job! Now we need to write a test to make sure this won't regress in the future. Take a look at test_throw.html and see how it already verifies that the |exception| property is a long string. 

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/test/test_throw.html#55

You only need to add a similar check for the exceptionMessage property.
Attachment #8515499 - Flags: review?(past) → feedback+
Attached patch 988826_LongStringActor.patch (obsolete) — Splinter Review
Please assign that to me!!
Attachment #8515499 - Attachment is obsolete: true
Attachment #8523021 - Flags: review?(past)
Comment on attachment 8523021 [details] [diff] [review]
988826_LongStringActor.patch

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

Nice, but it can be simplified even more.

::: toolkit/devtools/webconsole/test/test_throw.html
@@ +61,5 @@
>  
> +  tests.push(function() {
> +    aState.client.evaluateJS("throw '" + longString + "';", function(aResponse) {
> +      is(aResponse.exceptionMessage.initial, shortedString,
> +        "exceptionMessage.initial for throw longString"

You don't need a new evaluateJS request, just to do the extra check in the previous one.
Attachment #8523021 - Flags: review?(past) → feedback+
Assignee: nobody → akshendra521994
Status: NEW → ASSIGNED
Attachment #8523021 - Attachment is obsolete: true
Attachment #8523090 - Flags: review?(past)
Comment on attachment 8523090 [details] [diff] [review]
988826_LongStringActor.patch

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

Perfect!
Attachment #8523090 - Flags: review?(past) → review+
https://hg.mozilla.org/integration/fx-team/rev/ad98a5bdb2e1
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ad98a5bdb2e1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.