1476897 - HTMLEditor should inherit nsIEditorMailSupport directly and TextEditor shouldn't inherit it

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

HTMLEditor now defines overrides of nsIEditorMailSupport methods with  NS_DECL_NSIEDITORMAILSUPPORT. So, if we don't need to override some of them like Rewrap(), it just increases the binary size. So, HTMLEditor should override each of them only when it's necessary.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80a38c74f2e7c3c479e4711b1c7120ad75bc272c

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - Make HTMLEditor define overriding methods of nsIEditorMailSupport instead of using NS_DECL_NSIEDITORMAILSUPPORT

HTMLEditor defines overrides of nsIEditorMailSupport methods with
NS_DECL_NSIEDITORMAILSUPPORT. So, if we don't need to override some of them
like Rewrap(), it just increases the binary size. So, HTMLEditor should
override each of them only when it's necessary.

Review commit: https://reviewboard.mozilla.org/r/258284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258284/

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8993570 [details]
Bug 1476897 - Make HTMLEditor define overriding methods of nsIEditorMailSupport instead of using NS_DECL_NSIEDITORMAILSUPPORT

Sorry for the bug spam. I change my mind. We should make HTMLEditor inherits nsIEditorMailSupport directly instead of TextEditor.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23cb755ce0158885a75a869c5c204268a41390a0

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ecf7e950844c4541b644a12696e87721b1af5ea

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6287d0caf60906ef5bb2b15141b415d4b96a942c

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=507df2d060f00b7b67268241b94b61371bf2da96

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=255a3da1092dd82e6ef658f0209f2d9c810dd2d8

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bcba98f3a77b8ba40e9d88e9b2b1fd58b6b6a75

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28dd9ea5c8f97bf63cb8fb44bd9e523f87ca995d

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - part 1: Implement nsIEditorMailSupport in HTMLEditor since it's never used with TextEditor instance

Review commit: https://reviewboard.mozilla.org/r/258746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258746/

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - part 2: Make TextEditor::GetEmbeddedObjects() just return NS_ERROR_NOT_IMPLEMENTED since it's never used

TextEditor::GetEmbeddedObjects() is never called unless
nsIEditorMailSupport::GetEmbeddedObjects() is used with TextEditor instance.
However, nobody uses nsIEditorMailSupport interface with TextEditor instance.

Review commit: https://reviewboard.mozilla.org/r/258748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258748/

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - part 3: Make TextEditor::InsertAsCitedQuotation() return NS_ERROR_NOT_IMPLEMENTED since nobody uses this method with TextEditor instances

Review commit: https://reviewboard.mozilla.org/r/258750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258750/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - part 4: Make TextEditor::InsertTextWithQuotations() returns NS_ERROR_NOT_IMPLEMENTED since it's never used

InsertTextWithQuotations() is called only by HTMLEditor::Rewrap().  So,
HTMLEditor::InsertTextWithQuotations() is always called.  Therefore, we can
make it just return NS_ERROR_NOT_IMPLEMENTED.

Review commit: https://reviewboard.mozilla.org/r/258752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258752/

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - part 5: Move implementation of TextEditor::InsertAsQuotation() to new method

TextEditor::InsertAsQuotation() is not used as a method of nsIEditorMailSupport,
however, this is used internally.  So, we need to keep this method's function
as a non-virtual method but we can make the method just return
NS_ERROR_NOT_IMPLEMENTED.

Review commit: https://reviewboard.mozilla.org/r/258754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258754/

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - part 6: Move implementation of both TextEditor::PasteAsQuotation() and HTMLEditor::PasteAsQuotation() to new virtual methods

Neither TextEditor::PasteAsQuotation() nor HTMLEditor::PasteAsQuotation()
is used via nsIEditorMailSupport.  So, perhaps, we can remove this method
from the interface.  But for now, we should move each implementation to
new virtual methods and make only HTMLEditor::PasteAsQuotation() calls
new method since nobody uses nsIEditorMailSupport with TextEditor instances.

Review commit: https://reviewboard.mozilla.org/r/258756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258756/

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1476897 - part 7: Drop nsIEditorMailSupport interface from TextEditor

Nobody uses nsIEditorMailSupport interface with TextEditor instances.
Therefore, stopping nsIEditorMailSupport interface support of TextEditor
saves memory of TextEditor which may be created a lot even in a document.

