173767 - [FIXr]setProperty("foo", "bar", "important") broken for inline style

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

This was broken by the checkin for bug 170895.  ParsePropertyValue() does not
handle priorities (since they're not part of the property value anyway).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: patch. Thought I'd attached this...

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 102479 [details] [diff] [review]
patch.  Thought I'd attached this...

sr=dbaron, although it might be nice to do this a faster way...

You might get a code size savings if that tearoff Set##method_ called another
function (like what I did for the old nsComputedDOMStyle code (or was it the
other one?)).

Comment 3

[Boris Zbarsky [:bzbarsky]]

It was this one, actually.  That was also removed as part of bug 170895 and it
should not be hard to reinstate...

Going forward, we may want to change ParsePropertyValue to either allow
!important or not depending on caller preference and then it could handle both
cases, I guess... 

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actually, the code size savings would likely be pretty minimal in this case, no?

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 102479 [details] [diff] [review]
patch.  Thought I'd attached this...

The code size savings was in nsDOMCSSDeclaration.  Just removing that alone in
favor of what I did gained me about 8k in footprint, which is fairly
significant all things considered.  It would be nice to see just how much code
you add back, and if it's more than 8k (maybe even less?), you may want to
consider trying the old way...

r=caillon in the case this doesn't hurt us too badly.  Good catch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm... So the numbers (-O1 gcc 2.91 build on Linux, stripped libgkcontent.so) are:

Without my patch: 3,692,624 bytes
With my patch: 3,699,376 bytes
With my patch but without the nsAutoString-to-PRUnichar* change: 3,692,816 bytes

Since that part was supposed to be a perf fix, not a correctness fix, we could
just ditch it... Or replace nsAutoString() with NS_LITERAL_STRING(""), maybe?

I can also test movingthat PRUnichar* trickery into a helper function if people
think it's worth it.... That would sorta negate the perf win, no?

Comment 7

[Madhur Bhatia]

Boris - it would be nice if you could attach a testcase to this bug, so that QA
can see the impact or effect of this code change.
Thx!

Comment 8

[Boris Zbarsky [:bzbarsky]]

There's a testcase at the URL... it uses some JS and CSS common to a bunch of
other testcases, but I suppose I could try to inline those...  Let me know if
that's really necessary.

Comment 9

[Madhur Bhatia]

thx Boris.... this is good enough. 
Adding KW : testcase

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Doesn't regress the size.

caillon, could you review?

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 104352 [details] [diff] [review]
Doesn't regress the size.

>Index: content/html/style/src/nsCSSParser.cpp

>+//XXXbz this function does not deal well with something like "foo
>+//!important" as the aPropValue.  It will parse the "foo" and set it
>+//in the decl, then ignore the !important.  It should either fail to
>+//parse this or do !important correctly....

Wanna add spaces after the // on each line?


>Index: content/html/style/src/nsDOMCSSDeclaration.cpp

>+  // ParsePropertyValue does not handle priorities correctly -- it's
>+  // optimized for speed.  And the priority is not part of the
>+  // property value anyway.... So we have to use the full-blown
>+  // ParseDeclaration()

I'd nix 'correctly' on the first line; it really does not (explicitly) handle
them incorrectly either, as the statement implies.  It simply does not handle
them at all, which was an oversight on my part....

Also a slight inconsistency:  'ParseDeclaration()' has parentheses whereas
'ParsePropertyValue' has none.


r=caillon.  Thanks for keeping the size down.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 177115 has been marked as a duplicate of this bug. ***

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: test case

Comment 14

[Boris Zbarsky [:bzbarsky]]

fixed for 1.3a

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Boris, it's not fully fixed. The testcase I attached stays red because the
change of priority does not cause a repaint. Feel free to reopen or spin off.

Comment 16

[Boris Zbarsky [:bzbarsky]]

That'll be fixed by bug 171830 for inline style.  But yes, it's an underlying
problem in the parser.  It only happens when you set a property to the _same_
value but different importance -- you get a hint of 0.  That is not a
regression, unlike this bug....

I'm not sure whether we want to fix that independently of bug 171830 since it
also affects non-inline style consumers...  I'll spin off a just-in-case bug
this afternoon.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Bug 178668  filed

Comment 18

[Madhur Bhatia]

verified with the testcase provided in the url 

Comment 19

[Jesse Ruderman]

I think this bug's summary (and bug 173772's) should refer to "setProperty"
rather than "setPropertyValue".

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yeah.  Stupid naming asymmetry.