User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110527 Firefox/7.0a1 Build Identifier: The meter element is basically implemented in the Bug #657953 (no optimum management). The purpose of this bug is to add different colors for the meter bar according to the optimum and the value. Reproducible: Always

Changing the summary given that this is going to be done trough pseudo-classes. These names could be used: :-moz-meter-optimum :-moz-meter-sub-optimum :-moz-meter-sub-sub-optimum It's only a proposition, use other names if you feel like it.

Content changes to add states for optimal, sub-optimal and sub-sub-optimal <meter> elements.

Comment on attachment 536255 [details] [diff] [review] Content Patch v1.0 Review of attachment 536255 [details] [diff] [review]: ----------------------------------------------------------------- You should write tests too ;) ::: content/events/public/nsEventStates.h @@ +265,5 @@ > #define NS_EVENT_STATE_MOZ_UI_VALID NS_DEFINE_EVENT_STATE_MACRO(33) > +// Content is in the optimum region. > +#define NS_EVENT_STATE_OPTIMUM NS_DEFINE_EVENT_STATE_MACRO(34) > +// Content is in the suboptimal region. > +#define NS_EVENT_STATE_SUBOPTIMUM NS_DEFINE_EVENT_STATE_MACRO(35) I would prefer this name: NS_EVENT_STATE_SUB_OPTIMUM And you should probably add this state: NS_EVENT_STATE_SUB_SUB_OPTIMUM ::: content/html/content/src/nsHTMLMeterElement.cpp @@ +71,5 @@ > NS_IMETHOD_(PRUint32) GetType() const { return NS_FORM_METER; } > NS_IMETHOD Reset(); > NS_IMETHOD SubmitNamesValues(nsFormSubmission* aFormSubmission); > > + nsEventStates IntrinsicState(); Should be: virtual nsEventStates IntrinsicState() const; @@ +88,5 @@ > + * 0 if the actual value is in the sub-suboptimal region. > + * > + * @return the region of the actual value. > + */ > + bool IsOptimum(); 2 and 1 are going to be equivalent of 'true' here while '0' will be false. You should probably create an enum for those three states. And make the method const and private, please. Actually, I think you could change "protected" to "private". @@ +158,5 @@ > + state |= NS_EVENT_STATE_SUBOPTIMUM; > + } else if (isOptimum == 0) { > + state &= ~NS_EVENT_STATE_OPTIMUM; > + state &= ~NS_EVENT_STATE_SUBOPTIMUM; > + } I don't think you need to unset the other states given that they are not going to be set by nsGenericHTMLFormElement::IntrinsicState(). Also, it looks like you want to have sub-sub-optimum equivalent to not optimum and not sub-optimum. I don't think we should do that. @@ +441,5 @@ > + * and then the value returned is 1. > + * if the value is in [minimum, low[, > + * the value is in the sub-suboptimal region > + * and then the value returned is 0. > + */ I think this comment is too deep. You should just explain the general idea. @@ +476,5 @@ > + return 1; > + } else { > + return 0; > + } > + } In this function, you should use the tertiary operator "?" to make it simpler to read. In addition. when using "return", you shouldn't add "else" after. For example: if (foo > bar) { return 0; } return 1;

As said on IRC, it might be better to split this in two patches: 1. Create the needed const methods (for IntrinsincState) and have current getters calling them. 2. What should be done in this bug. And BTW, IsOptimum() should be renamed "GetOptimumState()" instead, returning NS_EVENT_STATE{,_SUB,_SUB_SUB}_OPTIMUM so you will be able to do: |state |= GetOptimumState();|.

I've created const getters which can be called by IntrinsticState and by the current getters.

Realisation of the IntrinsticState function with add of NS_EVENT_STATE_{,SUB_,SUB_SUB_}OPTIMUM states.

Correction of coding-style errors.

