838559 - Convert HTMLMenuElement to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=8239d4dc0cd7

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (b)

the webidl file was missing.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 7

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=ef78d185afde

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming (b)

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (c)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming (c)

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (d)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0b056ced49
https://hg.mozilla.org/integration/mozilla-inbound/rev/34927afb65e2

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (e)

Comment 17

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=0a1e9cb05950 green on try

Comment 18

[Peter Van der Beken [: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.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

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

removed.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cab8ac207ba4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a09e3d0f005d

Comment 23

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/cab8ac207ba4
https://hg.mozilla.org/mozilla-central/rev/a09e3d0f005d