657938 - Implement content part of the meter element

Status: RESOLVED FIXED

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

Description

[Vincent LAMOTTE]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

Block of the bug 555985, implement the meter element.
Refers to the content part.

Reproducible: Always

Comment 1

[dularylaurent]

Attachment: First version of nsHTMLMeterElement

This implementation is following the specifications of the settings of the different parameters.
This part must then be tested.

Comment 2

[Yoan TEBOUL]

Attachment: tests for the content part of the meter element (first version)

Comment 3

[Yoan TEBOUL]

Attachment: tests v1.1

removed some whitespaces and switched to unix EOL.

Comment 4

[Yoan TEBOUL]

Attachment: tests v1.2

Comment 5

[Yoan TEBOUL]

Attachment: tests v1.3

Comment 6

[dularylaurent]

Attachment: Patch v1.1

Implementation of content part of the meter element.
All the tests succeeds.
Ready for review.

Comment 7

[dularylaurent]

Comment on attachment 534797 [details] [diff] [review]
Patch v1.1

Review of attachment 534797 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 8

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

Comment on attachment 534797 [details] [diff] [review]
Patch v1.1

>diff -r b54fc6516c0c content/html/content/src/nsHTMLMeterElement.cpp
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/content/html/content/src/nsHTMLMeterElement.cpp	Tue May 24 18:13:45 2011 +0200
>@@ -0,0 +1,376 @@
>+ * The Initial Developer of the Original Code is Mozilla Foundation

Is that the case?

>+class nsHTMLMeterElement : public nsGenericHTMLFormElement,
>+                              public nsIDOMHTMLMeterElement

Spacing is messed up here.

The getters here are wrong, I'm afraid. They all want to use NS_IMPL_DOUBLE_ATTR or NS_IMPL_DOUBLE_ATTR_DEFAULT_VALUE. I'm not sure which, so I filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=12742>.

>+nsHTMLMeterElement::GetMin(double* aValue)
>+nsHTMLMeterElement::GetMax(double* aValue)
>+nsHTMLMeterElement::GetLow(double* aValue)
>+nsHTMLMeterElement::GetHigh(double* aValue)
>+nsHTMLMeterElement::GetOptimum(double* aValue)

>+nsHTMLMeterElement::GetValue(double* aValue)

(This one could be right, actually.)

Also, in general, please use NS_M{IN,AX} instead of the PR* variants. (You might need to include nsAlgorithm.h.)

>diff -r b54fc6516c0c dom/interfaces/html/nsIDOMHTMLMeterElement.idl
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/dom/interfaces/html/nsIDOMHTMLMeterElement.idl	Tue May 24 18:13:45 2011 +0200
>+/**
>+ * The nsIDOMHTMLMeterElement interface is the interface to a HTML
>+ * <meter> element.
>+ *
>+ * For more information on this interface, please see
>+ * http://www.whatwg.org/specs/web-apps/current-work/#the-meter-element

Could you please link to the multipage spec?

>+ *
>+ * @status UNDER_DEVELOPMENT

No need to bother with this line, IMO.

I haven't really reviewed the patch for correctness, but it looks pretty good in general.

Comment 9

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

And please add a test to content/html/content/test/test_bug389797.html.

Comment 10

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

(In reply to comment #8)
> Comment on attachment 534797 [details] [diff] [review] [review]
> The getters here are wrong, I'm afraid. They all want to use
> NS_IMPL_DOUBLE_ATTR or NS_IMPL_DOUBLE_ATTR_DEFAULT_VALUE. I'm not sure
> which, so I filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=12742>.
> 
> >+nsHTMLMeterElement::GetMin(double* aValue)
> >+nsHTMLMeterElement::GetMax(double* aValue)
> >+nsHTMLMeterElement::GetLow(double* aValue)
> >+nsHTMLMeterElement::GetHigh(double* aValue)
> >+nsHTMLMeterElement::GetOptimum(double* aValue)

Hixie says NS_IMPL_DOUBLE_ATTR. You probably want to add a function along the lines of |double nsHTMLMeterElement::Min() {}| for the actual minimum and similar functions for the others.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 534797 [details] [diff] [review]
Patch v1.1

Review of attachment 534797 [details] [diff] [review]:
-----------------------------------------------------------------

For headers (with licensing information), please make sure that the year is "2011", the project isn't "mozilla.org" but "Mozilla". For the original developer, I'm not sure...

According to the specs, min, max, low, high and optimum IDL attributes should reflect the content attributes. IOW, you should use NS_IMPL_DOUBLE_ATTR. Though, the value IDL attribute is reflecting the actually value. It seems inconsistent to me and I wouldn't be against the same behavior for all attributes. Olli ond Ms2ger do you have an opinion?
What other UA do regarding this?

If you choose to make min, max, low, [...] IDL attributes reflecting the content attributes, I would prefer to not have ::Low(), ::High() and ::Optimum() added in this patch but when needed (likely with the layout one). You might need ::Min() and ::Max() if you follow what the specs say.

f=me with these nits and Ms2ger nits fixed.

::: dom/interfaces/html/nsIDOMHTMLMeterElement.idl
@@ +59,5 @@
> +           attribute double high;
> +           attribute double optimum;
> +  readonly attribute nsIDOMHTMLFormElement form;
> +  /**
> +   * The labels attribute will be done with bug ???.

bug 556743

::: parser/htmlparser/src/nsElementTable.cpp
@@ +882,5 @@
> +    /*parent,incl,exclgroups*/          kFormControl, kFlowEntity, kNone,
> +    /*special props, prop-range*/       0,kDefaultPropRange,
> +    /*special parents,kids*/            0,0,
> +  },
> +  {

You will have to ask a review to mrbkap for this part.

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 534793 [details] [diff] [review]
tests v1.3

Review of attachment 534793 [details] [diff] [review]:
-----------------------------------------------------------------

I can't really give any feedback here because the tests might change. Though, it's mostly a copy-paste from the progress element tests so it seems ok to me (obviously ;)).
However, if you make min, max, low, high and optimum IDL attributes reflecting the content attributes, could you add a |reflectDouble| method to content/html/content/tests/reflect.js?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 534797 [details] [diff] [review]
Patch v1.1

