Closed Bug 945572 Opened 6 years ago Closed 6 years ago

make NS_NewHTMLElement take an Element* out param rather than nsIContent*

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: heycam, Assigned: bzbarsky)

References

Details

(Whiteboard: [qa-])

Attachments

(6 files, 2 obsolete files)

14.22 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.17 KB, patch
smaug
: review+
Details | Diff | Splinter Review
22.82 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.05 KB, patch
smaug
: review+
Details | Diff | Splinter Review
The version of NS_NewHTMLElement that takes an nsIContent* out param might more usefully take an Element* out param.
Blocks: 945573
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Attachment #8341519 - Attachment is obsolete: true
Attachment #8341519 - Flags: review?(bugs)
Attachment #8341520 - Attachment is obsolete: true
Attachment #8341520 - Flags: review?(bugs)
Comment on attachment 8341515 [details] [diff] [review]
part 1.  Make NS_NewHTMLElement take an Element** outparam instead of an nsIContent** one.

Review of attachment 8341515 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +11465,5 @@
>    nodeInfo = mNodeInfoManager->GetNodeInfo(aTag, nullptr, kNameSpaceID_XHTML,
>                                             nsIDOMNode::ELEMENT_NODE);
>    MOZ_ASSERT(nodeInfo, "GetNodeInfo should never fail");
>  
> +  nsCOMPtr<Element> element;

Is nsRefPtr insufficient? (Same comment throughout patches)
nsCOMPtr produces smaller code than nsRefPtr, last I checked.

And given that Element is an honest-to-God interface, with an IID and everything, there is no reason to not use nsCOMPtr here.
Comment on attachment 8341515 [details] [diff] [review]
part 1.  Make NS_NewHTMLElement take an Element** outparam instead of an nsIContent** one.


>   const nsCString& GetType() const { return mMimeType; }
>-  nsIContent*      GetPluginContent() { return mPluginContent; }
>+  Element*      GetPluginContent() { return mPluginContent; }
Nit, either align the method name with the other methods, or
use one space between * and the method name


(Looks like nsIDocument::CreateHTMLElement makes it easy to forget to think about
*_FROM_*. We probably should change that API to take mozilla::dom::FromParser)
Attachment #8341515 - Flags: review?(bugs) → review+
Attachment #8341517 - Flags: review?(bugs) → review+
Comment on attachment 8341521 [details] [diff] [review]
part 6.  Make NS_NewElement take an Element** outparam instead of an nsIContent** one.


>         else {
>             // It's just a generic element. Create it!
>-            rv = CreateElement(nameSpaceID, tag, getter_AddRefs(realKid));
>+            nsCOMPtr<Element> element;
>+            rv = CreateElement(nameSpaceID, tag, getter_AddRefs(element));
>             if (NS_FAILED(rv)) return rv;
>+            realKid = element;
realKid = element.forget();
Attachment #8341521 - Flags: review?(bugs) → review+
Attachment #8341528 - Flags: review?(bugs) → review+
Attachment #8341532 - Flags: review?(bugs) → review+
Attachment #8341533 - Flags: review?(bugs) → review+
Comment on attachment 8341515 [details] [diff] [review]
part 1.  Make NS_NewHTMLElement take an Element** outparam instead of an nsIContent** one.

Review of attachment 8341515 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +11471,3 @@
>                                    mozilla::dom::NOT_FROM_PARSER);
>  
>    MOZ_ASSERT(NS_SUCCEEDED(rv), "NS_NewHTMLElement should never fail");

Followup on dropping the rv?

::: content/xslt/src/xslt/txMozillaTextOutput.cpp
@@ +243,5 @@
>                                          nsIContent** aResult)
>  {
> +    nsCOMPtr<Element> element = mDocument->CreateHTMLElement(aName);
> +    element.forget(aResult);
> +    return NS_OK;

Followup on dropping the rv here?
> Looks like nsIDocument::CreateHTMLElement makes it easy to forget to think about
> *_FROM_*.

Hrm.  I consider that a feature: it's just like the DOM createElementNS except without having to pass in an XHTML namespace string.

If we ever made DOM's createElement() always return HTML elements, we could kill this method off altogether.
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla28
Why aren't we making NS_New*Element return an already_AddRefed<Element> directly?
We can't do that for NS_NewHTMLElement because such a thing already exists and means something different (specifically, means to return a generic HTML element, as opposed to basing the decision on the nodeinfo).

We could rename that existing function and then change the signature here, but that's a somewhat non-mechanical change.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.