nsIHTMLEditor has several scriptable methods (addDefaultProperty(), removeDefaultProperty(), etc.) that have nsIAtom parameters. We're in the process of deCOMtaminating nsIAtom (bug 1392883) so these methods need to be changed.

froydnj, I'm asking for feedback because we talked about this on IRC and you might be interested to see what the "aggressive" eliminate-nsIAtom.idl-altogether option looks like.

Comment on attachment 8900977 [details] [diff] [review] Remove nsIAtom/nsIAtomService usage from script in editor/ >diff --git a/editor/nsIHTMLEditor.idl b/editor/nsIHTMLEditor.idl >--- a/editor/nsIHTMLEditor.idl >+++ b/editor/nsIHTMLEditor.idl >@@ -45,34 +44,44 @@ interface nsIHTMLEditor : nsISupports > * @param aProperty the property to set by default > * @param aAttribute the attribute of the property, if applicable. > * May be null. > * Example: aProperty="font", aAttribute="color" > * @param aValue if aAttribute is not null, the value of the attribute. > * Example: aProperty="font", aAttribute="color", > * aValue="0x00FFFF" > */ >- void addDefaultProperty(in nsIAtom aProperty, >+ void addDefaultProperty(in AString aProperty, > in AString aAttribute, > in AString aValue); >+%{C++ >+ virtual nsresult AddDefaultPropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue) = 0; >+%} I don't think that we need to keep declaring the method for internal (native code) use here. This should be declared and implemented only in mozilla::HTMLEditor. Then, you don't need to use the post-fix "Cpp". You can overload the scriptable method with different argument (nsAString vs. nsIAtom*). I guess that you used this approach for editor/libeditor/TextEditorTest.cpp. However, it also can use the scriptable method instead. I think that using scriptable method is enough safe to test it. > > /** > * RemoveDefaultProperty() unregisters a default style property with the editor > * > * @param aProperty the property to remove from defaults > * @param aAttribute the attribute of the property, if applicable. > * May be null. > * Example: aProperty="font", aAttribute="color" > * @param aValue if aAttribute is not null, the value of the attribute. > * Example: aProperty="font", aAttribute="color", > * aValue="0x00FFFF" > */ >- void removeDefaultProperty(in nsIAtom aProperty, >+ void removeDefaultProperty(in AString aProperty, > in AString aAttribute, > in AString aValue); >+%{C++ >+ virtual nsresult RemoveDefaultPropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue) = 0; >+%} Same here. >@@ -82,19 +91,24 @@ interface nsIHTMLEditor : nsISupports > * @param aAttribute the attribute of the property, if applicable. > * May be null. > * Example: aProperty="font", aAttribute="color" > * @param aValue if aAttribute is not null, the value of the attribute. > * May be null. > * Example: aProperty="font", aAttribute="color", > * aValue="0x00FFFF" > */ >- void setInlineProperty(in nsIAtom aProperty, >+ void setInlineProperty(in AString aProperty, > in AString aAttribute, > in AString aValue); >+%{C++ >+ virtual nsresult SetInlinePropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue) = 0; >+%} Same. >@@ -106,29 +120,46 @@ interface nsIHTMLEditor : nsISupports > * aValue="0x00FFFF" > * @param aFirst [OUT] PR_TRUE if the first text node in the > * selection has the property > * @param aAny [OUT] PR_TRUE if any of the text nodes in the > * selection have the property > * @param aAll [OUT] PR_TRUE if all of the text nodes in the > * selection have the property > */ >- void getInlineProperty(in nsIAtom aProperty, >- in AString aAttribute, >- in AString aValue, >+ void getInlineProperty(in AString aProperty, >+ in AString aAttribute, >+ in AString aValue, > out boolean aFirst, > out boolean aAny, > out boolean aAll); >+%{C++ >+ virtual nsresult GetInlinePropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue, >+ bool* aFirst, >+ bool* aAny, >+ bool* aAll) = 0; >+%} Same. >- AString getInlinePropertyWithAttrValue(in nsIAtom aProperty, >- in AString aAttribute, >- in AString aValue, >- out boolean aFirst, >- out boolean aAny, >- out boolean aAll); >+ AString getInlinePropertyWithAttrValue(in AString aProperty, >+ in AString aAttribute, >+ in AString aValue, >+ out boolean aFirst, >+ out boolean aAny, >+ out boolean aAll); >+%{C++ >+ virtual nsresult GetInlinePropertyWithAttrValueCpp(nsIAtom* aProperty, >+ const nsAString& aAttr, >+ const nsAString& aValue, >+ bool* aFirst, >+ bool* aAny, >+ bool* aAll, >+ nsAString& outValue) = 0; >+%} Same. >@@ -144,17 +175,21 @@ interface nsIHTMLEditor : nsISupports > * @param aAttribute the attribute of the property, if applicable. > * May be null. > * Example: aProperty=nsIEditorProptery::font, > * aAttribute="color" > * nsIEditProperty::allAttributes is special. > * It indicates that all content-based text properties > * are to be removed from the selection. > */ >- void removeInlineProperty(in nsIAtom aProperty, in AString aAttribute); >+ void removeInlineProperty(in AString aProperty, in AString aAttribute); >+%{C++ >+ virtual nsresult RemoveInlinePropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute) = 0; >+%} Same. >diff --git a/editor/composer/nsComposerCommands.cpp b/editor/composer/nsComposerCommands.cpp Then, I guess you don't need to modify this file. >diff --git a/editor/libeditor/HTMLEditor.cpp b/editor/libeditor/HTMLEditor.cpp >--- a/editor/libeditor/HTMLEditor.cpp >+++ b/editor/libeditor/HTMLEditor.cpp >@@ -2716,17 +2716,17 @@ HTMLEditor::InsertLinkAroundSelection(ns > value.Truncate(); > > rv = attribute->GetName(name); > NS_ENSURE_SUCCESS(rv, rv); > > rv = attribute->GetValue(value); > NS_ENSURE_SUCCESS(rv, rv); > >- rv = SetInlineProperty(nsGkAtoms::a, name, value); >+ rv = SetInlinePropertyCpp(nsGkAtoms::a, name, value); And here. >diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h >--- a/editor/libeditor/HTMLEditor.h >+++ b/editor/libeditor/HTMLEditor.h >@@ -136,16 +136,40 @@ public: > > // nsStubMutationObserver overrides > NS_DECL_NSIMUTATIONOBSERVER_CONTENTAPPENDED > NS_DECL_NSIMUTATIONOBSERVER_CONTENTINSERTED > NS_DECL_NSIMUTATIONOBSERVER_CONTENTREMOVED > > // nsIHTMLEditor methods > NS_DECL_NSIHTMLEDITOR >+ nsresult AddDefaultPropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue) override; >+ nsresult RemoveDefaultPropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue) override; >+ nsresult SetInlinePropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue) override; >+ nsresult GetInlinePropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute, >+ const nsAString& aValue, >+ bool* aFirst, >+ bool* aAny, >+ bool* aAll) override; >+ nsresult GetInlinePropertyWithAttrValueCpp(nsIAtom* aProperty, >+ const nsAString& aAttr, >+ const nsAString& aValue, >+ bool* aFirst, >+ bool* aAny, >+ bool* aAll, >+ nsAString& outValue) override; >+ nsresult RemoveInlinePropertyCpp(nsIAtom* aProperty, >+ const nsAString& aAttribute) override; Those method should be declared here: https://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/editor/libeditor/HTMLEditor.h#220 >diff --git a/editor/libeditor/HTMLStyleEditor.cpp b/editor/libeditor/HTMLStyleEditor.cpp >--- a/editor/libeditor/HTMLStyleEditor.cpp >+++ b/editor/libeditor/HTMLStyleEditor.cpp >@@ -694,17 +719,17 @@ HTMLEditor::NodeIsProperty(nsINode& aNod > nsresult > HTMLEditor::ApplyDefaultProperties() > { > size_t defcon = mDefaultStyles.Length(); > for (size_t j = 0; j < defcon; j++) { > PropItem *propItem = mDefaultStyles[j]; > NS_ENSURE_TRUE(propItem, NS_ERROR_NULL_POINTER); > nsresult rv = >- SetInlineProperty(propItem->tag, propItem->attr, propItem->value); >+ SetInlinePropertyCpp(propItem->tag, propItem->attr, propItem->value); I guess you don't need this change anymore. >diff --git a/editor/libeditor/TextEditorTest.cpp b/editor/libeditor/TextEditorTest.cpp >--- a/editor/libeditor/TextEditorTest.cpp >+++ b/editor/libeditor/TextEditorTest.cpp >@@ -166,68 +166,68 @@ nsresult TextEditorTest::TestTextPropert > NS_ENSURE_TRUE(htmlEditor, NS_ERROR_FAILURE); > > bool any = false; > bool all = false; > bool first=false; > > const nsString& empty = EmptyString(); > >- rv = htmlEditor->GetInlineProperty(nsGkAtoms::b, empty, empty, &first, >- &any, &all); >+ rv = htmlEditor->GetInlinePropertyCpp(nsGkAtoms::b, empty, empty, &first, >+ &any, &all); I guess that you can replace nsGkAtoms::b with NS_LITERAL_STRING("b"). And similar for the below cases. Looks nice enough, but I'd like to check the new patch. So, r-'ing for now.

