Closed
Bug 838559
Opened 11 years ago
Closed 11 years ago
Convert HTMLMenuElement to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: baku, Assigned: baku)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 9 obsolete files)
15.52 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
9.59 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•11 years ago
|
Blocks: ParisBindings, 826738
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8239d4dc0cd7
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #710680 -
Flags: review?(peterv)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #710681 -
Flags: review?(peterv)
Assignee | ||
Comment 4•11 years ago
|
||
the webidl file was missing.
Attachment #710681 -
Attachment is obsolete: true
Attachment #710681 -
Flags: review?(peterv)
Attachment #710690 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #710680 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #710690 -
Flags: review?(peterv)
Comment 5•11 years ago
|
||
Comment on attachment 710680 [details] [diff] [review] part 1 - renaming Review of attachment 710680 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsGenericHTMLElement.h @@ +44,4 @@ > } > } > > +using namespace mozilla::dom; No using statements in headers, please.
Comment 6•11 years ago
|
||
Comment on attachment 710690 [details] [diff] [review] part 2 - webidl (b) Review of attachment 710690 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLMenuElement.cpp @@ +258,5 @@ > } > > +JSObject* > +HTMLMenuElement::WrapNode(JSContext *aCx, JSObject *aScope, > + bool* aTriedToWrap) Indentation is off, and all *s to the left. ::: content/html/content/src/HTMLMenuElement.h @@ +72,5 @@ > + bool Compact() const > + { > + return GetBoolAttr(nsGkAtoms::compact); > + } > + void SetCompact(bool& aCompact, mozilla::ErrorResult& aError) No & for aCompact. ::: dom/webidl/HTMLMenuElement.webidl @@ +4,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + * > + * The origin of this IDL file is > + * http://www.whatwg.org/specs/web-apps/current-work/#the-menu-element > + * http://www.whatwg.org/specs/web-apps/current-work/#dom-hr-color Really? Oh, I guess you got that from the status box, which probably makes it my fault. Please use the section ID, though.
Assignee | ||
Comment 7•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ef78d185afde
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #710680 -
Attachment is obsolete: true
Attachment #710800 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #710690 -
Attachment is obsolete: true
Attachment #710802 -
Flags: review?(Ms2ger)
Comment 10•11 years ago
|
||
Comment on attachment 710800 [details] [diff] [review] part 1 - renaming (b) Review of attachment 710800 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: content/html/content/src/nsHTMLMenuElement.cpp @@ +256,5 @@ > aSeparator = ST_TRUE; > } > + > +} // namespace mozilla > +} // namespace dom Other way around ::: content/html/content/src/nsHTMLMenuElement.h @@ +3,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#ifndef __mozilla_dom_htmlmenuelement_h__ > +#define __mozilla_dom_htmlmenuelement_h__ I would tend to use 'mozilla_dom_HTMLMenuElement_h'. @@ +70,5 @@ > uint8_t mType; > }; > + > +} // dom > +} // mozilla Add 'namespace', please
Attachment #710800 -
Flags: review?(Ms2ger) → review+
Comment 11•11 years ago
|
||
Comment on attachment 710802 [details] [diff] [review] part 2 - webidl (c) Review of attachment 710802 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLMenuElement.cpp @@ +259,5 @@ > } > > +JSObject* > +HTMLMenuElement::WrapNode(JSContext *aCx, JSObject *aScope, > + bool* aTriedToWrap) Previous comments still apply. ::: content/html/content/src/HTMLMenuElement.h @@ +55,5 @@ > uint8_t GetType() const { return mType; } > > + /** > + * WebIDL > + */ Nit: this isn't a javadoc comment; // will do. @@ +72,5 @@ > + bool Compact() const > + { > + return GetBoolAttr(nsGkAtoms::compact); > + } > + void SetCompact(bool aCompact, mozilla::ErrorResult& aError) You don't need mozilla:: qualification. @@ +80,5 @@ > + > + > +protected: > + virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope, > + bool *aTriedToWrap) MOZ_OVERRIDE; * to the left, please ::: dom/webidl/HTMLMenuElement.webidl @@ +19,5 @@ > +}; > + > +// http://www.whatwg.org/specs/web-apps/current-work/#other-elements,-attributes-and-apis > +partial interface HTMLMenuElement { > + attribute boolean compact; [SetterThrows] for all of these
Attachment #710802 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #710800 -
Attachment is obsolete: true
Attachment #710832 -
Flags: review+
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #710802 -
Attachment is obsolete: true
Attachment #710835 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0b056ced49 https://hg.mozilla.org/integration/mozilla-inbound/rev/34927afb65e2
Keywords: checkin-needed
Comment 15•11 years ago
|
||
Backed out for causing mochitest-1 failures in test_contextmenu.html. https://hg.mozilla.org/integration/mozilla-inbound/rev/c71474d33f63 https://tbpl.mozilla.org/php/getParsedLog.php?id=19504491&tree=Mozilla-Inbound
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #710835 -
Attachment is obsolete: true
Attachment #711344 -
Flags: review?(peterv)
Assignee | ||
Comment 17•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0a1e9cb05950 green on try
Comment 18•11 years ago
|
||
Comment on attachment 711344 [details] [diff] [review] part 2 - webidl (e) Review of attachment 711344 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLMenuElement.cpp @@ +120,5 @@ > +{ > + if (!nsContentUtils::IsCallerChrome()) { > + aRv = NS_ERROR_DOM_SECURITY_ERR; > + return nullptr; > + } This is marked as ChromeOnly, so no need to check for this (which means you can remove [Throws] from the webidl). I'd also make the XPCOM implementation just call this one after it does its chrome check.
Attachment #711344 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #710832 -
Attachment is obsolete: true
Attachment #715102 -
Flags: review+
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #711344 -
Attachment is obsolete: true
Attachment #715103 -
Flags: review+
Assignee | ||
Comment 21•11 years ago
|
||
'HTMLMenuElement': { 'nativeType': 'nsIDOMTMLMenuElement' }, removed.
Attachment #715103 -
Attachment is obsolete: true
Attachment #715104 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cab8ac207ba4 https://hg.mozilla.org/integration/mozilla-inbound/rev/a09e3d0f005d
Flags: in-testsuite?
Keywords: checkin-needed
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cab8ac207ba4 https://hg.mozilla.org/mozilla-central/rev/a09e3d0f005d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•