824940 - convert HTMLTitleElement to webidl

Status: RESOLVED FIXED

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

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: rename nsHTMLTitleElement -> mozilla::dom::HTMLTitleElement

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: convert HTMLTitleElement to webidl

Comment 3

[Boris Zbarsky [: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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Trevor Saunders (:tbsaunde)]

> >+++ 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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: bug 824940 - convert HTMLTitleElement to webidl

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696336 [details] [diff] [review]
bug 824940 - convert HTMLTitleElement to webidl

r=me

Comment 10

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/470bffebe490
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9451057c1e00

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/470bffebe490
https://hg.mozilla.org/mozilla-central/rev/9451057c1e00