1364772 - Editor test failures using DebugQA extension.

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Frank-Rainer Grahl (:frg)]

+++ 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.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1364772-EditorConstVar.patch

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().

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[Frank-Rainer Grahl (:frg)]

>> 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.

Comment 4

[Jorg K (CEST = GMT+2)]

(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 5

[Jorg K (CEST = GMT+2)]

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.

Comment 6

[Frank-Rainer Grahl (:frg)]

>> Another patch coming, right?

Yes. Sorry for the delay. Somewhat swamped and not issue #1 :)

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1364772-EditorConstVar-V2.patch

still need to check it so not setting review yet.

Comment 8

[Frank-Rainer Grahl (:frg)]

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/

Comment 9

[Jorg K (CEST = GMT+2)]

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.

Comment 10

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c297dfe79c058e3e8119f12e86d30167a119a5d1

Comment 11

[Frank-Rainer Grahl (:frg)]

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.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8878914 [details] [diff] [review]
1364772-EditorConstVar-V2.patch

Let's run it through a beta cycle first.

Comment 13

[Frank-Rainer Grahl (:frg)]

> Let's run it through a beta cycle first.

Sure. Will you do the checkin or should I?

Comment 14

[Jorg K (CEST = GMT+2)]

I will.

Comment 15

[Jorg K (CEST = GMT+2)]

Beta (TB 55, SM 2.52):
https://hg.mozilla.org/releases/comm-beta/rev/0ee7bd5435b5

Comment 16

[Jorg K (CEST = GMT+2)]

ESR (TB 52, SM 2.49):
https://hg.mozilla.org/releases/comm-esr52/rev/9793cec8750b0600c8d27b3aa65115f75fcf708b