Closed Bug 956586 Opened 10 years ago Closed 10 years ago

tooltip modernization - FillInHTMLTooltip is deprecated

Categories

(Thunderbird :: Mail Window Front End, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch proposed fix (obsolete) — Splinter Review
The functionality in FillInHTMLTooltip is now in a binding (bug 480356), so we should drop that code. That makes e.g. HTML5 form validation messages work.

Thunderbird had tests for the tooltips - added in bug 561854. The xlink one would now fail. I don't think it's necessarily the correct behavior either, so I've dropped that part of the test.

protz: do you want to review this, as you wrote the tests originally in bug 561854
Attachment #8355900 - Flags: review?(jonathan.protzenko)
Comment on attachment 8355900 [details] [diff] [review]
proposed fix

Just reviewed this, thanks for the ping. This looks like an excellent change (I'm always happy to see more Thunderbird-specific code go away!).

I'm r-'ing because I think we should completely get rid of the FillInHTMLTooltip function: a quick MXR search <http://mxr.mozilla.org/comm-central/search?string=FillInHTMLTooltip&filter=^[^\0]*%24&tree=comm-central> shows that you replaced all the occurrences, if I'm not mistaken, so we might as well remove it completely.
Attachment #8355900 - Flags: review?(jonathan.protzenko)
Attachment #8355900 - Flags: review-
Attachment #8355900 - Flags: feedback+
For the record, bug 956579 removes FillInHTMLTooltip in convbrowser.xml.
Hm, should I remove the test too? The test works, but what it actually tests is now toolkit functionality. At least the xul in html part is already tested in toolkit now.
I think it's ok to leave the test; it's reasonably small, and we've had a history of things breaking at random because of changes in mozilla-central. Let's keep us covered, since having this broken could go a long way unnoticed.
Attached patch proposed fix, v2Splinter Review
Removing FillInHTMLTooltip completely
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8357699 - Flags: review?(jonathan.protzenko)
Attachment #8355900 - Attachment is obsolete: true
Comment on attachment 8357699 [details] [diff] [review]
proposed fix, v2

I haven't run the tests myself, but I trust you that this is not breaking the build. The patch looks just right, thanks!
Attachment #8357699 - Flags: review?(jonathan.protzenko) → review+
https://hg.mozilla.org/comm-central/rev/f444666b101c -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.