Closed
Bug 1364772
Opened 4 years ago
Closed 4 years ago
Editor test failures using DebugQA extension.
Categories
(MailNews Core :: Composition, defect)
MailNews Core
Composition
Tracking
(seamonkey2.48 wontfix, seamonkey2.49esr fixed, seamonkey2.50 wontfix, seamonkey2.51 wontfix, seamonkey2.52 fixed)
RESOLVED
FIXED
Thunderbird 56.0
People
(Reporter: frg, Assigned: frg)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
8.54 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
+++ 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.
![]() |
Assignee | |
Comment 1•4 years ago
|
||
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)
![]() |
Assignee | |
Updated•4 years ago
|
Component: Composer → Composition
Product: SeaMonkey → MailNews Core
Comment 2•4 years ago
|
||
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.
![]() |
Assignee | |
Comment 3•4 years ago
|
||
>> 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•4 years ago
|
||
(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•4 years ago
|
||
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)
![]() |
Assignee | |
Comment 6•4 years ago
|
||
>> Another patch coming, right?
Yes. Sorry for the delay. Somewhat swamped and not issue #1 :)
![]() |
Assignee | |
Comment 7•4 years ago
|
||
still need to check it so not setting review yet.
Attachment #8867561 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•4 years ago
|
![]() |
Assignee | |
Comment 8•4 years ago
|
||
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)
Comment 9•4 years ago
|
||
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+
Comment 10•4 years ago
|
||
https://hg.mozilla.org/comm-central/rev/c297dfe79c058e3e8119f12e86d30167a119a5d1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
![]() |
Assignee | |
Comment 11•4 years ago
|
||
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 12•4 years ago
|
||
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+
![]() |
Assignee | |
Comment 13•4 years ago
|
||
> Let's run it through a beta cycle first.
Sure. Will you do the checkin or should I?
Comment 14•4 years ago
|
||
I will.
Comment 15•4 years ago
|
||
Beta (TB 55, SM 2.52): https://hg.mozilla.org/releases/comm-beta/rev/0ee7bd5435b5
Updated•4 years ago
|
Attachment #8878914 -
Flags: approval-comm-esr52? → approval-comm-esr52+
Comment 16•4 years ago
|
||
ESR (TB 52, SM 2.49): https://hg.mozilla.org/releases/comm-esr52/rev/9793cec8750b0600c8d27b3aa65115f75fcf708b
You need to log in
before you can comment on or make changes to this bug.
Description
•