Convert HTMLSharedElement to WebIDL

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla21
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

No description provided.
Whiteboard: [need review]
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 :)
Attachment #711501 - Flags: review?(Ms2ger) → review+
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
Attachment #711502 - Flags: review?(Ms2ger) → review+
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 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 :)
Attachment #711503 - Flags: review?(Ms2ger) → review+
> 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!
(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.

Updated

6 years ago
Depends on: 847119

Updated

6 years ago
No longer depends on: 847119
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.