159757 - HTMLValue parsing methods belong in HTMLValue

Status: RESOLVED FIXED

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

Description

[John Keiser (jkeiser)]

All sorts of HTMLValue parsing methods--to and from strings--are in
nsGenericHTMLElement.  Most of these belong in HTMLValue.  We should move them
there.  Also a lot of them can be consolidated with no performance
loss--ValueOrPercentToString and ValueOrPercentOrProportionalToString being one
such example.

Comment 1

[John Keiser (jkeiser)]

This does not include the HTML specific ones :)

Comment 2

[John Keiser (jkeiser)]

Attachment: Patch

This patch moves the methods over and makes them a bit slimmer (and fewer) than
they were before.

In Patch Viewer (collapsed initially to show an overview of changes, click
"Expand All" or the "(+)" twisties):
http://landfill.bugzilla.org/bz176656/attachment.cgi?id=117&action=prettyview&collapsed=true

Comment 3

[John Keiser (jkeiser)]

The major changes to the methods were collapsing all the ParseValueOrPercent*
methods into one (ParseIntValue with bool params), and collapsing *all* the
string -outputting methods into one ToString() method on nsHTMLValue.

Comment 4

[John Keiser (jkeiser)]

Attachment: Testcase

This testcase includes all the things I could think of to test.  Every single
kind of attribute that this patch touches is there.

Comment 5

[John Keiser (jkeiser)]

Attachment: Testcase (strict)

Color attribute parsing is different between quirks and strict mode.  This
testcase is in strict.

Comment 6

[John Keiser (jkeiser)]

BTW, there *is* a single difference introduced (deliberately) with this patch:
now we parse "*" the same as "%" -- i.e. since we accept "1 % x" as 1%, we
accept "1 * x" as "1*".

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

a better name for ParseIntValue might be ParseNumberValue

+    if (aDefaultUnit == eHTMLUnit_Pixel) {
+      SetPixelValue(val);
+    } else {
+      SetIntValue(val, aDefaultUnit);
+    }

Is this really needed? isn't SetPixelValue(val) the same as SetIntValue(val,
eHTMLUnit_Pixel)? This is in ParseIntWithBounds and ParseIntValue.



nsHTMLValue::ParseEnumValue would probably be faster if you did

if (aCaseSensitive) {
  while() {
    ..
  }
}
else {
  while() {
    ..
  }
}

It's a tad bigger, but since this is used fairly much i think it'd be worth it.
Also i think EqualsWithConversion/EqualsIgnoreCase are deprecated (or at least
other methods are prefered to it)



+  nsAutoString tmp(aString);
+  PRInt32 ec;
+  PRInt32 val = tmp.ToInteger(&ec);
+
+  if (ec == NS_OK) {

yach! That should be either |if(!ec)| or |if(NS_SUCCEEDED(rc))|. Same in
ParseIntWithBounds


+      SetIntValue(val, aDefaultUnit);
+    }
+    return PR_TRUE;
+
+  // Even if the integer could not be parsed, it might just be "*"
+  } else if (aCanBeProportional) {
+    tmp.CompressWhitespace(PR_TRUE, PR_TRUE);

else after return


+PRBool
+nsHTMLValue::ToString(nsAString& aResult) const
+{
+  nsAutoString intStr;
+  aResult.Truncate(0);

nit: Truncate() is enough (and a tad clearer IMHO)

+  switch (GetUnit()) {
+    case eHTMLUnit_Integer:
+    case eHTMLUnit_Pixel:
+    case eHTMLUnit_Proportional:
+      intStr.AppendInt(GetIntValue());
+      aResult.Append(intStr);

couldn't you append directly to aResult? same in eHTMLUnit_Percent



+    if (val < aMin) {
+      val = aMin;
+    }
+    if (val > aMax) {
+      val = aMax;
+    }

nit: You could use PR_MIN and PR_MAX


+  // Previously we did a complicated algorithm to strip leading and trailing
+  // whitespace; now we just use CompressWhitespace like everyone else.
+  // Since all color values are just one word, this is ok.

That other algorithm might have been faster, and done correctly would avoid the
copy.


+    // XXX Why do we bother converting to RGB if we're storing the colorname?
To make sure that it's really a colorname and not "foo"?


+    // % (percent) (XXX RFindChar means that 5%x will be parsed!)
+    if (aCanBePercent && tmp.RFindChar('%') >= 0) {

Don't know if it matters or not, but you might as well do a forward-seach.


with that, r=sicking. Nice patch!

Comment 8

[John Keiser (jkeiser)]

Fixed:
- That should be either |if(!ec)| or |if(NS_SUCCEEDED(rc))|. Same in
ParseIntWithBounds
- else after return
- use PR_MIN and PR_MAX

Other Responses:

> a better name for ParseIntValue might be ParseNumberValue

You could well be right.  But one could argue that the class of all of these
things is integer (see nsHTMLValue.h), or one could argue that "number" in many
systems includes floats; more potently, one could also argue that I'd have to go
through and change all the files again :)

> isn't SetPixelValue(val) the same as SetIntValue(val, eHTMLUnit_Pixel)? This
> is in ParseIntWithBounds and ParseIntValue.

Nope, a pixel isn't in the same class as an integer, go figure.  I didn't want
to mess with *that*.

> It's a tad bigger, but since this is used fairly much i think it'd be worth
> it.

I disagree.  The single jez check (or whatever assembly does nowadays) is so
cheap as to not even show up on the radar, and I don't think this is used so
much ... we need to start avoiding overoptimization.

> Also i think EqualsWithConversion/EqualsIgnoreCase are deprecated (or at least
> other methods are prefered to it)

They are deprecated, but changing that would require storing non-char*'s in the
enum tables, something I wasn't prepared to start doing yet.

> couldn't you append directly to aResult? same in eHTMLUnit_Percent

aResult is an nsAString, and somebody thought it would be a great idea not to
have helper functions in nsAString. :(

> That other algorithm might have been faster, and done correctly would avoid
> the copy.

I'd like a separate bug for that; I see where you are going with that (store in
nsDependentSubstring) and I think we need that method to be in the string
classes, because this sort of thing needs to be done in multiple places in the code.

> Don't know if it matters or not, but you might as well do a forward-seach.

welll, if it matters at all, reverse-search will be more efficient because the %
or * is supposed to be at the end of the string.

Comment 9

[John Keiser (jkeiser)]

Attachment: Patch v1.0.1

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 111560 [details] [diff] [review]
Patch v1.0.1

r=sicking with or without rename of ParseIntValue to ParseNumberValue

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 111560 [details] [diff] [review]
Patch v1.0.1

sr=jst

Comment 12

[John Keiser (jkeiser)]

Fix checked in.

Comment 13

[John Keiser (jkeiser)]

Attachment: Booboo Patch

This is cvs diff -r1.105 content/html/content/src/nsHTMLBodyElement.h.

(i.e. what it *should* have been.)  I missed this when I merged.  The change
regressed bug 187744.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsHTMLBodyElement.cpp&root=/cvsroot&subdir=mozilla/content/html/content/src&command=DIFF_FRAMESET&rev1=1.105&rev2=1.106

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112198 [details] [diff] [review]
Booboo Patch

feel free to change the first two to

nsHTMLValue(bgcolor).ToString(aBgColor);

r=sicking

Comment 15

[John Keiser (jkeiser)]

Booboo Patch checked in.

Comment 16

[Brian Ryner (not reading)]

this may have caused bug 190060 (crash on intel.com)