Closed
Bug 173767
Opened 22 years ago
Closed 22 years ago
[FIXr]setProperty("foo", "bar", "important") broken for inline style
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.3alpha
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
()
Details
(Keywords: regression, testcase)
Attachments
(2 files, 1 obsolete file)
2.42 KB,
patch
|
caillon
:
review+
|
Details | Diff | Splinter Review |
558 bytes,
text/html
|
Details |
This was broken by the checkin for bug 170895. ParsePropertyValue() does not handle priorities (since they're not part of the property value anyway).
Assignee | ||
Updated•22 years ago
|
Blocks: 173772
Keywords: regression
Summary: setPropertyValue("foo", "bar", "important") broken for inline style → [FIX]setPropertyValue("foo", "bar", "important") broken for inline style
Assignee | ||
Comment 1•22 years ago
|
||
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?)).
Attachment #102479 -
Flags: superreview+
Assignee | ||
Comment 3•22 years ago
|
||
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...
Assignee | ||
Comment 4•22 years ago
|
||
Actually, the code size savings would likely be pretty minimal in this case, no?
Comment 5•22 years ago
|
||
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.
Attachment #102479 -
Flags: review+
Assignee | ||
Comment 6•22 years ago
|
||
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•22 years ago
|
||
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!
Whiteboard: [dev notes]
Assignee | ||
Comment 8•22 years ago
|
||
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.
Updated•22 years ago
|
Whiteboard: [dev notes]
Assignee | ||
Comment 10•22 years ago
|
||
caillon, could you review?
Attachment #102479 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla1.3alpha
Comment 11•22 years ago
|
||
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.
Attachment #104352 -
Flags: review+
Comment 12•22 years ago
|
||
*** Bug 177115 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•22 years ago
|
Summary: [FIX]setPropertyValue("foo", "bar", "important") broken for inline style → [FIXr]setPropertyValue("foo", "bar", "important") broken for inline style
Assignee | ||
Comment 14•22 years ago
|
||
fixed for 1.3a
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Assignee | ||
Comment 16•22 years ago
|
||
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.
Assignee | ||
Comment 17•22 years ago
|
||
Bug 178668 filed
Comment 18•21 years ago
|
||
verified with the testcase provided in the url
Status: RESOLVED → VERIFIED
Comment 19•21 years ago
|
||
I think this bug's summary (and bug 173772's) should refer to "setProperty" rather than "setPropertyValue".
Assignee | ||
Comment 20•21 years ago
|
||
Yeah. Stupid naming asymmetry.
Summary: [FIXr]setPropertyValue("foo", "bar", "important") broken for inline style → [FIXr]setProperty("foo", "bar", "important") broken for inline style
You need to log in
before you can comment on or make changes to this bug.
Description
•