Closed Bug 1364772 Opened 3 years ago Closed 3 years ago

Editor test failures using DebugQA extension.

Categories

(MailNews Core :: Composition, defect)

defect
Not set

Tracking

(seamonkey2.48 wontfix, seamonkey2.49esr fixed, seamonkey2.50 wontfix, seamonkey2.51 wontfix, seamonkey2.52 fixed)

RESOLVED FIXED
Thunderbird 56.0
Tracking Status
seamonkey2.48 --- wontfix
seamonkey2.49esr --- fixed
seamonkey2.50 --- wontfix
seamonkey2.51 --- wontfix
seamonkey2.52 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1290187 +++

While in the process of uploading a new version of the DebugQA extension I ran into some problems during final testing with Debug->Composer (with test page).

In the resulting composer windows Debug->Plaintext Editor is supposed to open a new editor window. That works but from there on it goes downhill because of the scope changes in bug 589199. First you get a gMac not available error. Fix this and you run into other errors e.g. the toolbar is not fuctional. Various top level const in editor\ui\composer\content\editorUtilities.js need to be changed to var like in bug 1209777 and bug 1290187.
Attached patch 1364772-EditorConstVar.patch (obsolete) — Splinter Review
Jorg, I just saw that editor.js is also used in TB so you better review this one. I will move it over to mailnews in a moment.

I changed isMac detection and removed GetOS().
Attachment #8867561 - Flags: review?(jorgk)
Component: Composer → Composition
Product: SeaMonkey → MailNews Core
Comment on attachment 8867561 [details] [diff] [review]
1364772-EditorConstVar.patch

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

This looks OK in general, but why did you change all the const to var? It's not necessary, also see bug 1209777 comment #4.
Please revert this, the only ones you need to change (maybe) are:
var nsIWebNavigation = Components.interfaces.nsIWebNavigation; and 
var nsIFilePicker = Components.interfaces.nsIFilePicker;
All the constants that start with a "k" are really just constants.
>> but why did you change all the const to var? It's not necessary,

Unfortunately wrong. Any top level const used somewhere else is affected. 
This const caused the first problem: "const gMac = "Mac";" and there was at least one other besides nsIWebNavigation and nsIFilePicker. So instead of checked which ones were used in other files I just changed all of them. I will check them again and see if I can pick only the affected ones.
(In reply to Frank-Rainer Grahl from comment #3)
> Any top level const used somewhere else is affected. 
OK, I can see gMac being used in two files:
https://dxr.mozilla.org/comm-central/search?q=gMac&redirect=false
Equally kOutputWrap, I haven't checked more. So why don't the constants move to where they are used? Can we do that?
Comment on attachment 8867561 [details] [diff] [review]
1364772-EditorConstVar.patch

(In reply to Frank-Rainer Grahl (:frg) from comment #3)
> I will check them again and see if I can pick only the affected ones.
Another patch coming, right? Clearing review for now.
Attachment #8867561 - Flags: review?(jorgk)
>> Another patch coming, right?

Yes. Sorry for the delay. Somewhat swamped and not issue #1 :)
still need to check it so not setting review yet.
Attachment #8867561 - Attachment is obsolete: true
Comment on attachment 8878914 [details] [diff] [review]
1364772-EditorConstVar-V2.patch

Globally used constants declared as var or removed if unused. I now no longer see errors in SeaMonkeys debugQA extension. Hope I understood this correctly:

https://blog.mozilla.org/addons/2015/10/14/breaking-changes-let-const-firefox-nightly-44/
Attachment #8878914 - Flags: review?(jorgk)
Blocks: 1374094
Comment on attachment 8878914 [details] [diff] [review]
1364772-EditorConstVar-V2.patch

I think this is better than v1: Less const->var and more removal of unused stuff.
Attachment #8878914 - Flags: review?(jorgk) → review+
https://hg.mozilla.org/comm-central/rev/c297dfe79c058e3e8119f12e86d30167a119a5d1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
Comment on attachment 8878914 [details] [diff] [review]
1364772-EditorConstVar-V2.patch

[Approval Request Comment]
Regression caused by (bug #): 589199
User impact if declined: Some tests in debugQA extension will not work. Might affect other extensions too.
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): I think none.
Attachment #8878914 - Flags: approval-comm-esr52?
Comment on attachment 8878914 [details] [diff] [review]
1364772-EditorConstVar-V2.patch

Let's run it through a beta cycle first.
Attachment #8878914 - Flags: approval-comm-beta+
> Let's run it through a beta cycle first.

Sure. Will you do the checkin or should I?
Attachment #8878914 - Flags: approval-comm-esr52? → approval-comm-esr52+
See Also: → 1417819
You need to log in before you can comment on or make changes to this bug.