Closed Bug 1387406 Opened 3 years ago Closed 3 years ago

nsDocShellEditorData should store editor as HTMLEditor instead of nsIEditor

Categories

(Core :: DOM: Navigation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(5 files)

Currently, nsDocShell treats editor as nsIEditor. Therefore some internal API need to use nsIEditor instead of concrete classes. If nsDocShell starts to treat editor as HTMLEditor, a lot of places start to use concrete editor classes and that will improve the runtime performance.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 8895714 [details]
Bug 1387406 - part1: nsDocShellEditorData should store editor as HTMLEditor

https://reviewboard.mozilla.org/r/167018/#review172268

::: docshell/base/nsDocShell.cpp:13174
(Diff revision 1)
> +  if (!aEditor && !mEditorData) {
> +    return NS_OK;
> +  }
> +
> +  HTMLEditor* htmlEditor = aEditor ? aEditor->AsHTMLEditor() : nullptr;
> +  // If TextEditor comes, throw an error.

And even Thunderbird doesn't pass non-html-editor here, right?
Attachment #8895714 - Flags: review?(bugs) → review+
Comment on attachment 8895715 [details]
Bug 1387406 - part2: nsIDocShell should have GetHTMLEditor() and SetHTMLEditor()

https://reviewboard.mozilla.org/r/167020/#review172270
Attachment #8895715 - Flags: review?(bugs) → review+
Comment on attachment 8895716 [details]
Bug 1387406 - part3: nsFocusManager should treat editor as HTMLEditor

https://reviewboard.mozilla.org/r/167022/#review172272
Attachment #8895716 - Flags: review?(bugs) → review+
Comment on attachment 8895714 [details]
Bug 1387406 - part1: nsDocShellEditorData should store editor as HTMLEditor

https://reviewboard.mozilla.org/r/167018/#review172268

> And even Thunderbird doesn't pass non-html-editor here, right?

I believe so since Thunderbird always uses HTMLEditor for composing emails even if it's plaintext mode.
Comment on attachment 8895717 [details]
Bug 1387406 - part4: nsFrameLoader should treat editor as HTMLEditor

https://reviewboard.mozilla.org/r/167024/#review172276
Attachment #8895717 - Flags: review?(bugs) → review+
Comment on attachment 8895718 [details]
Bug 1387406 - part5: nsXBLWindowKeyHandler should treat editor as HTMLEditor

https://reviewboard.mozilla.org/r/167026/#review172280
Attachment #8895718 - Flags: review?(bugs) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1edef26a94c6
part1: nsDocShellEditorData should store editor as HTMLEditor r=smaug
https://hg.mozilla.org/integration/autoland/rev/17cce816b7a4
part2: nsIDocShell should have GetHTMLEditor() and SetHTMLEditor() r=smaug
https://hg.mozilla.org/integration/autoland/rev/9a4f6a269d1d
part3: nsFocusManager should treat editor as HTMLEditor r=smaug
https://hg.mozilla.org/integration/autoland/rev/0c7b5abe10cc
part4: nsFrameLoader should treat editor as HTMLEditor r=smaug
https://hg.mozilla.org/integration/autoland/rev/a5622f42070b
part5: nsXBLWindowKeyHandler should treat editor as HTMLEditor r=smaug
Ccing Jorg K.

Hi, please let us know if this change breaks Thunderbird (or SeaMonkey). As far as I check the nsIDocShell.editor in c-c, they always set HTMLEditor. So, I think that they won't be broken by those patches.
FIY: I'll be offline tomorrow due to National holiday of Japan. So, if these patch would break something, feel free to back them our by yourself.
Thanks for the heads-up.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> As far as I check the nsIDocShell.editor in c-c, they always set
> HTMLEditor. So, I think that they won't be broken by those patches.
I'm sad to say that I've lost you here. Where do we use |attribute nsIEditor editor;| of nsIDocShell.idl?
(In reply to Jorg K (GMT+2) from comment #18)
> Where do we use |attribute nsIEditor editor;| of nsIDocShell.idl?

And nsIEditor of nsIEdittingSession.setEditorOnControllers are used for associating an HTMLEditor with a document. So, it's not needed to expose them to addons.  However, starting of Gecko development, they had dream of XPCOM world...
Sorry, I still don't follow. I can see that you landed not only this bug but also bug 1319340. I have no idea whether TB is affected, since we use nsIEditor and nsIPlaintextEditor quite a bit:
https://dxr.mozilla.org/comm-central/search?q=nsIEditor&redirect=false
https://dxr.mozilla.org/comm-central/search?q=nsIPlaintextEditor&redirect=false

I still don't understand your comment #16:
> As far as I checked the nsIDocShell.editor in c-c, they always set HTMLEditor.
So that would imply that we set (or get?) 'editor' on a nsIDocShell object. Where?
Our first build after this has landed has gone through. There are no test failures and the program still seems to work ;-)
I meant that c-c doesn't set nsIDocShell.editor to TextEditor (which is base class of HTMLEditor but not abstract class) instance. If c-c did so, my patches for this bug would break something on c-c since nsDocShell::SetEditor() becomes to fail.
You need to log in before you can comment on or make changes to this bug.