improve test_editing_identity in mozmill/composition/test-newmsg-compose-identity.js

RESOLVED FIXED in Thunderbird 55.0

Status

Thunderbird
Testing Infrastructure
--
trivial
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 55.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.67 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 months ago
I think the test test_editing_identity() in mozmill/composition/test-newmsg-compose-identity.js could use some small tweaks:

1. compWin.e("msgIdentityPopup").value is usually written as compWin.eid("msgIdentityPopup")

2. The pref mail.compose.warned_about_customize_from we use internally in Thunderbird isn't actually defined anywhere. I think that's bad as fetching nonexistent prefs throws in JS. We got away with it because it is only queried via getPref() which checks if the pref exists.
(Assignee)

Comment 1

6 months ago
Created attachment 8870560 [details] [diff] [review]
patch

Should be checked after bug 1366517 is done.
Attachment #8870560 - Flags: review?(jorgk)
(In reply to :aceman from comment #1)
> Should be checked after bug 1366517 is done.
Meaning? You check, I check or did you mean "check in". I agree that not defining the preference is bad, but do we need the other hunk? BTW, this should go to all-thunderbird.js since it's not a mailnews.js function:
https://dxr.mozilla.org/comm-central/search?q=mail.compose.warned_about_customize_from&redirect=false
(Assignee)

Comment 3

6 months ago
(In reply to Jorg K (GMT+2) from comment #2)
> (In reply to :aceman from comment #1)
> > Should be checked after bug 1366517 is done.
> Meaning? You check, I check

We check: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ed3e87ed9625eaaa3772f47aa273bbed1cff4b63 :)

> or did you mean "check in". I agree that not
> defining the preference is bad, but do we need the other hunk?

Why not? .e().value seems strange and not used elsewhere.

> BTW, this
> should go to all-thunderbird.js since it's not a mailnews.js function:
> https://dxr.mozilla.org/comm-central/search?q=mail.compose.
> warned_about_customize_from&redirect=false

I saw that, but somehow I thought Neil would implement the same feature in Seamonkey too. See Bug 87987 comment 189. I'm not sure if it materialized or not.
Comment on attachment 8870560 [details] [diff] [review]
patch

The pref is not used in suite/, so it should go to all-thunderbird.js:
https://dxr.mozilla.org/comm-central/search?q=mail.compose.warned_about_customize_from&redirect=false

They have their own MsgComposeCommands.js and can move the pref if they ever port the editable header to SM.
Attachment #8870560 - Flags: review?(jorgk) → review+
(Assignee)

Comment 5

6 months ago
Created attachment 8871026 [details] [diff] [review]
patch v2

OK, thanks.
Attachment #8870560 - Attachment is obsolete: true
Attachment #8871026 - Flags: review+
(Assignee)

Updated

6 months ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ff32dbcafcd4e82afb672b30fb0a5ee817083291
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 55.0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.