839116 - Convert HTMLSharedElement to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Rename nsHTMLSharedElement to HTMLSharedElement.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Convert HTMLSharedElement to WebIDL.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add tests for HTMLSharedElement attribute reflection.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 711501 [details] [diff] [review]
part 1.  Rename nsHTMLSharedElement to HTMLSharedElement.

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

::: content/html/content/src/nsHTMLSharedElement.cpp
@@ +7,1 @@
>  

Please remove this one too

@@ -20,5 @@
>  #include "nsMappedAttributes.h"
>  #include "nsContentUtils.h"
>  
> -using namespace mozilla;
> -using namespace mozilla::dom;

I've wondered how long this would survive... Good to see it go, though :)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 711502 [details] [diff] [review]
part 2.  Convert HTMLSharedElement to WebIDL.

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

lgtm

::: content/html/content/src/HTMLSharedElement.h
@@ +108,5 @@
>    }
> +
> +  // WebIDL API
> +  // HTMLParamElement
> +  void GetName(mozilla::dom::DOMString& aValue)

No need for the mozilla::dom:: here, and mozilla:: below.

@@ +110,5 @@
> +  // WebIDL API
> +  // HTMLParamElement
> +  void GetName(mozilla::dom::DOMString& aValue)
> +  {
> +    GetHTMLAttr(nsGkAtoms::name, aValue);

Consider asserting that these are called on the right elements.

@@ +150,5 @@
> +  void SetTarget(const nsAString& aValue, mozilla::ErrorResult& aResult)
> +  {
> +    SetHTMLAttr(nsGkAtoms::target, aValue, aResult);
> +  }
> +  // The XPCOM GetHref is fine for us

You added DOMString versions of all other getters, why not this one?

@@ +161,5 @@
> +  bool Compact() const
> +  {
> +    return GetBoolAttr(nsGkAtoms::compact);
> +  }
> +  void SetCompact(bool aCompact, mozilla::ErrorResult& rv)

aResult

@@ +188,5 @@
> +  }
> +
> +protected:
> +  virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope,
> +                             bool *aTriedToWrap) MOZ_OVERRIDE;

* to the left, please

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 711502 [details] [diff] [review]
part 2.  Convert HTMLSharedElement to WebIDL.

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

::: content/html/content/src/HTMLSharedElement.h
@@ +169,5 @@
> +
> +  // HTMLQuoteElement
> +  void GetCite(mozilla::dom::DOMString& aValue)
> +  {
> +    GetHTMLAttr(nsGkAtoms::cite, aValue);

Except that this should be reflect as a URL.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 711503 [details] [diff] [review]
part 3.  Add tests for HTMLSharedElement attribute reflection.

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

::: content/html/content/test/Makefile.in
@@ +263,5 @@
> +		test_param_attributes_reflection.html \
> +		test_base_attributes_reflection.html \
> +		test_dir_attributes_reflection.html \
> +		test_q_attributes_reflection.html \
> +		test_html_attributes_reflection.html \

I guess trying to keep those somewhat sorted is a lost cause...

::: content/html/content/test/test_li_attributes_reflection.html
@@ +17,2 @@
>  
> +// .href is a URI attr; we don't have a way to test those yet

URL :)

(Actually, looks like it's even more complex in the spec.)

@@ +24,1 @@
>  });

This test isn't correct; it should be reflected as a URL. I'm not going to ask you to write a testing function for that, though :)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> No need for the mozilla::dom:: here, and mozilla:: below.
> Consider asserting that these are called on the right elements.

Good catches.

> You added DOMString versions of all other getters, why not this one?

Because this one is building the string dynamically, so wouldn't benefit from the DOMString optimizations anyway.

> Except that this should be reflect as a URL.

Oh, _good_ catch.  We really need a way to test reflection of URL attrs.  :(  I guess I'll remove the now-no-op test for now.

> This test isn't correct; it should be reflected as a URL.

I assume this is about the QuoteElement test.  If not, please let me know!

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> > You added DOMString versions of all other getters, why not this one?
> 
> Because this one is building the string dynamically, so wouldn't benefit
> from the DOMString optimizations anyway.

Makes sense.

> > Except that this should be reflect as a URL.
> 
> Oh, _good_ catch.  We really need a way to test reflection of URL attrs.  :(
> I guess I'll remove the now-no-op test for now.

Thanks.

> > This test isn't correct; it should be reflected as a URL.
> 
> I assume this is about the QuoteElement test.  If not, please let me know!

Yep.

Comment 10

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/7cfb020b01ac
   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb74c00aff76
   https://hg.mozilla.org/integration/mozilla-inbound/rev/33abe9d3d691

Comment 11

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/8e84efba19b9 to fix a broken test.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cfb020b01ac
https://hg.mozilla.org/mozilla-central/rev/cb74c00aff76
https://hg.mozilla.org/mozilla-central/rev/33abe9d3d691
https://hg.mozilla.org/mozilla-central/rev/8e84efba19b9