Closed
Bug 225060
Opened 22 years ago
Closed 22 years ago
deCOMtaminate nsINodeInfo / nsINodeInfoManager
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: bryner, Assigned: bryner)
Details
Attachments
(1 file)
|
70.32 KB,
patch
|
sicking
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
Patch coming up.
|
Assignee | |
Comment 1•22 years ago
|
||
Moved some members up to nsINodeInfo / nsINodeInfoManager, got rid of
NS_IMETHOD_(foo) (replaced with [virtual] foo), got rid of unused nsresult
return values, replaced nsISupportsArray with nsCOMArray.
|
|
Assignee | |
Comment 2•22 years ago
|
||
Comment on attachment 135048 [details] [diff] [review]
patch
One other thing for reviewers - the NS_ADDREF for the name atom vs.
NS_IF_ADDREF for the prefix atom is intentional -- only the name atom is
guaranteed to never be null.
Attachment #135048 -
Flags: superreview?(jst)
Attachment #135048 -
Flags: review?(bugmail)
|
|
||
Comment 3•22 years ago
|
||
Do we really need the nsINodeInfo vs nsNodeInfo distinction?
|
|
Assignee | |
Comment 4•22 years ago
|
||
Well... there are users of nsINodeInfo outside of gklayout (transformiix,
editor, cookie, and webservices). Don't we need to have all the methods on the
interface they use be either inline or pure virtual to avoid link errors? Maybe
we only need to worry about the methods they actually call.
Another way we could go about this is to just use nsNodeInfo within layout, and
create an nsINodeInfo wrapper for external callers who want one (it would just
forward the calls to the nsNodeInfo that it wraps).
|
|
||
Comment 5•22 years ago
|
||
Yeah, that may be nice (the nsNodeInfo inside, nsINodeInfo outside idea).
Comment on attachment 135048 [details] [diff] [review]
patch
>@@ -113,9 +186,8 @@ public:
> * For the HTML element "<body>" this will return the "body" atom and for
> * the XML element "<html:body>" this will return the "body" atom.
> */
>- already_AddRefed<nsIAtom> GetNameAtom() const
>+ nsIAtom* GetNameAtom() const
> {
>- NS_ADDREF(mInner.mName);
> return mInner.mName;
> }
According to the latest naming-convention this should be called |NameAtom|.
Same goes for GetNodeInfoManager and (probably) GetNamespaceID. Not sure if you
want to do that in a separate patch? (though it should be fairly easy to do a
search-n-replace in the patch)
>@@ -1216,8 +1216,7 @@ nsXULElement::SetAttribute(const nsAStri
> {
> nsCOMPtr<nsINodeInfo> ni = GetExistingAttrNameFromQName(aName);
> if (!ni) {
>- nsCOMPtr<nsINodeInfoManager> nimgr;
>- NodeInfo()->GetNodeInfoManager(getter_AddRefs(nimgr));
>+ nsINodeInfoManager *nimgr = NodeInfo()->GetNodeInfoManager();
> NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE);
Optional nit: You can remove the NS_ENSURE_TRUE. Same in
nsGenericElement::SetAttribute, nsGenericElement::SetAttributeNS,
nsGenericHTMLElement::GetExistingAttrNam, nsXULElement::SetAttr,
nsXULElement::SetInlineStyleRule and a lot of places in nsDOMAttributeMap.cpp.
>@@ -4674,7 +4658,7 @@ nsresult
> nsXULPrototypeElement::Deserialize(nsIObjectInputStream* aStream,
> nsIScriptContext* aContext,
> nsIURI* aDocumentURI,
>- nsISupportsArray* aNodeInfos)
>+ const nsCOMArray<nsINodeInfo> *aNodeInfos)
> {
> NS_PRECONDITION(aNodeInfos, "missing nodeinfo array");
> nsresult rv;
>@@ -4682,7 +4666,7 @@ nsXULPrototypeElement::Deserialize(nsIOb
> // Read Node Info
> PRUint32 number;
> rv = aStream->Read32(&number);
>- mNodeInfo = do_QueryElementAt(aNodeInfos, number);
>+ mNodeInfo = aNodeInfos->ObjectAt(number);
You need to make sure that |number| isn't out-of-bounds before calling
->ObjectAt, do_QueryElementAt was boundssafe but ObjectAt isn't. Or add an
nsCOMArray::SafeObjectAt (calling nsVoidArray::SafeElementAt), we should
probably have that function anyway.
> if (!mNodeInfo)
> return NS_ERROR_UNEXPECTED;
>
>@@ -4699,7 +4683,7 @@ nsXULPrototypeElement::Deserialize(nsIOb
> nsAutoString attributeValue;
> for (i = 0; i < mNumAttributes; ++i) {
> rv |= aStream->Read32(&number);
>- mAttributes[i].mNodeInfo = do_QueryElementAt(aNodeInfos, number);
>+ mAttributes[i].mNodeInfo = aNodeInfos->ObjectAt(number);
Same thing here.
With that, r=sicking
Attachment #135048 -
Flags: review?(bugmail) → review+
|
||
Comment 7•22 years ago
|
||
Comment on attachment 135048 [details] [diff] [review]
patch
Very nice! sr=jst
Attachment #135048 -
Flags: superreview?(jst) → superreview+
|
|
Assignee | |
Comment 8•22 years ago
|
||
checked in, plus bustage fix from foo->Blah(getter_AddRefs(foo)) - type crashes
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
|
||
Updated•7 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•