Closed
Bug 839116
Opened 12 years ago
Closed 12 years ago
Convert HTMLSharedElement to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
(deleted),
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•12 years ago
|
Blocks: ParisBindings, 826738
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #711501 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #711502 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #711503 -
Flags: review?(Ms2ger)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [need review]
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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 6•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
> 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•12 years ago
|
||
(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.
Assignee | ||
Comment 10•12 years ago
|
||
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
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla21
Assignee | ||
Comment 11•12 years ago
|
||
And https://hg.mozilla.org/integration/mozilla-inbound/rev/8e84efba19b9 to fix a broken test.
Comment 12•12 years ago
|
||
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
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
•