-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127835/
-----------------------------------------------------------
(Updated May 22, 2016, 11:53 p.m.)
Review request for KDE Frameworks, Andrea Iacovitti and Bernd Buschinski.
Changes
-------
Latest attempt, which passes the testcase on Bug 363378 and maintains the
previous Acid2 and Acid3 results. However the MDN CSS background-property page
no longer renders properly, though this seems to be due to an unrelated change.
This change still downgrades single-element lists to a single element result.
I'm uncertain of the proper memory handling for this, if we don't return the
original list then we should free it once we've extracted our item, but if we
do that then the list will deref() the item (and I'm assuming this will lead to
its deletion). To avert this I manually mess with the ref counts but maybe this
is too paranoid?
Bugs: 363378
https://bugs.kde.org/show_bug.cgi?id=363378
Repository: khtml
Description
-------
In KHTML commit b52325eb49 I attempted to fix a Coverity error (CID 257928)
indicating that the CSS background property parser could sometimes leak
`CSSValueImpl` objects.
Coverity shows that the leak is still possible (and I think it's correct, even
with my change, in situations where the first return value is upgraded to a
list while the second return value remains unset).
Because of that I'm trying a different approach to fix the leak by factoring
out the code that promotes a saved return value into a value-list when needed,
in the hopes that simpler code will be more correct.
Note for reviewers that, as far as I can see, the idea in the current code is
that there's 3 levels of values, for 2 separate return values:
1. "currValue", the value found during this pass through the main loop
2. "value", the saved return value when it's still only a single value
(assigned from currValue)
3. "values", the saved return value when it's now a list of values (assigned
from value and/or currValue)
Similar concepts apply to currValue2/value2/values2. I am unsure why there's a
distinction between a single value and a value list in the return value -- it
may be that this distinction is unneeded.
Diffs (updated)
-----
src/css/cssparser.cpp a325c60
Diff: https://git.reviewboard.kde.org/r/127835/diff/
Testing
-------
The updated code compiles without warnings in cssparser.cpp, and runs fine in
Konqueror.
To test I tried going to the MDN site on the CSS background properties
(including background-position in particular, since that's the only CSS
property that requires the second value/return value). I then used the DOM
Inspector tool to manually verify that the CSS background properties were
properly read, that the examples rendered as before, etc.
The Acid3 test makes it to 92/100, but I'm pretty sure it was only at 92/100
before this change as well. ;)
Thanks,
Michael Pyne
_______________________________________________
Kde-frameworks-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
