560273 - Stop using DOM tearoffs from quickstubs

Status: RESOLVED FIXED

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

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Part 1 (nsINode::GetBaseURI) v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Part 2 (move code) v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Part 3 (UserData) v1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Part 4 (nsGenericTextNode) v1

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Part 5 (namespace-related methods) v1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: Part 6 (nsINode::GetFeature) v1

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Part 7 (nsINode::IsEqualNode/CompareDocumentPosition/IsSameNode) v1

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Part 8 (nsINode::Get/SetTextContent) v1

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Part 9 (nsNode3Tearoff for all nodes) v1

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Part 10 (nsNodeSelectorTearoff for nsDocument) v1

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Part 11 (nsIDOMNSElement) v1

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Part 12 (change 'canFail' default and allow missing 'code') v1

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Part 13 (stop using tearoffs) v1

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: Part 1 (nsINode::GetBaseURI) v1.1

Adds a virtual nsINode::GetBaseURI() and a non-virtual nsINode::GetBaseURI(nsAString&). The latter is not inline, though maybe it should be? (have to pull in nsIURI if so) Also renamed non-addrefing nsIDocument::GetBaseURI to nsIDocument::GetDocumentBaseURI.

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 440784 [details] [diff] [review]
Part 2 (move code) v1

Just moving code around, should have no functional changes.

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 440786 [details] [diff] [review]
Part 4 (nsGenericTextNode) v1

Add a nsGenericTextNode between nsGenericDOMDataNode and nsTextNode and nsXMLCDATASection for implementing nsIDOM3Text (quickstubs will be able to cast to nsGenericTextNode instead of QI'ing).

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Part 5 (namespace-related methods) v1

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Part 7 (nsINode::IsEqualNode/CompareDocumentPosition/IsSameNode) v1.1

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: Part 9 (nsNode3Tearoff for all nodes) v1

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Part 9 (nsNode3Tearoff for all nodes) v1

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: Part 10 (nsNodeSelectorTearoff for nsDocument) v1.1

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: Part 11 (nsIDOMNSElement) v1.1

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 440794 [details] [diff] [review]
Part 12 (change 'canFail' default and allow missing 'code') v1

This inverts the default value for canFail from False to True, so that if we by accident forget to add it we'll get a compile error (|rv = foo();| with foo returning void won't compile) or just check a return value that'll always be NS_OK. Currently, if we forget to add canFail it'll compile, but ignore the nsresult return value.
The patch also allows us to not add any custom code even if a thisType is specified, so that we can call functions with the same signature as the one we're stubbing but on a different interface/class, without having to write custom code.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 440784 [details] [diff] [review]
Part 2 (move code) v1

- In nsContentUtils.cpp:

+struct ClassMatchingInfo {
+  nsCOMArray<nsIAtom> mClasses;
+  nsCaseTreatment mCaseTreatment;
+};
+
+// static
+PRBool
+nsDocument::MatchClassNames(nsIContent* aContent,
+                            PRInt32 aNamespaceID,
+                            nsIAtom* aAtom, void* aData)

I don't see why this is important... I get why you move ClassMatchingInfo, but could we not put that in a header file and leave the nsDocument method implementations in nsDocument.cpp rather than separating the implementations of some class methods from others? Same goes for sticking nsContentUtils methods in nsGenericElement.cpp.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 442383 [details] [diff] [review]
Part 1 (nsINode::GetBaseURI) v1.1

Uber Nit: Feel free to name the function GetDocBaseURI as that's shorter and we want people to use it. Totally up to you.

