Closed Bug 959445 Opened 7 years ago Closed 7 years ago

add lldb summaries for nsIAtoms, nsTextNodes and nsTextFragments showing their text content

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: heycam, Assigned: heycam)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #8359563 - Flags: review?(ehsan)
Attached patch patch v2 (obsolete) — Splinter Review
Make the string formatting function more lenient about the types it accepts, so we can print nsAStrings etc. later more easily.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8359580 - Flags: review?(ehsan)
Attachment #8359563 - Attachment is obsolete: true
Attachment #8359563 - Flags: review?(ehsan)
Attached patch patch v2.1Splinter Review
Sorry one more tweak.
Attachment #8359580 - Attachment is obsolete: true
Attachment #8359580 - Flags: review?(ehsan)
Attachment #8359584 - Flags: review?(ehsan)
Blocks: 959452
Comment on attachment 8359584 [details] [diff] [review]
patch v2.1

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

This is pretty neat, I must say!

::: python/lldbutils/lldbutils/content.py
@@ +9,5 @@
> +        field = "m2b"
> +    else:
> +        field = "m1b"
> +    ptr = content_union.GetChildMemberWithName(field)
> +    return utils.format_string(ptr, min(length, 100))

Hmm, I don't think we should clamp the string length like this, please remove the clamping code.
Attachment #8359584 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/bb7af8904cac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8359584 [details] [diff] [review]
patch v2.1

>+    elif c == 0x07:
>+        return "\\a"
>+    elif c == 0x08:
>+        return "\\b"
>+    elif c == 0x0c:
>+        return "\\f"
>+    elif c == 0x0a:
>+        return "\\n"
>+    elif c == 0x0d:
>+        return "\\r"
>+    elif c == 0x09:
>+        return "\\t"
>+    elif c == 0x0b:
>+        return "\\v"
I would have thought numerical order would make more sense than alphabetical order of special character.

>+    elif c < 0x20 or c >= 0x80 and c <= 0xff:
>+        return "\\x%02x" % c
IIRC 0x7f is also unprintable.

>+    elif c >= 0x0100:
If you swap the conditions then you can write
elif c >= 0x0100:
    ...
elif c < 0x20 or c >= 0x7f:
    ...
else:
    etc.

>+    i = 0
>+    terminated = False
>+    while i < length:
>+        c = lldb_value.CreateValueFromAddress("x", ptr + i * size, char_type).GetValueAsUnsigned(0) & mask
>+        if c == 0:
>+            terminated = True
Strings are always the exact provided length; embedded nulls are completely legal, in fact you will only hit this condition in such a case. It's also unsafe to read the length character to check whether the string is terminated; you can look at the flags if you really want to be sure, but it's certainly wrong to add an ellipsis in any event.
Neil, can you open a separate bug for these things?
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.