Closed Bug 444377 Opened 12 years ago Closed 11 years ago

Color attributes should keep the original value

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.1 --- wontfix

People

(Reporter: smaug, Assigned: zwol)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently we trim whitespace etc from color attribute values.
Component: DOM: HTML → DOM: Core & HTML
Flags: blocking1.9.2?
The latest revision of the patch for bug 488649 will fix this as a side effect.
Depends on: 488649
Attached patch patch (obsolete) — Splinter Review
For 1.9.1 we want a minimal patch for bug 488649, so here's the additional change required to fix *this* bug as a separate patch.  Still requires the changes currently being contemplated in 488649, though.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #378487 - Flags: review?(bzbarsky)
Comment on attachment 378487 [details] [diff] [review]
patch

>+++ b/content/base/src/nsAttrValue.cpp
>@@ -385,17 +385,8 @@ nsAttrValue::ToString(nsAString& aResult
>     }
>     case eColor:
>     {
>+      NS_NOTREACHED("color attribute without string data");
>+      aResult.Truncate();
>       break;
>     }

Just make this whole case #ifdef DEBUG so that we fall through to default: in opt code?

> nsAttrValue::GetColorValue(nscolor& aColor) const
> {
>-  NS_PRECONDITION(Type() == eColor || Type() == eString, "wrong type");

Why remove that?

The switch in this method can just become:

  if (Type() != eColor) {
    // Unparseable value
    return PR_FALSE;
  }
  aColor = GetMiscContainer()->mColor;
  return PR_TRUE;

right?
  
>+      goto success;

I'd prefer a structure like:

  do {
    if (something) break;
    if (something-else) break;

    fallback stuff;
  } while(0);
  // success

It's basically the same, but plays nicer with stack-local things that have constructors, iirc.

>+    // Save the literal string we were passed for round-tripping.
>+    nsStringBuffer* buf = GetStringBuffer(aString);
>+    cont->mStringBits = reinterpret_cast<PtrBits>(buf) | eStringBase;

That assumes we won't get OOM in GetStringBuffer().  I'd rather not assume that, honestly; maybe just Reset() if that happens?

Should you also adjust nsAttrValue::Equals() to not compare equal for different strings that parse to the same color?  It should be pretty easy to write tests where that will break your roundtripping...  Something like <font color="#ffffff"></font><font color="#fff"></font> should produce equal values for getAttribute("color") with your patch (not sure which of the two it'll end up with).

It'd be interesting to get some data on how often real web pages hit that case (colors equal but strings not).  If it's rare, we should fix Equals.  If it's very common, maybe not....
Attachment #378487 - Flags: review?(bzbarsky) → review-
Bug 488649 is fixed now, do we need anything else here, or should we close this bug?
See comment 2.  This bug is alive and well.
This fell off the bottom of my todo pile.  I'll try to get around to updating the patch soon.
Ok, fair enough. I don't think I'd hold 1.9.2 for this though.
Flags: wanted-next+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #3)
> (From update of attachment 378487 [details] [diff] [review])
> >     case eColor:
> >     {
> >+      NS_NOTREACHED("color attribute without string data");
> >+      aResult.Truncate();
> 
> Just make this whole case #ifdef DEBUG so that we fall through to default: in
> opt code?

Done.

> >-  NS_PRECONDITION(Type() == eColor || Type() == eString, "wrong type");
> 
> Why remove that?

Redundant with the NS_NOTREACHED in the default case of the switch.

> The switch in this method can just become:
> 
>   if (Type() != eColor) {
>     // Unparseable value
>     return PR_FALSE;
>   }
>   aColor = GetMiscContainer()->mColor;
>   return PR_TRUE;

Done (structured slightly differently, though).

> >+      goto success;
> 
> I'd prefer a structure like:
> 
>   do {
>     if (something) break;
>     if (something-else) break;

I'm not really a fan of this kind of fake loop.  Instead I made a new SetColorValue() private method and call it from the three places that used to wind up at the success: label.  This reads easier than both options IMO.

> >+    // Save the literal string we were passed for round-tripping.
> >+    nsStringBuffer* buf = GetStringBuffer(aString);
> >+    cont->mStringBits = reinterpret_cast<PtrBits>(buf) | eStringBase;
> 
> That assumes we won't get OOM in GetStringBuffer().  I'd rather not assume
> that, honestly; maybe just Reset() if that happens?

Done.

> Should you also adjust nsAttrValue::Equals() to not compare equal for
> different strings that parse to the same color?

I have no idea whether this kind of thing is common on the web, but it was easy to make that change in Equals() and consistent with other parsed-but-roundtripped string values, so I did it.
Attachment #378487 - Attachment is obsolete: true
Attachment #390339 - Flags: review?(bzbarsky)
Comment on attachment 390339 [details] [diff] [review]
patch v2

> Done (structured slightly differently, though).

I'd really prefer no else-after-return; I don't much care which branch is which, though.

r=bzbarsky with that and a test that exercises the string-comparison code in Equals() for colors.
Attachment #390339 - Flags: review?(bzbarsky) → review+
Both requested changes made.
Attachment #390339 - Attachment is obsolete: true
Attachment #394383 - Flags: review+
Attachment #394383 - Flags: approval1.9.2?
Flags: wanted1.9.2?
Keywords: checkin-needed
Comment on attachment 394383 [details] [diff] [review]
patch v3
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/9c11bef56fc3
Attachment #394383 - Attachment description: patch v3 → patch v3 [Checkin: Comment 11]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 394383 [details] [diff] [review]
patch v3
[Checkin: Comment 11]

guess this isn't going into 1.9.2 at this point.
Attachment #394383 - Flags: approval1.9.2?
Flags: wanted1.9.2?
You need to log in before you can comment on or make changes to this bug.