Boris Zbarsky [:bzbarsky] Assignee
	Description • 12 years ago

      No description provided.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 12 years ago

Comment on attachment 674389 [details] [diff] [review]
And with some legacy methods

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

Just a few nits I noticed

::: content/base/public/nsIDocument.h
@@ +67,5 @@
>  class nsSMILAnimationController;
>  class nsStyleSet;
>  class nsWindowSizes;
> +class nsTextNode;
> +class nsRange;

I think this list is sorted, fwiw

::: content/base/src/nsDocument.cpp
@@ +4466,5 @@
> +  *aReturn = nsIDocument::CreateComment(aData, rv).get();
> +  return rv.ErrorCode();
> +}
> +
> +already_AddRefed<dom::Comment>

Do you need the dom::?

@@ +4475,5 @@
> +  if (NS_FAILED(res)) {
> +    rv.Throw(res);
> +    return nullptr;
> +  }
> +      

Trailing whitespace

@@ +4478,5 @@
> +  }
> +      
> +  // Don't notify; this node is still being created.
> +  comment->SetText(aData, false);
> +  return static_cast<dom::Comment*>(comment.forget().get());

:/

@@ +5171,5 @@
> +  if (NS_FAILED(res)) {
> +    rv.Throw(res);
> +    return nullptr;
> +  }
> +  

Trailing ws

@@ +5960,5 @@
> +  return CallQueryInterface(result, aResult);
> +}
> +
> +nsINode*
> +nsIDocument::AdoptNode(nsINode& aAdoptedNode, ErrorResult& rv)

aRv, I think

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 12 years ago

> Do you need the dom::?

Yes, otherwise "Comment" is ambiguous with something from Cocoa headers....

> :/

This will get better when we change those NS_New functions to suck less.  ;)

> aRv, I think

Hrm.  I kinda like it being "rv", actually.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 12 years ago

A note: I left both GetRootElement and GetDocumentElement, but we could also remove the former in favor of the latter...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 12 years ago

Comment on attachment 682041 [details] [diff] [review]
Including the WebIDL, so that can be matched up to the implementation

Hmm.  Actually, some of the stuff from HTML, not domcore, lives on nsIDOMDocument already.... So I need more things here.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 12 years ago

nsXPathEvaluator member, or if the WebIDL API were on
nsIDOMXPathEvaluator or something, all that stuff would need a lot
less code...

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 10 • 12 years ago

Comment on attachment 686441 [details] [diff] [review]
Add the WebIDL API for document.  not entirely happy with the XPath bits.  If we had an

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

::: content/base/src/nsDocument.cpp
@@ +5275,4 @@
>    nsRefPtr<nsRange> range = new nsRange();
> +  nsresult res = range->Set(this, 0, this, 0);
> +  if (NS_FAILED(res)) {
> +    rv.Throw(res);

Why not rv = like elsewhere?

@@ +10179,5 @@
> +                                   uri,
> +                                   uri,
> +                                   prin->GetPrincipal(),
> +                                   global,
> +                                   DocumentFlavorLegacyGuess,

Hrm. I'm not sure we need to use DocumentFlavorLegacyGuess for a new API.

::: dom/webidl/Document.webidl
@@ +97,5 @@
>  
> +http://www.whatwg.org/specs/web-apps/current-work/#the-document-object
> +partial interface Document {
> +*/
> +  [/*(Need Location in WebIDL for PutForwards)PutForwards=href,*/Unforgeable] readonly attribute Location? location;

Hrm. Is the lack of PutForwards not an issue?

@@ +322,5 @@
> +
> +/*
> +};
> +
> +http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html#api

http://fullscreen.spec.whatwg.org/

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 12 years ago

> Why not rv = like elsewhere?

I actually prefer not using the rv = version; I just got lazy about avoiding it partway through this thing.

> Hrm. I'm not sure we need to use DocumentFlavorLegacyGuess for a new API.

If I'm not trying to create an HTMLDocument or an SVGDocument, it's the only option.

> Hrm. Is the lack of PutForwards not an issue?

For now, yes, because right now that's done via the classinfo hooks for Document.  Obviously we'll need to address it before we actually switch the document object itself to WebIDL bindings.

	Peter Van der Beken [:peterv]
	Comment 13 • 12 years ago

Comment on attachment 688571 [details] [diff] [review]
Now with Location PutForwards stuff hooked up

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

::: content/base/public/nsIDocument.h
@@ +1867,5 @@
> +  void GetSelectedStyleSheetSet(nsAString& aSheetSet);
> +  virtual void SetSelectedStyleSheetSet(const nsAString& aSheetSet) = 0;
> +  virtual void GetLastStyleSheetSet(nsString& aSheetSet) = 0;
> +  void GetPreferredStyleSheetSet(nsAString& aSheetSet);
> +  virtual nsIDOMDOMStringList* StyleSheetSets() = 0;

Hmm, I thought we used nsDOMStringList.

::: content/base/public/nsINode.h
@@ +393,5 @@
>      return nullptr;
>    }
>  
>  public:
> +  virtual mozilla::dom::ParentObject GetParentObject() const;

I think that technically this doesn't need to be virtual, since we use the native type to call this. Given that I was planning to just have a concrete base one in nsINode and have concrete overriding ones wherever we need different behaviour (which should be only a few cases).

::: content/base/src/nsDocument.cpp
@@ +2787,5 @@
>    nsIFrame *rootFrame = ps->GetRootFrame();
>  
>    // XUL docs, unlike HTML, have no frame tree until everything's done loading
>    if (!rootFrame)
> +    return nullptr; // return null to premature XUL callers as a reminder to wait

Braces.

@@ +2792,5 @@
>  
>    nsIFrame *ptFrame = nsLayoutUtils::GetFrameForPoint(rootFrame, pt, true,
>                                                        aIgnoreRootScrollFrame);
>    if (!ptFrame)
> +    return nullptr;

Braces.

@@ +4987,5 @@
> +  if (rv.Failed()) {
> +    return rv.ErrorCode();
> +  }
> +
> +  return CallQueryInterface(result, aResult);

AsDOMNode?

@@ +6185,5 @@
> +  if (rv.Failed()) {
> +    return rv.ErrorCode();
> +  }
> +
> +  return CallQueryInterface(result, aResult);

AsDOMNode

@@ +6385,5 @@
> +  *aReturn = nsIDocument::CreateEvent(aEventType, rv).get();
> +  if (rv.Failed()) {
> +    return rv.ErrorCode();
> +  }
> +  return NS_OK;

Just return rv.ErrorCode()?

::: dom/webidl/Location.webidl
@@ +16,5 @@
> +  void assign(DOMString url);
> +  void replace(DOMString url);
> +  void reload();
> +
> +  // URL decomposition IDL attributes 

Trailing whitespace.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 12 years ago

> Hmm, I thought we used nsDOMStringList.

Sadly, no.  It's a nsDOMStyleSheetSetList.

> I think that technically this doesn't need to be virtual, since we use the native type
> call this.

Oh, hmm.  I guess this is called from the DocumentBinding::Wrap, which has an nsIDocument*, yeah.  Makes sense; I'll devirtualize this and add comments.

I'll fix the other nits.  Thank you for the review!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 12 years ago

Interestingly enough, the use of ParentObject actually caused us to fail editor/libeditor/base/crashtests/382527-1.html and content/test/reftest/bug592366-1.html due to compartment mismatches while wrapping the parent object in the middle of an adopt...  Switching to the non-virtual thing lets me not use ParentObject, so things are good.  I have no idea what was broken with it before.  :(

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f93588a041ec

Fingers crossed!

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f93588a041ec