>diff --git a/content/base/src/nsGenericDOMDataNode.cpp b/content/base/src/nsGenericDOMDataNode.cpp
> nsGenericDOMDataNode::GetBaseURI() const
> {
>   // DOM Data Node inherits the base from its parent element/document
>   nsIContent *parent = GetParent();
>   if (parent) {
>     return parent->GetBaseURI();
>   }
> 
>-  nsIURI *uri;
>   nsIDocument *doc = GetOwnerDoc();
>-  if (doc) {
>-    NS_IF_ADDREF(uri = doc->GetBaseURI());
>-  }
>-  else {
>-    uri = nsnull;
>-  }
> 
>-  return uri;
>+  return doc ? doc->GetBaseURI() : nsnull;

Nit: You could use GetDocumentBaseURI here, right?

r=me

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 442409 [details] [diff] [review]
Part 7 (nsINode::IsEqualNode/CompareDocumentPosition/IsSameNode) v1.1

This will conflict with bz's changes in bug 562688, but should be fairly straight forward to update.

Comment 27

[Peter Van der Beken [:peterv]]

(In reply to comment #24)
> I don't see why this is important...

Ok, I'll leave GetElementsByClassName on nsDocument. But later patches actually move those nsNodeUtils and nsContentUtils methods to nsINode (see for example attachment 442409 [details] [diff] [review] for s/nsContentUtils::ComparePosition/nsINode::CompareDocumentPosition/). Are you objecting to that too?

(In reply to comment #25)
> > nsGenericDOMDataNode::GetBaseURI() const

> >+  return doc ? doc->GetBaseURI() : nsnull;
> 
> Nit: You could use GetDocumentBaseURI here, right?

Well, GetDocumentBaseURI returns a non-addrefed pointer but GetBaseURI returns an already_AddRefed so I don't think that would work as is, I'd need to NS_IF_ADDREF.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 442412 [details] [diff] [review]
Part 9 (nsNode3Tearoff for all nodes) v1

 NS_INTERFACE_TABLE_HEAD(nsDocument)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_DOCUMENT_INTERFACE_TABLE_BEGIN(nsDocument)
-    NS_INTERFACE_TABLE_ENTRY(nsDocument, nsINode)

This is either wrong, or I'm really confused.

r=jst with that sorted out or explained.

Comment 29

[Peter Van der Beken [:peterv]]

Attachment: Part 13 (stop using tearoffs) v1.1

I'll file a bug about making nsDocumentFragment not inherit from nsGenericElement and mention the number in the comment in nsDOMQS.h.

Comment 30

[Peter Van der Beken [:peterv]]

(In reply to comment #27)
> (In reply to comment #24)
> > I don't see why this is important...
> 
> Ok, I'll leave GetElementsByClassName on nsDocument.

I started doing that, but it turns out it's a bit of a hassle to inline GetElementsByClassName in nsGenericElement then, so I ended up keeping the attached patch.

(In reply to comment #28)
> (From update of attachment 442412 [details] [diff] [review])
>  NS_INTERFACE_TABLE_HEAD(nsDocument)
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>    NS_DOCUMENT_INTERFACE_TABLE_BEGIN(nsDocument)
> -    NS_INTERFACE_TABLE_ENTRY(nsDocument, nsINode)
> 
> This is either wrong, or I'm really confused.

Ah, sorry, forgot to mention that when asking for review. Turns out that these were redundant, NS_NODE_OFFSET_AND_INTERFACE_TABLE_BEGIN adds nsINode already (NS_DOCUMENT_INTERFACE_TABLE_BEGIN calls NS_NODE_OFFSET_AND_INTERFACE_TABLE_BEGIN, same thing for NS_NODE_INTERFACE_TABLE* in nsDOMAttribute.cpp).

Comment 31

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/a5e772392c05
http://hg.mozilla.org/mozilla-central/rev/d3b20261c4ef
http://hg.mozilla.org/mozilla-central/rev/ef29b5596b77
http://hg.mozilla.org/mozilla-central/rev/d1a0437dff3e
http://hg.mozilla.org/mozilla-central/rev/3abca4e8001c
http://hg.mozilla.org/mozilla-central/rev/5813224215db
http://hg.mozilla.org/mozilla-central/rev/4434bc7cd016
http://hg.mozilla.org/mozilla-central/rev/0786d96d6d21
http://hg.mozilla.org/mozilla-central/rev/c635db22672e
http://hg.mozilla.org/mozilla-central/rev/7b8a7ae82c41
http://hg.mozilla.org/mozilla-central/rev/08a17f1b0c14
http://hg.mozilla.org/mozilla-central/rev/74e306877161
http://hg.mozilla.org/mozilla-central/rev/00955067e4b5