444377 - Color attributes should keep the original value

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently we trim whitespace etc from color attribute values.

Comment 1

[Zack Weinberg (:zwol)]

The latest revision of the patch for bug 488649 will fix this as a side effect.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: patch

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.

Comment 3

[Boris Zbarsky [: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....

Comment 4

[Johnny Stenback (:jst)]

Bug 488649 is fixed now, do we need anything else here, or should we close this bug?

Comment 5

[Boris Zbarsky [:bzbarsky]]

See comment 2.  This bug is alive and well.

Comment 6

[Zack Weinberg (:zwol)]

This fell off the bottom of my todo pile.  I'll try to get around to updating the patch soon.

Comment 7

[Johnny Stenback (:jst)]

Ok, fair enough. I don't think I'd hold 1.9.2 for this though.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: patch v2

(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.

Comment 9

[Boris Zbarsky [: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.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: patch v3 [Checkin: Comment 11]

Both requested changes made.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 394383 [details] [diff] [review]
patch v3
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/9c11bef56fc3

Comment 12

[Zack Weinberg (:zwol)]

Comment on attachment 394383 [details] [diff] [review]
patch v3
[Checkin: Comment 11]

guess this isn't going into 1.9.2 at this point.