Review commit: https://reviewboard.mozilla.org/r/258758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258758/

Comment 18

[Code Review Bot [:reviewbot]]

Comment on attachment 8994140 [details]
Bug 1476897 - part 5: Move implementation of TextEditor::InsertAsQuotation() to new method

https://reviewboard.mozilla.org/r/258754/#review265640


Code analysis found 3 defects in this patch:
 - 3 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: editor/libeditor/HTMLEditorDataTransfer.cpp:1840
(Diff revision 1)
>        newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>          NS_LITERAL_STRING("white-space: pre-wrap;"), true);
>      }
>  
>      // and set the selection inside it:
> -    selection->Collapse(newNode, 0);
> +    rv = selection->Collapse(newNode, 0);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    rv = selection->Collapse(newNode, 0);
    ^

::: editor/libeditor/HTMLEditorDataTransfer.cpp:1840
(Diff revision 1)
>        newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>          NS_LITERAL_STRING("white-space: pre-wrap;"), true);
>      }
>  
>      // and set the selection inside it:
> -    selection->Collapse(newNode, 0);
> +    rv = selection->Collapse(newNode, 0);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    rv = selection->Collapse(newNode, 0);
    ^

::: editor/libeditor/HTMLEditorDataTransfer.cpp:1840
(Diff revision 1)
>        newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>          NS_LITERAL_STRING("white-space: pre-wrap;"), true);
>      }
>  
>      // and set the selection inside it:
> -    selection->Collapse(newNode, 0);
> +    rv = selection->Collapse(newNode, 0);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    rv = selection->Collapse(newNode, 0);
    ^

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8994141 [details]
Bug 1476897 - part 6: Move implementation of both TextEditor::PasteAsQuotation() and HTMLEditor::PasteAsQuotation() to new virtual methods

https://reviewboard.mozilla.org/r/258756/#review265642


Code analysis found 3 defects in this patch:
 - 3 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: editor/libeditor/HTMLEditorDataTransfer.cpp:1853
(Diff revision 1)
>        newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>          NS_LITERAL_STRING("white-space: pre-wrap;"), true);
>      }
>  
>      // and set the selection inside it:
>      rv = selection->Collapse(newNode, 0);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    rv = selection->Collapse(newNode, 0);
    ^

::: editor/libeditor/HTMLEditorDataTransfer.cpp:1853
(Diff revision 1)
>        newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>          NS_LITERAL_STRING("white-space: pre-wrap;"), true);
>      }
>  
>      // and set the selection inside it:
>      rv = selection->Collapse(newNode, 0);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    rv = selection->Collapse(newNode, 0);
    ^

::: editor/libeditor/HTMLEditorDataTransfer.cpp:1853
(Diff revision 1)
>        newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>          NS_LITERAL_STRING("white-space: pre-wrap;"), true);
>      }
>  
>      // and set the selection inside it:
>      rv = selection->Collapse(newNode, 0);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    rv = selection->Collapse(newNode, 0);
    ^

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8994140 [details]
Bug 1476897 - part 5: Move implementation of TextEditor::InsertAsQuotation() to new method

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258754/diff/1-2/

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8994141 [details]
Bug 1476897 - part 6: Move implementation of both TextEditor::PasteAsQuotation() and HTMLEditor::PasteAsQuotation() to new virtual methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258756/diff/1-2/

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8994142 [details]
Bug 1476897 - part 7: Drop nsIEditorMailSupport interface from TextEditor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258758/diff/1-2/

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f894d23da6e8ef049603af6b96ccc401914d85b

Comment 24

[Makoto Kato [:m_kato]]

Comment on attachment 8994136 [details]
Bug 1476897 - part 1: Implement nsIEditorMailSupport in HTMLEditor since it's never used with TextEditor instance

https://reviewboard.mozilla.org/r/258746/#review265966

Comment 25

[Makoto Kato [:m_kato]]

Comment on attachment 8994137 [details]
Bug 1476897 - part 2: Make TextEditor::GetEmbeddedObjects() just return NS_ERROR_NOT_IMPLEMENTED since it's never used

https://reviewboard.mozilla.org/r/258748/#review265968

