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

RESOLVED FIXED in mozilla28

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: heycam, Assigned: bz)

Tracking

Trunk
mozilla28
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(6 attachments, 2 obsolete attachments)

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
Created attachment 8341515 [details] [diff] [review]
part 1.  Make NS_NewHTMLElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341515 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8341517 [details] [diff] [review]
part 2.  Make NS_NewXULElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341517 - Flags: review?(bugs)
Created attachment 8341519 [details] [diff] [review]
part 3.  Make NS_NewMathMLElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341519 - Flags: review?(bugs)
Created attachment 8341520 [details] [diff] [review]
part 4.  Make NS_NewXMLElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341520 - Flags: review?(bugs)
Created attachment 8341521 [details] [diff] [review]
part 6.  Make NS_NewElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341521 - Flags: review?(bugs)
Whiteboard: [need review]
Created attachment 8341528 [details] [diff] [review]
part 5.  Make NS_NewSVGElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341528 - Flags: review?(bugs)
Created attachment 8341532 [details] [diff] [review]
part 3.  Make NS_NewMathMLElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341532 - Flags: review?(bugs)
Attachment #8341519 - Attachment is obsolete: true
Attachment #8341519 - Flags: review?(bugs)
Created attachment 8341533 [details] [diff] [review]
part 4.  Make NS_NewXMLElement take an Element** outparam instead of an nsIContent** one.
Attachment #8341533 - 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae6663f558c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f841d982dcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e643dc35f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6538f6d647
https://hg.mozilla.org/integration/mozilla-inbound/rev/15480b9592fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b17d20681a5

and a missing header followup (that built for me locally, too; yay --enable-optimize on tinderbox):

https://hg.mozilla.org/integration/mozilla-inbound/rev/debee75c3c1c
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.
https://hg.mozilla.org/mozilla-central/rev/2ae6663f558c
https://hg.mozilla.org/mozilla-central/rev/4f841d982dcb
https://hg.mozilla.org/mozilla-central/rev/e2e643dc35f5
https://hg.mozilla.org/mozilla-central/rev/fb6538f6d647
https://hg.mozilla.org/mozilla-central/rev/15480b9592fc
https://hg.mozilla.org/mozilla-central/rev/6b17d20681a5
https://hg.mozilla.org/mozilla-central/rev/debee75c3c1c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.