Closed Bug 1060051 (nsIEditor-builtin) Opened 10 years ago Closed 7 years ago

Make nsIEditor and related interfaces builtin classes

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: rkent, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1060051 - Make editor related interfaces builtin classes if it's possible
59 bytes, text/x-review-board-request
smaug
: review+
ehsan.akhgari
: review+
Details
In Bug 1053048 nsIEditor was made builtinclass, which caused issues in the Thunderbird-stdlib maintained by Jonathan Protzenko and used, for example, in the Conversations Thunderbird addon. The Editor team kindly reverted that in bug 1059705 but clearly wants a better solution that allows them to make nsIEditor builtinclass. Determine and implement that solution.
Alias: nsIEditor-builtin
Blocks: cc-backlog
bug 1059171 is a good interim fix for lack of a better refactoring of nsMsgCompose
(In reply to Jonathan Protzenko [:protz] from comment #1) > bug 1059171 is a good interim fix for lack of a better refactoring of > nsMsgCompose Would that still mean that you need to implement the nsIEditor you want to set as the value of that attribute in JS though?
No, this would alleviate for me the need to implement an editor in JS.
Sounds great then!
I am not sure this bug is a right place. I wrote a test with an implementation of nsIEditor in JS before. https://bug653342.bugzilla.mozilla.org/attachment.cgi?id=766581 How can be written those mocks with builtinclass nsIEditor in JS?
Looks like that nobody in current c-c implements nsIEditor at testing: https://dxr.mozilla.org/comm-central/search?q=nsIEditor+path%3Atest&redirect=false So, can we make nsIEditor builtinclass again? # I cannot find the test mentioned in comment 5.
Oh, but I found at least one add-ons which is still available in AMO...
Still implementing nsIEditor as JS objects are: https://addons.mozilla.org/en-US/thunderbird/addon/compose-for-thunderbird/ https://addons.mozilla.org/en-US/thunderbird/addon/rss-tab/ https://addons.mozilla.org/en-US/thunderbird/addon/emic/ https://addons.mozilla.org/en-US/thunderbird/addon/rethread/ https://addons.mozilla.org/en-US/thunderbird/addon/gmail-conversation-view/ They use same library, the fine name is "send.js". (Search "FakeEditor" in the addon tree.) Some of them already use new API introduced by bug 1059171. However, some of them have not been updated yet. So, I think that we can make nsIEditor builtin class now. Then, we can get rid of a lot of virtual calls in editor code with using pointers to concrete classes. What do you think? (Should we contact the addon developers first?)
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)
And at least in the addon tree, there are no addons which implements nsIPlaintextEditor nor nsIHTMLEditor interface.
Additionally, nsIHTMLAbsPosEditor, nsIHTMLInlineTableEditor, nsITableEditor, nsIEditorMailSupport, nsIEditorStyleSheets, nsIEditorBlobListener and nsIHTMLObjectResizer can be builtin class.
Yeah, I think we should make nsIEditor builtinclass, but informing addons authors early on would be good.
Flags: needinfo?(bugs)
Can I ask a very silly question? What does it mean for TB that nsIEditor becomes a "builtin" class? In fact, what is a "builtin" class? We seem to use nsIEditor in a few places: https://dxr.mozilla.org/comm-central/search?q=nsIEditor&redirect=false Sorry about the ignorance, but it can only become better ;-)
It means the implementation would be C++ only. JS could still use it, but not re-implement.
I also think we should make these interfaces builtinclass. Informing the affected add-ons would be nice, but we should be clear that the notices are informational only (that is to say, we should make this change even at the risk of add-on breakage, and ask the add-on authors to work around it.)
Flags: needinfo?(ehsan)
Okay, I emailed to :protz and send message via FB to Inbox Cleaner of expired Emails's author.
Looks like that all the addons are not active since none of them is available on Tb52. So, we can just make nsIEditor and related interfaces builtin classes here. Changing the summary and component of this bug.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Composition → Editor
Product: MailNews Core → Core
Summary: Resolve obstacles that require Conversations to wrap nsIEditor → Make nsIEditor and related interfaces builtin classes
As far as I checked, bluegriffon doesn't implement these interfaces too.
Oh, and also we can make nsIEdittingSession a buildin class.
Comment on attachment 8894160 [details] Bug 1060051 - Make editor related interfaces builtin classes if it's possible https://reviewboard.mozilla.org/r/165230/#review170580 ::: commit-message-933a0:3 (Diff revision 1) > +Bug 1060051 - Make editor related interfaces builtin classes if it's possible r?ehsan, smaug > + > +If we make nsIEditor a buildin class, that means that its instance can be only TextEditor or HTMLEditor. Then, users of nsIEditor can use concrete classes such as EditorBase, TextEditor or HTMLEditor instead. Then, the users can reduce unnecessary QI and a lot of virtual calls if we'll create non-virtual methods. s/buildin/builtin/ ::: commit-message-933a0:5 (Diff revision 1) > +Bug 1060051 - Make editor related interfaces builtin classes if it's possible r?ehsan, smaug > + > +If we make nsIEditor a buildin class, that means that its instance can be only TextEditor or HTMLEditor. Then, users of nsIEditor can use concrete classes such as EditorBase, TextEditor or HTMLEditor instead. Then, the users can reduce unnecessary QI and a lot of virtual calls if we'll create non-virtual methods. > + > +So, let's make editor related interfaces buildin classes. builtin
Comment on attachment 8894160 [details] Bug 1060051 - Make editor related interfaces builtin classes if it's possible https://reviewboard.mozilla.org/r/165230/#review170582
Attachment #8894160 - Flags: review?(bugs) → review+
Comment on attachment 8894160 [details] Bug 1060051 - Make editor related interfaces builtin classes if it's possible https://reviewboard.mozilla.org/r/165230/#review171452
Attachment #8894160 - Flags: review?(ehsan) → review+
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/cddc1b05b876 Make editor related interfaces builtin classes if it's possible r=Ehsan,smaug
https://hg.mozilla.org/mozilla-central/rev/cddc1b05b876
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: