Convert HTMLMenuElement to WebIDL

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: amarchesini, Assigned: amarchesini)

Tracking

(Blocks: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

15.52 KB, patch
amarchesini
: review+
Details | Diff | Splinter Review
9.59 KB, patch
amarchesini
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 710680 [details] [diff] [review]
part 1 - renaming
Attachment #710680 - Flags: review?(peterv)
Created attachment 710681 [details] [diff] [review]
part 2 - webidl
Attachment #710681 - Flags: review?(peterv)
Created attachment 710690 [details] [diff] [review]
part 2 - webidl (b)

the webidl file was missing.
Attachment #710681 - Attachment is obsolete: true
Attachment #710681 - Flags: review?(peterv)
Attachment #710690 - Flags: review?(peterv)
Attachment #710680 - Flags: review?(peterv)
Attachment #710690 - Flags: review?(peterv)
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 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.
Created attachment 710800 [details] [diff] [review]
part 1 - renaming (b)
Attachment #710680 - Attachment is obsolete: true
Attachment #710800 - Flags: review?(Ms2ger)
Created attachment 710802 [details] [diff] [review]
part 2 - webidl (c)
Attachment #710690 - Attachment is obsolete: true
Attachment #710802 - Flags: review?(Ms2ger)
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 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+
Created attachment 710832 [details] [diff] [review]
part 1 - renaming (c)
Attachment #710800 - Attachment is obsolete: true
Attachment #710832 - Flags: review+
Created attachment 710835 [details] [diff] [review]
part 2 - webidl (d)
Attachment #710802 - Attachment is obsolete: true
Attachment #710835 - Flags: review+
Keywords: checkin-needed
Created attachment 711344 [details] [diff] [review]
part 2 - webidl (e)
Attachment #710835 - Attachment is obsolete: true
Attachment #711344 - Flags: review?(peterv)
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+
Created attachment 715102 [details] [diff] [review]
part 1 - renaming
Attachment #710832 - Attachment is obsolete: true
Attachment #715102 - Flags: review+
Created attachment 715103 [details] [diff] [review]
part 2 - webidl
Attachment #711344 - Attachment is obsolete: true
Attachment #715103 - Flags: review+
Created attachment 715104 [details] [diff] [review]
part 2 - webidl

'HTMLMenuElement': {
    'nativeType': 'nsIDOMTMLMenuElement'
},

removed.
Attachment #715103 - Attachment is obsolete: true
Attachment #715104 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cab8ac207ba4
https://hg.mozilla.org/mozilla-central/rev/a09e3d0f005d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.