Closed Bug 1692480 Opened 9 months ago Closed 2 months ago

get rid of setElementValue(), setAttributeToChildren() and other unhelpful helper functions

Categories

(Calendar :: General, defect)

defect

Tracking

(thunderbird_esr78 wontfix, thunderbird_esr91 unaffected, thunderbird87 wontfix)

RESOLVED FIXED
88 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird_esr91 --- unaffected
thunderbird87 --- wontfix

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(6 files, 1 obsolete file)

https://searchfox.org/comm-central/rev/af1bf36d0b1e04e5af4bbbdca43661bc4cbd8c90/calendar/base/content/calendar-ui-utils.js#32

setElementValue is such a weird thing to have, and with all the attribute/property replacements going on (in m-c) with this function it's kind of hard to script or guess what it does. We should get rid of it.

Get rid of setElementValue.
Notably for commands we need to set the attributes for broadcasting to work like it's supposed to.

Attachment #9205338 - Flags: review?(geoff)

Remvove setAttributeToChildren which doesn't do what it says. Needs to land at the same time, I could squash it for landing.

Attachment #9205339 - Flags: review?(geoff)

I fixed the one test failure, forgot to remove the comment.

Status: NEW → ASSIGNED
Target Milestone: --- → 88 Branch
Attachment #9205338 - Flags: review?(geoff) → review+
Comment on attachment 9205339 [details] [diff] [review]
bug1692480_replace_setAttributeToChildren.patch

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

::: calendar/base/content/calendar-multiday-base-view.js
@@ -1072,5 @@
> -        const box = this.querySelector(selector);
> -        setAttributeToChildren(box, "orient", orient);
> -      }
> -
> -      setAttributeToChildren(this.labeldaybox, "orient", otherOrient);

Where did this go? It probably appeared to do nothing because you made setAttributeToChildren set properties instead of attributes. But setting the attributes is definitely needed.
Attachment #9205339 - Flags: review?(geoff) → review-

Setting the orient property - which this did in reality - did not do anything after bug 1604154. I looked if any rotations are broken but couldn't find anything.

There are CSS selectors depending on the attribute, if nothing else.

Yes, but it never set the attribute, it set the property. The method is (was) wrongly named.

You made that change, in the first patch. Before that it was setting attributes.

Ah yes I should learn to read. Thanks for catching!

Attachment #9205339 - Attachment is obsolete: true
Attachment #9205638 - Flags: review?(geoff)
Comment on attachment 9205638 [details] [diff] [review]
bug1692480_replace_setAttributeToChildren.patch

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

::: calendar/base/content/calendar-multiday-base-view.js
@@ +1071,3 @@
>        }
>  
> +      for (let child of this.labeldaybox) {

this.labeldaybox.children
Attachment #9205638 - Flags: review?(geoff) → review+
Keywords: leave-open
Summary: get rid of setElementValue() → get rid of setElementValue(), setAttributeToChildren()
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/48b6fcecc977
get rid of setElementValue(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/075c10bb1254
remove setAttributeToChildren. r=darktrojan
Attachment #9205717 - Flags: review?(geoff)
Attachment #9205718 - Flags: review?(geoff)
Attachment #9205719 - Flags: review?(geoff)
Summary: get rid of setElementValue(), setAttributeToChildren() → get rid of setElementValue(), setAttributeToChildren() and other unhelpful helper functions
Attachment #9205717 - Flags: review?(geoff) → review+
Attachment #9205718 - Flags: review?(geoff) → review+
Attachment #9205719 - Flags: review?(geoff) → review+
Attachment #9205720 - Flags: review?(geoff) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/44a1b3d9c276
remove removeChildren(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/4fdadfa83c05
remove menuListSelectItem(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/1c1c25ad250e
remove getElementValue. r=darktrojan
https://hg.mozilla.org/comm-central/rev/9f18ca80de5e
remove setBooleanAttribute(). r=darktrojan
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/9fdb69cfae80
follow-up - Fix remove/unsubscribe calendar menu items. rs=me
Regressions: 1695516

mkmelin, does the "leave-open" keyword still apply here or can this bug report be marked resolved?

Flags: needinfo?(mkmelin+mozilla)

Let's close. That file still has a few dubious functions but the worst were removed.

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Flags: needinfo?(mkmelin+mozilla)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.