>+ * The Initial Developer of the Original Code is Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.

I assume this is right because you have copied some code from
<progress> element implementation.



r=me, +what Ms2ger and volkmar have said

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to comment #13)
> Comment on attachment 534797 [details] [diff] [review] [review]
> Patch v1.1
> 
> >+ * The Initial Developer of the Original Code is Mozilla Foundation
> >+ * Portions created by the Initial Developer are Copyright (C) 2011
> >+ * the Initial Developer. All Rights Reserved.
> 
> I assume this is right because you have copied some code from
> <progress> element implementation.

Indeed. The right way would probably to copy-paste the license header from nsHTMLProgressElement.cpp and add your names after mine.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

> Though, the value IDL attribute is reflecting the
> actually value. It seems inconsistent to me and I wouldn't be against the
> same behavior for all attributes. Olli ond Ms2ger do you have an opinion?
I'd say do what the spec says, for now.

> What other UA do regarding this?
This is a good question

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 534793 [details] [diff] [review]
tests v1.3

Could you ask review for the tests once the main patch has been updated

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Check .value, .min, .max, .low, .high and .optimum reflection

So, Webkit and Presto do not follow the specs and do what this patch is trying to do. I believe we should do the same thing.

Comment 18

[Mounir Lamouri (:mounir)]

By the way, I wonder why the default optimum is the midpoint between min and max and not max. I would say that most <meter> like widget out there have optimum <=> max.

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #17)
> Created attachment 535047 [details]
> Check .value, .min, .max, .low, .high and .optimum reflection
> 
> So, Webkit and Presto do not follow the specs and do what this patch is
> trying to do. I believe we should do the same thing.

Specs but open: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12780

Comment 20

[Mounir Lamouri (:mounir)]

Comment on attachment 534797 [details] [diff] [review]
Patch v1.1

Review of attachment 534797 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLMeterElement.cpp
@@ +164,5 @@
> +  if (attrMin && attrMin->Type() == nsAttrValue::eDoubleValue) {
> +    *aValue = attrMin->GetDoubleValue();
> +  } else {
> +    *aValue = kDefaultMin;
> +  }

Could you do this instead:
if (attrMin && attrMin->Type() == nsAttrValue::eDoubleValue) {
  *aValue = attrMin->GetDoubleValue();
  return NS_OK;
}

*aValue = kDefaultMin;
return NS_OK;

