richlistitem label property inconsistencies

RESOLVED FIXED in mozilla1.9.3a1

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: arno, Assigned: arno)

Tracking

Trunk
mozilla1.9.3a1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 344052 [details]
testcase: label should be "one two" for both richlistitem

Hi,
richlistitem label is supposed to concatenate label texts contained by item. but it does not work fine. See testcase
(Assignee)

Comment 1

10 years ago
Created attachment 344065 [details] [diff] [review]
patch v1
Assignee: nobody → arenevier
Attachment #344065 - Flags: review?(neil)

Comment 2

10 years ago
I think that the current binding takes care of all the common cases, and you can easily provide your own getter if your binding needs it. Enn, thoughts?

Comment 3

10 years ago
That was my thought too when I saw this bug. But item1 in the testcase seems like it should return a value, no?
(Assignee)

Comment 4

10 years ago
Created attachment 344688 [details] [diff] [review]
do not count childs of anonymous content

> I think that the current binding takes care of all the common cases, and you
> can easily provide your own getter if your binding needs it.

Good point. But as neil deakin pointed out, current behaviour is still messy in common case: label are only counted if they are descendant of first node. So, I rewrote my patch by only managing common case.
Attachment #344065 - Attachment is obsolete: true
Attachment #344688 - Flags: review?(neil)
Attachment #344065 - Flags: review?(neil)

Comment 5

10 years ago
Comment on attachment 344688 [details] [diff] [review]
do not count childs of anonymous content

Punting the review to Enn because I don't have tests enabled.

>+
>+            var labels = this.getElementsByTagNameNS(XULNS, "label");
>+            for (var count = 0; count < labels.length; count++) {
>+                labelText += labels[count].value + " ";
>+            }
Why the extra blank line and {}s?

>+            if (labelText.length) // removes trailing space
>+                labelText = labelText.slice(0, -1)
You sliced off the trailing ; by mistake!
Attachment #344688 - Flags: review?(neil) → review?(enndeakin)
(Assignee)

Comment 6

10 years ago
Created attachment 344742 [details] [diff] [review]
fixes typo

sorry for typos
Attachment #344688 - Attachment is obsolete: true
Attachment #344742 - Flags: review?(enndeakin)
Attachment #344688 - Flags: review?(enndeakin)
(Assignee)

Comment 7

10 years ago
Created attachment 344743 [details] [diff] [review]
also removes extra blank line
Attachment #344052 - Attachment is obsolete: true
Attachment #344743 - Flags: review?(enndeakin)
(Assignee)

Updated

10 years ago
Attachment #344742 - Flags: review?(enndeakin)
(Assignee)

Updated

10 years ago
Attachment #344742 - Attachment is obsolete: true

Comment 8

10 years ago
Comment on attachment 344743 [details] [diff] [review]
also removes extra blank line

>+            if (labelText.length) // removes trailing space
>+              labelText = labelText.slice(0, -1);

Is there a reason to do this?

If so, there's a recently added (but not yet documented) trim() function that trims spaces which you can use instead.

Updated

10 years ago
Attachment #344743 - Flags: review?(enndeakin) → review+

Comment 9

10 years ago
Comment on attachment 344743 [details] [diff] [review]
also removes extra blank line

>+            if (labelText.length) // removes trailing space
>+              labelText = labelText.slice(0, -1);

OK, I see that this is just removing the space that was added at the end. It may be easier to just not add the space in that case, but no big deal.
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 10

10 years ago
(In reply to comment #9)
> It may be easier to just not add the space in that case, but no big deal.

return Array.map(labels, function(aLabel) aLabel.value).join(" ");
Whiteboard: [c-n: wait for comment 10, or not ?]
Version: unspecified → Trunk
(Assignee)

Comment 11

10 years ago
Is a new patch necessary ?

Comment 12

10 years ago
It's an easy win in simpler code, so yes please. The only thing I'm not quite sure of is whether we can suppose that JavaScript 1.8 will always be available for this binding, so you might want to use the more conservative

function(aLabel) { return aLabel.value; }

instead of the new shorthand.
(Assignee)

Comment 13

10 years ago
Created attachment 346346 [details] [diff] [review]
patch v3 (uses Array.map)
Attachment #344743 - Attachment is obsolete: true
Attachment #346346 - Flags: review?(neil)

Comment 14

10 years ago
Comment on attachment 346346 [details] [diff] [review]
patch v3 (uses Array.map)

>+            var labels = this.getElementsByTagNameNS(XULNS, "label");
>+            return Array.map(labels, 
>+                            function(aLabel) { return aLabel.value }
>+                            ).join(" ");
That's some weird wrapping. I'd wrap it like this:
            return Array.map(labels, function(aLabel) { return aLabel.value })
                        .join(" ");
OK, so I lied, I'd actually wrap it like this ;-)
            return Array.map(this.getElementsByTagNameNS(XULNS, "label"),
                             function(aLabel) { return aLabel.value })
                        .join(" ");
Attachment #346346 - Flags: review?(neil) → review+
(Assignee)

Comment 15

10 years ago
Created attachment 346548 [details] [diff] [review]
better indentation

same patch with indenting as proposed by   neil
Attachment #346346 - Attachment is obsolete: true
Attachment #346548 - Flags: review+

Comment 16

10 years ago
Not quite. Please align dots (before map and join) and arguments (this... and function...).
(Assignee)

Updated

10 years ago
Attachment #346548 - Flags: review+
(Assignee)

Comment 17

10 years ago
Created attachment 347401 [details] [diff] [review]
better indentation (again)
Attachment #346548 - Attachment is obsolete: true
Attachment #347401 - Flags: review?

Updated

10 years ago
Keywords: checkin-needed

Updated

9 years ago
Attachment #347401 - Flags: review? → review?(neil)

Updated

9 years ago
Attachment #347401 - Flags: review?(neil) → review+
http://hg.mozilla.org/mozilla-central/rev/bb94e11d66a3
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: wait for comment 10, or not ?]
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.