Closed Bug 824940 Opened 7 years ago Closed 7 years ago

convert HTMLTitleElement to webidl

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attachment #696017 - Flags: review?(bzbarsky)
Comment on attachment 696016 [details] [diff] [review]
rename nsHTMLTitleElement -> mozilla::dom::HTMLTitleElement

I would somewhat prefer if the closing '}' for the namespaces were followed with "// namespace mozilla" and the like.

I would also somewhat prefer if the cpp file wrapped everything except the NS_IMPL_NS_NEW_HTML_ELEMENT and DOMCI_NODE_DATA in namespaces, instead of "using namespace mozilla::dom".

r=me with those nits.
Attachment #696016 - Flags: review?(bzbarsky) → review+
Comment on attachment 696016 [details] [diff] [review]
rename nsHTMLTitleElement -> mozilla::dom::HTMLTitleElement

Er, and please include the header as "mozilla/dom/HTMLTitleElement.h" in the C++.
Comment on attachment 696017 [details] [diff] [review]
convert HTMLTitleElement to webidl

>+++ b/content/html/content/src/HTMLTitleElement.cpp
>+HTMLTitleElement::SetText(const nsAString& aText, ErrorResult& aError)

Any reason to not just inline that in the header?

>+++ b/content/html/content/src/HTMLTitleElement.h
>+  nsIDocument* GetParentObject() const { return OwnerDoc(); }

Why do you need that?  nsINode::GetParentObject() should do the right thing already, last I checked.

>+++ b/dom/webidl/HTMLTitleElement.webidl
>+ * http://www.whatwg.org/specs/web-apps/current-work/ and

Please link to the specific section?

>+ * http://dev.w3.org/csswg/cssom-view/

I don't think any part of this comes from cssom-view.

>+[PrefControlled]

Why does this need to be prefcontrolled?
> >+++ b/content/html/content/src/HTMLTitleElement.cpp
> >+HTMLTitleElement::SetText(const nsAString& aText, ErrorResult& aError)
> 
> Any reason to not just inline that in the header?

I guess not, though when the xpidl goes away we'll be including nsContentUtils in the header, but that's probably not a big deal.

> >+++ b/content/html/content/src/HTMLTitleElement.h
> >+  nsIDocument* GetParentObject() const { return OwnerDoc(); }
> 
> Why do you need that?  nsINode::GetParentObject() should do the right thing
> already, last I checked.

nice, I wasn't aware and was too silly to check.

> >+[PrefControlled]
> 
> Why does this need to be prefcontrolled?

hm, so its possible to have non pref controlled thing inherit from a pref controlled interface?

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 696016 [details] [diff] [review]
> rename nsHTMLTitleElement -> mozilla::dom::HTMLTitleElement
> 
> Er, and please include the header as "mozilla/dom/HTMLTitleElement.h" in the
> C++.

sure, though I'm sort of curious why it matters.
> though when the xpidl goes away we'll be including nsContentUtils in the header

We can out-of-line it then if needed.

> hm, so its possible to have non pref controlled thing inherit from a pref controlled
> interface?

Nothing prevents it per se.  I agree it's weird, so maybe something should, but the good news is that the PrefControlled bit on HTMLElement is a total hack; it always returns true, but has side-effects that matter.  So in practice it's never preffed off.

> sure, though I'm sort of curious why it matters.

I think it can affect compile times in some cases if the same header is included from different locations in the filesystem...  In any case, best to just be consistent.
Attachment #696017 - Attachment is obsolete: true
Attachment #696017 - Flags: review?(bzbarsky)
Attachment #696336 - Flags: review?(bzbarsky)
Comment on attachment 696336 [details] [diff] [review]
bug 824940 - convert HTMLTitleElement to webidl

r=me
Attachment #696336 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/470bffebe490
https://hg.mozilla.org/mozilla-central/rev/9451057c1e00
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.