Last Comment Bug 657938 - Implement content part of the meter element
: Implement content part of the meter element
Status: RESOLVED FIXED
[mentor=volkmar]
: dev-doc-needed, html5, student-project
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: dularylaurent
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 661256
Blocks: 555985 657953
  Show dependency treegraph
 
Reported: 2011-05-18 08:23 PDT by Vincent LAMOTTE
Modified: 2012-06-06 06:18 PDT (History)
8 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First version of nsHTMLMeterElement (14.95 KB, patch)
2011-05-18 09:46 PDT, dularylaurent
no flags Details | Diff | Review
tests for the content part of the meter element (first version) (11.02 KB, patch)
2011-05-20 07:49 PDT, Yoan TEBOUL
no flags Details | Diff | Review
tests v1.1 (10.73 KB, patch)
2011-05-20 08:31 PDT, Yoan TEBOUL
no flags Details | Diff | Review
tests v1.2 (10.74 KB, patch)
2011-05-21 03:41 PDT, Yoan TEBOUL
no flags Details | Diff | Review
tests v1.3 (10.74 KB, patch)
2011-05-24 09:15 PDT, Yoan TEBOUL
bugs: review+
mounir: feedback+
Details | Diff | Review
Patch v1.1 (31.36 KB, patch)
2011-05-24 09:28 PDT, dularylaurent
bugs: review+
mounir: feedback+
Details | Diff | Review
Check .value, .min, .max, .low, .high and .optimum reflection (996 bytes, text/html)
2011-05-25 06:38 PDT, Mounir Lamouri (:mounir)
no flags Details
Patch v1.2 (31.53 KB, patch)
2011-05-26 07:52 PDT, dularylaurent
no flags Details | Diff | Review
tests_v2.0 (11.63 KB, patch)
2011-05-26 09:32 PDT, Yoan TEBOUL
no flags Details | Diff | Review
Patch v1.3 (31.53 KB, patch)
2011-05-26 10:01 PDT, dularylaurent
no flags Details | Diff | Review

Description Vincent LAMOTTE 2011-05-18 08:23:39 PDT
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 2011-05-18 09:46:35 PDT
Created attachment 533308 [details] [diff] [review]
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 2011-05-20 07:49:46 PDT
Created attachment 533971 [details] [diff] [review]
tests for the content part of the meter element (first version)
Comment 3 Yoan TEBOUL 2011-05-20 08:31:56 PDT
Created attachment 533984 [details] [diff] [review]
tests v1.1

removed some whitespaces and switched to unix EOL.
Comment 4 Yoan TEBOUL 2011-05-21 03:41:09 PDT
Created attachment 534216 [details] [diff] [review]
tests v1.2
Comment 5 Yoan TEBOUL 2011-05-24 09:15:23 PDT
Created attachment 534793 [details] [diff] [review]
tests v1.3
Comment 6 dularylaurent 2011-05-24 09:28:44 PDT
Created attachment 534797 [details] [diff] [review]
Patch v1.1

Implementation of content part of the meter element.
All the tests succeeds.
Ready for review.
Comment 7 dularylaurent 2011-05-24 10:53:35 PDT
Comment on attachment 534797 [details] [diff] [review]
Patch v1.1

Review of attachment 534797 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 8 :Ms2ger 2011-05-24 12:13:10 PDT
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 2011-05-24 12:14:24 PDT
And please add a test to content/html/content/test/test_bug389797.html.
Comment 10 :Ms2ger 2011-05-24 12:45:13 PDT
(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) 2011-05-25 05:50:00 PDT
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) 2011-05-25 05:54:28 PDT
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] 2011-05-25 06:06:20 PDT
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) 2011-05-25 06:11:34 PDT
(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] 2011-05-25 06:17:05 PDT
> 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] 2011-05-25 06:31:07 PDT
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) 2011-05-25 06:38:38 PDT
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.
Comment 18 Mounir Lamouri (:mounir) 2011-05-25 06:41:20 PDT
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) 2011-05-25 08:36:25 PDT
(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) 2011-05-25 08:58:16 PDT
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) 2011-05-25 09:17:14 PDT
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) 2011-05-25 09:18:04 PDT
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 2011-05-25 10:08:05 PDT
"for each" is still considered harmful, please use array.forEach() or a plain or loop.
Comment 24 Mounir Lamouri (:mounir) 2011-05-25 10:09:41 PDT
(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 2011-05-26 07:52:36 PDT
Created attachment 535338 [details] [diff] [review]
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) 2011-05-26 08:30:28 PDT
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) 2011-05-26 08:32:15 PDT
(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 2011-05-26 09:32:30 PDT
Created attachment 535367 [details] [diff] [review]
tests_v2.0

added a test to content/html/content/test/test_bug389797.html and adjusted indentation.
Comment 29 dularylaurent 2011-05-26 10:01:00 PDT
Created attachment 535378 [details] [diff] [review]
Patch v1.3

Note You need to log in before you can comment on or make changes to this bug.