Closed
Bug 804631
Opened 12 years ago
Closed 12 years ago
WebIDL API for Document
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #674375 -
Attachment is obsolete: true
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
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.
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #681893 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Attachment #674389 -
Attachment is obsolete: true
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...
Assignee | ||
Updated•12 years ago
|
Whiteboard: [need review]
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #682041 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Attachment #681893 -
Attachment is obsolete: true
Attachment #681893 -
Flags: review?(peterv)
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.
Attachment #682041 -
Flags: review?(peterv) → review-
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...
Attachment #686441 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Attachment #682041 -
Attachment is obsolete: true
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/
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.
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #688571 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Attachment #686441 -
Attachment is obsolete: true
Attachment #686441 -
Flags: review?(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.
Attachment #688571 -
Flags: review?(peterv) → review+
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!
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. :(
Assignee | ||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f93588a041ec
Fingers crossed!
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla20
Comment 17•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•