No one uses getEmbeddedObjects without debugQAEditorOverlay.js?  I cannot find any uses from m-c, c-c and bluegriffon.

Comment 26

[Makoto Kato [:m_kato]]

Comment on attachment 8994137 [details]
Bug 1476897 - part 2: Make TextEditor::GetEmbeddedObjects() just return NS_ERROR_NOT_IMPLEMENTED since it's never used

https://reviewboard.mozilla.org/r/258748/#review265970

Comment 27

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #25)
> Comment on attachment 8994137 [details]
> Bug 1476897 - part 2: Make TextEditor::GetEmbeddedObjects() just return
> NS_ERROR_NOT_IMPLEMENTED since it's never used
> 
> https://reviewboard.mozilla.org/r/258748/#review265968
> 
> No one uses getEmbeddedObjects without debugQAEditorOverlay.js?  I cannot
> find any uses from m-c, c-c and bluegriffon.

Ah, nsMsgCompose.cpp uses it.  Sorry.

Comment 28

[Makoto Kato [:m_kato]]

(but we should move GetEmbeddedObjects to c-c since it doesn't depend on editor code.)

Comment 29

[Makoto Kato [:m_kato]]

Comment on attachment 8994138 [details]
Bug 1476897 - part 3: Make TextEditor::InsertAsCitedQuotation() return NS_ERROR_NOT_IMPLEMENTED since nobody uses this method with TextEditor instances

https://reviewboard.mozilla.org/r/258750/#review265972

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 8994139 [details]
Bug 1476897 - part 4: Make TextEditor::InsertTextWithQuotations() returns NS_ERROR_NOT_IMPLEMENTED since it's never used

https://reviewboard.mozilla.org/r/258752/#review265978

Comment 31

[Makoto Kato [:m_kato]]

Comment on attachment 8994140 [details]
Bug 1476897 - part 5: Move implementation of TextEditor::InsertAsQuotation() to new method

https://reviewboard.mozilla.org/r/258754/#review265986

Comment 32

[Makoto Kato [:m_kato]]

Comment on attachment 8994141 [details]
Bug 1476897 - part 6: Move implementation of both TextEditor::PasteAsQuotation() and HTMLEditor::PasteAsQuotation() to new virtual methods

https://reviewboard.mozilla.org/r/258756/#review266002

Comment 33

[Makoto Kato [:m_kato]]

Comment on attachment 8994142 [details]
Bug 1476897 - part 7: Drop nsIEditorMailSupport interface from TextEditor

https://reviewboard.mozilla.org/r/258758/#review266004

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07d193decac0
part 1: Implement nsIEditorMailSupport in HTMLEditor since it's never used with TextEditor instance r=m_kato
https://hg.mozilla.org/integration/autoland/rev/39ff01e6d729
part 2: Make TextEditor::GetEmbeddedObjects() just return NS_ERROR_NOT_IMPLEMENTED since it's never used r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e8d209654ba4
part 3: Make TextEditor::InsertAsCitedQuotation() return NS_ERROR_NOT_IMPLEMENTED since nobody uses this method with TextEditor instances r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e5c9e728940a
part 4: Make TextEditor::InsertTextWithQuotations() returns NS_ERROR_NOT_IMPLEMENTED since it's never used r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2356492fb27c
part 5: Move implementation of TextEditor::InsertAsQuotation() to new method r=m_kato
https://hg.mozilla.org/integration/autoland/rev/62e18ecb29ea
part 6: Move implementation of both TextEditor::PasteAsQuotation() and HTMLEditor::PasteAsQuotation() to new virtual methods r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2c8b3ee07751
part 7: Drop nsIEditorMailSupport interface from TextEditor r=m_kato

Comment 35

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/07d193decac0
https://hg.mozilla.org/mozilla-central/rev/39ff01e6d729
https://hg.mozilla.org/mozilla-central/rev/e8d209654ba4
https://hg.mozilla.org/mozilla-central/rev/e5c9e728940a
https://hg.mozilla.org/mozilla-central/rev/2356492fb27c
https://hg.mozilla.org/mozilla-central/rev/62e18ecb29ea
https://hg.mozilla.org/mozilla-central/rev/2c8b3ee07751

Comment 36

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #28)
> (but we should move GetEmbeddedObjects to c-c since it doesn't depend on
> editor code.)

I file as bug 1478546