Closed Bug 1387406 Opened 7 years ago Closed 7 years ago

nsDocShellEditorData should store editor as HTMLEditor instead of nsIEditor

Categories

(Core :: DOM: Navigation, enhancement)

Product:
Core
Component:
DOM: Navigation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(5 files)

Bug 1387406 - part1: nsDocShellEditorData should store editor as HTMLEditor
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1387406 - part2: nsIDocShell should have GetHTMLEditor() and SetHTMLEditor()
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1387406 - part3: nsFocusManager should treat editor as HTMLEditor
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1387406 - part4: nsFrameLoader should treat editor as HTMLEditor
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1387406 - part5: nsXBLWindowKeyHandler should treat editor as HTMLEditor
59 bytes, text/x-review-board-request
smaug
: review+
Details 
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.

Attachment

General

Creator:
Created:
Updated:
Size: