Closed Bug 1476897 Opened 6 years ago Closed 6 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla63

Tracking Flags:

Tracking

Status

firefox63

---

fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

Bug 1476897 - Make HTMLEditor define overriding methods of nsIEditorMailSupport instead of using NS_DECL_NSIEDITORMAILSUPPORT 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request		Details
Bug 1476897 - part 1: Implement nsIEditorMailSupport in HTMLEditor since it's never used with TextEditor instance 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1476897 - part 2: Make TextEditor::GetEmbeddedObjects() just return NS_ERROR_NOT_IMPLEMENTED since it's never used 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1476897 - part 3: Make TextEditor::InsertAsCitedQuotation() return NS_ERROR_NOT_IMPLEMENTED since nobody uses this method with TextEditor instances 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1476897 - part 4: Make TextEditor::InsertTextWithQuotations() returns NS_ERROR_NOT_IMPLEMENTED since it's never used 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1476897 - part 5: Move implementation of TextEditor::InsertAsQuotation() to new method 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1476897 - part 6: Move implementation of both TextEditor::PasteAsQuotation() and HTMLEditor::PasteAsQuotation() to new virtual methods 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1476897 - part 7: Drop nsIEditorMailSupport interface from TextEditor 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28) 59 bytes, text/x-review-board-request	m_kato : review+	Details

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Description

•

6 years ago

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.

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 1

•

6 years ago

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

Comment hidden (mozreview-request)

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/

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 3

•

6 years ago

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.

Attachment #8993570 - Flags: review?(m_kato)

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Updated

•

6 years ago

Attachment #8993570 - Attachment is obsolete: true

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Updated

•

6 years ago

Summary: HTMLEditor should define only necessary methods of nsIEditorMailSupport → HTMLEditor should inherit nsIEditorMailSupport directly and TextEditor shouldn't inherit it

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 4

•

6 years ago

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

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 5

•

6 years ago

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

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 6

•

6 years ago

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

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 7

•

6 years ago

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

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 8

•

6 years ago

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

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 9

•

6 years ago

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

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 10

•

6 years ago

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Code Review Bot [:reviewbot]

Comment 18

•

6 years ago

mozreview-review

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);
    ^

Code Review Bot [:reviewbot]

Comment 19

•

6 years ago

mozreview-review

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Updated

•

6 years ago

Blocks: 1467800, 1467801

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Comment 23

•

6 years ago

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

Makoto Kato [:m_kato]

Comment 24

•

6 years ago

mozreview-review

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

Attachment #8994136 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 25

•

6 years ago

mozreview-review

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.

Attachment #8994137 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 26

•

6 years ago

mozreview-review

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

Makoto Kato [:m_kato]

Comment 27

•

6 years ago

(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.

Makoto Kato [:m_kato]

Comment 28

•

6 years ago

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

Makoto Kato [:m_kato]

Comment 29

•

6 years ago

mozreview-review

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

Attachment #8994138 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 30

•

6 years ago

mozreview-review

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

Attachment #8994139 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 31

•

6 years ago

mozreview-review

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

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

Attachment #8994140 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 32

•

6 years ago

mozreview-review

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

Attachment #8994141 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 33

•

6 years ago

mozreview-review

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

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

Attachment #8994142 - Flags: review?(m_kato) → review+

Pulsebot

Comment 34

•

6 years ago

Pushed by masayuki@d-toybox.com:
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

Bogdan Tara[:bogdan_tara | bogdant]

Comment 35

•

6 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox63: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla63

Jorg K (CEST = GMT+2)

Updated

•

6 years ago

Depends on: 1478268

Makoto Kato [:m_kato]

Comment 36

•

6 years ago

(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

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)

Assignee

Updated

•

4 years ago

Blocks: redesign-editor-scriptable-API

You need to log in before you can comment on or make changes to this bug.