Convert HTMLMenuElement to WebIDL

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: baku, Assigned: baku)

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)

Assignee

Description

7 years ago
No description provided.
Assignee

Comment 2

7 years ago
Posted patch part 1 - renaming (obsolete) — Splinter Review
Attachment #710680 - Flags: review?(peterv)
Assignee

Comment 3

7 years ago
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #710681 - Flags: review?(peterv)
Assignee

Comment 4

7 years ago
Posted patch part 2 - webidl (b) (obsolete) — Splinter Review
the webidl file was missing.
Attachment #710681 - Attachment is obsolete: true
Attachment #710681 - Flags: review?(peterv)
Attachment #710690 - Flags: review?(peterv)
Assignee

Updated

6 years ago
Attachment #710680 - Flags: review?(peterv)
Assignee

Updated

6 years ago
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.
Assignee

Comment 8

6 years ago
Posted patch part 1 - renaming (b) (obsolete) — Splinter Review
Attachment #710680 - Attachment is obsolete: true
Attachment #710800 - Flags: review?(Ms2ger)
Assignee

Comment 9

6 years ago
Posted patch part 2 - webidl (c) (obsolete) — Splinter Review
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+
Assignee

Comment 12

6 years ago
Posted patch part 1 - renaming (c) (obsolete) — Splinter Review
Attachment #710800 - Attachment is obsolete: true
Attachment #710832 - Flags: review+
Assignee

Comment 13

6 years ago
Posted patch part 2 - webidl (d) (obsolete) — Splinter Review
Attachment #710802 - Attachment is obsolete: true
Attachment #710835 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
Assignee

Comment 16

6 years ago
Posted patch part 2 - webidl (e) (obsolete) — Splinter Review
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+
Assignee

Comment 19

6 years ago
Attachment #710832 - Attachment is obsolete: true
Attachment #715102 - Flags: review+
Assignee

Comment 20

6 years ago
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #711344 - Attachment is obsolete: true
Attachment #715103 - Flags: review+
Assignee

Comment 21

6 years ago
'HTMLMenuElement': {
    'nativeType': 'nsIDOMTMLMenuElement'
},

removed.
Attachment #715103 - Attachment is obsolete: true
Attachment #715104 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cab8ac207ba4
https://hg.mozilla.org/mozilla-central/rev/a09e3d0f005d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.