Remove nsIAtom/nsIAtomService usage from script in editor/

RESOLVED FIXED in mozilla57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 affected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Attachment #8900977 - Flags: review?(masayuki)
Attachment #8900977 - Flags: feedback?(nfroyd)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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.
Attachment #8900977 - Flags: review?(masayuki) → review-
Thank you for the previous review. This version is much nicer :)
Attachment #8901034 - Flags: review?(masayuki)
Attachment #8900977 - Attachment is obsolete: true
Attachment #8900977 - Flags: feedback?(nfroyd)
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.
Attachment #8901047 - Flags: review?(jorgk)
Comment on attachment 8901047 [details] [diff] [review]
Update editorUtilities.js for the changes in nsIHTMLEditor

Thanks, so we're switching this to strings, interesting.
Attachment #8901047 - Flags: review?(jorgk) → review+
Blocks: 1392884, 1392883
Comment on attachment 8901034 [details] [diff] [review]
Remove nsIAtom/nsIAtomService usage from script in editor/

Excellent!
Attachment #8901034 - Flags: review?(masayuki) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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
Attachment #8905321 - Flags: review?(masayuki)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6fcd43e5a505475df2c38a8475bc84d485975b
Bug 1393642 (follow-up) - Fix potential leak in HTMLEditor methods. r=masayuki.
You need to log in before you can comment on or make changes to this bug.