@@ +180,5 @@
> +nsHTMLMeterElement::GetMax(double* aValue)
> +{
> +  /**
> +   * If the attribute max is defined, the maximum is this value.
> +   * Otherwise, the minimum is the default value.

I think you meant "maximum" here. And please, write a comment explaining the entire method instead of splitting them inside the code.

@@ +212,5 @@
> +nsHTMLMeterElement::GetValue(double* aValue)
> +{
> +  /**
> +   * If the attribute value is defined, the actual value is this value.
> +   * Otherwise, the actual value is the default value.

Same thing: write a comment explaining the entire method.

@@ +228,5 @@
> +   */
> +  double min;
> +  GetMin(&min);
> +
> +  *aValue = PR_MAX(*aValue, min);

If aValue < min, then aValue will be set to min. In that case, it's useless to check aValue against max because max is known to be greater or equals to min.

You can do that by checking after NS_MAX():
if (aValue == min) {
  return NS_OK;
}

Or, and I would prefer that, instead of NS_MAX():
if (aValue <= min) {
  *aValue = min;
  return NS_OK;
}

@@ +252,5 @@
> +NS_IMETHODIMP
> +nsHTMLMeterElement::GetLow(double* aValue)
> +{
> +  double max;
> +  GetMax(&max);

Move the line before using max.

@@ +259,5 @@
> +  GetMin(&min);
> +
> +  /**
> +   * If the low value is defined, the low value is this value.
> +   * Otherwise, the low value is the minimum value.

Write a comment explaining the entire method at the beginning of the method.

@@ +272,5 @@
> +  /**
> +   * If the low value is less than the minimum value,
> +   * the low value is the same as the minimum value.
> +   */
> +  *aValue = PR_MAX(*aValue, min);

You can move this inside the if statement. IOW, you don't need to compare aValue and min if you went trough the else statement: |*aValue = min;|.

@@ +300,5 @@
> +  GetLow(&low);
> +
> +  /**
> +   * If the high value is defined, the high value is this value.
> +   * Otherwise, the high value is the maximum value.

Write a comment explaining the entire method at the beginning of the method.

@@ +319,5 @@
> +  /**
> +   * If the high value is greater than the maximum value,
> +   * the high value is the same as the maximum value.
> +   */
> +  *aValue = PR_MIN(*aValue, max);

Like for |GetLow()|: you can move this inside the if statement.

@@ +342,5 @@
> +
> +  /**
> +   * If the optimum value is defined, the optimum value is this value.
> +   * Otherwise, the optimum value is the midpoint between
> +   * the minimum value and the maximum value.

Write a comment explaining the entire method at the beginning of the method.

@@ +349,5 @@
> +              mAttrsAndChildren.GetAttr(nsGkAtoms::optimum);
> +  if (attrOptimum && attrOptimum->Type() == nsAttrValue::eDoubleValue) {
> +    *aValue = attrOptimum->GetDoubleValue();
> +  } else {
> +    *aValue = (min + max) / 2;

Could you write a comment explaining this is equivalent to: min + (max - min)/2 ? Might not be obvious at first glance.
I would prefer to use 2.0 instead of 2 because we are using doubles, not integers.

@@ +356,5 @@
> +  /**
> +   * If the optimum value is less than the minimum value,
> +   * the optimum value is the same as the minimum value.
> +   */
> +  *aValue = PR_MAX(*aValue, min);

You can move this inside the if statement.

@@ +362,5 @@
> +  /**
> +   * If the optimum value is greater than the maximum value,
> +   * the optimum value is the same as the maximum value.
> +   */
> +  *aValue = PR_MIN(*aValue, max);

You can move this inside the if statement.

@@ +364,5 @@
> +   * the optimum value is the same as the maximum value.
> +   */
> +  *aValue = PR_MIN(*aValue, max);
> +
> +  return NS_OK;

Actually, it might be better to change this to:
if (attrOptimum && attrOptimum->Type() != nsAttrValue::eDoubleValue) {
  *aValue = min + (min + max) / 2.0;
  return NS_OK;
}

// else ...

Comment 21

[Mounir Lamouri (:mounir)]

Comment on attachment 534793 [details] [diff] [review]
tests v1.3

Review of attachment 534793 [details] [diff] [review]:
-----------------------------------------------------------------

f=me

::: content/html/content/test/test_bug657938.html
@@ +116,5 @@
> +{
> +  var tests = [
> +    // min default value is 0.0.
> +    [ null, 0.0 ],
> +  [ 'foo', 0.0 ],

Indentation :)

@@ +192,5 @@
> +
> +  var element = document.createElement('meter');
> +
> +  for each(var test in tests) {
> +

Please, remove this blank line :)

@@ +234,5 @@
> +
> +  var element = document.createElement('meter');
> +
> +  for each(var test in tests) {
> +

Please, remove this blank line :)

@@ +276,5 @@
> +
> +  var element = document.createElement('meter');
> +
> +  for each(var test in tests) {
> +

Please, remove this blank line :)

Comment 22

[Mounir Lamouri (:mounir)]

Comment on attachment 534793 [details] [diff] [review]
tests v1.3

Olli, I'm re-assigning the review to you given that we are going to implement what is tested here I guess.

Comment 23

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

"for each" is still considered harmful, please use array.forEach() or a plain or loop.

Comment 24

[Mounir Lamouri (:mounir)]

(In reply to comment #23)
> "for each" is still considered harmful, please use array.forEach() or a
> plain or loop.

I've been using "for each" in a lot of my tests. We can open a bug to have them all changed but I don't think it really worth changing that only here.

Comment 25

[dularylaurent]

Attachment: Patch v1.2

Correction of the patch, following the different remarks over the precedent patch.

I haven't changed the implementation of the getters and still don't follow the specs. I wait for a clear answer on the better implementation.

Comment 26

[Mounir Lamouri (:mounir)]

mrbkap gave an oral r+ for the parser part of the patch.
As soon as we have the test update fixing the nits, everything will be ready for this bug.

Assigning this to Laurent.

Comment 27

[Mounir Lamouri (:mounir)]

(In reply to comment #25)
> I haven't changed the implementation of the getters and still don't follow
> the specs. I wait for a clear answer on the better implementation.

You don't need to follow the specs given that Presto (Opera) and Webkit (Chrome and Safari) do not follow the specs either. Your implementation is the same as theirs. I've opened a bug against the specs to have them changed in the way it is actually implemented by all vendors.

Comment 28

[Yoan TEBOUL]

Attachment: tests_v2.0

added a test to content/html/content/test/test_bug389797.html and adjusted indentation.

Comment 29

[dularylaurent]

Attachment: Patch v1.3

Comment 30

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/203c76227211
https://hg.mozilla.org/mozilla-central/rev/9822d3931e31