Thank you for the previous review. This version is much nicer :)

froydnj: the updated patch is much nicer. I'll let you decide if you want to take a look at it.

jorgk: if you are happy with this, please land it after the other patch lands on mozilla-central.

Comment on attachment 8901047 [details] [diff] [review] Update editorUtilities.js for the changes in nsIHTMLEditor Thanks, so we're switching this to strings, interesting.

Comment on attachment 8901034 [details] [diff] [review] Remove nsIAtom/nsIAtomService usage from script in editor/ Excellent!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e774e827d2fb359580c6975d2df01d5730c48d61 Bug 1393642 - Remove nsIAtom/nsIAtomService usage from script in editor/. r=masayuki.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/66b55e22b0c9 Update editorUtilities.js for the changes in nsIHTMLEditor. r=jorgk

njn, thanks for providing this patch and caring about Thunderbird :)

Comment on attachment 8901034 [details] [diff] [review] Remove nsIAtom/nsIAtomService usage from script in editor/ Review of attachment 8901034 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/HTMLStyleEditor.cpp @@ +58,5 @@ > +HTMLEditor::AddDefaultProperty(const nsAString& aProperty, > + const nsAString& aAttribute, > + const nsAString& aValue) > +{ > + return AddDefaultProperty(NS_Atomize(aProperty).take(), aAttribute, aValue); Won't this leak? Is there some reason we don't care if we leak these atoms?

> Won't this leak? Is there some reason we don't care if we leak these atoms? You're right, it will. Unless they are static atoms, and it turns out that all the uses of these methods (including in comm-central, which is where most of the uses are) involve static atoms such as "font", "big", "small", "href", and "name". Which explains why we don't have any test failures. Nonetheless, I will fix it. Thank you for noticing.

As written, these functions will leak if they are passed strings that don't match static atoms. In practice, all strings passed *do* match static atoms, but let's fix it anyway in case that changes in the future

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6fcd43e5a505475df2c38a8475bc84d485975b Bug 1393642 (follow-up) - Fix potential leak in HTMLEditor methods. r=masayuki.