Comment on attachment 536566 [details] [diff] [review] Modification of getters Review of attachment 536566 [details] [diff] [review]: ----------------------------------------------------------------- f=mounir with all the nits fixed. ::: content/html/content/src/nsHTMLMeterElement.cpp @@ +83,5 @@ > static const double kDefaultValue; > static const double kDefaultMin; > static const double kDefaultMax; > + > +private: You can just rename protected to private. We don't need to have these static variables protected. @@ +201,5 @@ > + * Otherwise, the maximum is the default value. > + * If the maximum value is less than the minimum value, > + * the maximum value is the same as the minimum value. > + */ > + double aValue; Please rename the variable (max or value, as you want). The reader might be confused assuming it's a parameter. @@ +224,5 @@ > + * the actual value is the same as the minimum value. > + * If the actual value is greater than the maximum value, > + * the actual value is the same as the maximum value. > + */ > + double aValue; ditto @@ +261,5 @@ > + if (!attrLow || attrLow->Type() != nsAttrValue::eDoubleValue) { > + return min; > + } > + > + double aValue = attrLow->GetDoubleValue(); ditto @@ +289,5 @@ > + if (!attrHigh || attrHigh->Type() != nsAttrValue::eDoubleValue) { > + return max; > + } > + > + double aValue = attrHigh->GetDoubleValue(); ditto @@ +322,5 @@ > + if (!attrOptimum || attrOptimum->Type() != nsAttrValue::eDoubleValue) { > + return (min + max) / 2.0; > + } > + > + double aValue = attrOptimum->GetDoubleValue(); ditto

Comment on attachment 536569 [details] [diff] [review] Creation of IntrinsticState Review of attachment 536569 [details] [diff] [review]: ----------------------------------------------------------------- You have to add the new pseudo-classes to this file: layout/style/nsCSSPseudoClassList.h ::: content/html/content/src/nsHTMLMeterElement.cpp @@ +467,5 @@ > + if (value <= high) { > + return NS_EVENT_STATE_SUB_OPTIMUM; > + } > + return NS_EVENT_STATE_SUB_SUB_OPTIMUM; > + } else if (optimum <= high) { You don't need the |else if| here. You can just put an |if| statement. @@ +471,5 @@ > + } else if (optimum <= high) { > + if (value >= low && value <= high) { > + return NS_EVENT_STATE_OPTIMUM; > + } > + return NS_EVENT_STATE_SUB_OPTIMUM; Could be: return (value >= low && value <= high) ? NS_EVENT_STATE_OPTIMUM : NS_EVENT_STATE_SUB_OPTIMUM; @@ +472,5 @@ > + if (value >= low && value <= high) { > + return NS_EVENT_STATE_OPTIMUM; > + } > + return NS_EVENT_STATE_SUB_OPTIMUM; > + } else { Don't need the |else|.

Comment on attachment 536615 [details] [diff] [review] Creation of IntrinsticState v1.1 Review of attachment 536615 [details] [diff] [review]: ----------------------------------------------------------------- Great :) ::: content/html/content/src/nsHTMLMeterElement.cpp @@ +455,5 @@ > + double low = GetLow(); > + > + double high = GetHigh(); > + > + double optimum = GetOptimum(); You don't need a blank line between each declaration. @@ +472,5 @@ > + return NS_EVENT_STATE_OPTIMUM; > + } > + return NS_EVENT_STATE_SUB_OPTIMUM; > + } > + if (value > high) { A comment like: "// optimum > high" could be helpful. Though, I would set the optimum > high case in second and optimum in [low, high] for the last case. This said, it's a detail ;)

Comment on attachment 536617 [details] [diff] [review] Content Tests v1.0 Review of attachment 536617 [details] [diff] [review]: -----------------------------------------------------------------

https://hg.mozilla.org/mozilla-central/rev/ca2a01618e43 https://hg.mozilla.org/mozilla-central/rev/3ffbdbac1f7f https://hg.mozilla.org/mozilla-central/rev/25c91c36638c

Small question in order to help me in documenting this. Is this in a spec in the standard track? If so in which one? If not, is this planned? (So that I can track the spec in the future)

Nothing is planned for the moment. You can point to what webkit does though: http://trac.webkit.org/wiki/Styling%20Form%20Controls#meter