HTMLValue parsing methods belong in HTMLValue

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
DOM: Core & HTML
P3
normal
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: John Keiser (jkeiser), Assigned: John Keiser (jkeiser))

Tracking

Trunk
mozilla1.4alpha
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FIX])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
This does not include the HTML specific ones :)
Status: NEW → ASSIGNED
(Assignee)

Updated

15 years ago
Priority: -- → P4
Target Milestone: --- → mozilla1.3beta
(Assignee)

Updated

15 years ago
Priority: P4 → --
Target Milestone: mozilla1.3beta → ---
(Assignee)

Comment 2

15 years ago
Created attachment 109632 [details] [diff] [review]
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
(Assignee)

Updated

15 years ago
Attachment #109632 - Flags: review?(bugmail)
(Assignee)

Comment 3

15 years ago
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.
Priority: -- → P3
Whiteboard: [FIX]
Target Milestone: --- → mozilla1.4alpha
(Assignee)

Comment 4

15 years ago
Created attachment 109635 [details]
Testcase

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

Comment 5

15 years ago
Created attachment 109636 [details]
Testcase (strict)

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

Comment 6

15 years ago
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*".
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!
(Assignee)

Comment 8

15 years ago
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.
(Assignee)

Comment 9

15 years ago
Created attachment 111560 [details] [diff] [review]
Patch v1.0.1
Attachment #109632 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #109632 - Flags: review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #111560 - Flags: review?(bugmail)
Comment on attachment 111560 [details] [diff] [review]
Patch v1.0.1

r=sicking with or without rename of ParseIntValue to ParseNumberValue
Attachment #111560 - Flags: review?(bugmail) → review+
(Assignee)

Updated

15 years ago
Attachment #111560 - Flags: superreview?(jst)
Comment on attachment 111560 [details] [diff] [review]
Patch v1.0.1

sr=jst
Attachment #111560 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 12

15 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

15 years ago
Created attachment 112198 [details] [diff] [review]
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
(Assignee)

Updated

15 years ago
Attachment #112198 - Flags: superreview?(bzbarsky)
Attachment #112198 - Flags: review?(bugmail)
Comment on attachment 112198 [details] [diff] [review]
Booboo Patch

feel free to change the first two to

nsHTMLValue(bgcolor).ToString(aBgColor);

r=sicking
Attachment #112198 - Flags: review?(bugmail) → review+
Attachment #112198 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 15

15 years ago
Booboo Patch checked in.
this may have caused bug 190060 (crash on intel.com)

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.