> On May 23, 2016, 10:14 p.m., Andrea Iacovitti wrote:
> > You are not too paranoid, you are right and the patch looks correct to me
> > and can be shipped!
> > However i think i have finally found a way to simplify this code.
> >
> > We have to distinguish between a single value and a value list in the
> > return value because this function is used as an helper to either parse a
> > specific background property
> > ('background-image:','background-position:',..) or to parse each single
> > value inside a background shorthand property ('background:').
> > When parsing an isolated property it returns a list containing the comma
> > separated values specified by the property, while when parsing a background
> > shorthand it's used in parseBackgroundShorthand() just to parse each single
> > value in the shorthand; those values are then collected in lists (one for
> > each property) built up by parseBackgroundShorthand() itself.
> > In facts, looking at the code, when in shorthand what we basically do is to
> > break from the while loop to simply return the single value just parsed,
> > however having collected it in a list it's necessary to first take it out.
> > So i think the code can be simplified by immediately return the value when
> > in shorthand instead of appending it in a list and detaching it later.
> > Here it is what i mean in term of code https://paste.kde.org/pf26dz1se
> > (patch - tested).
> > What do you think?
I think that's a much better fix (I've built and tested as well) and should
also finally resolve the Coverity complaints. I'd be quite happy if you could
commit and close this RR. :)
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127835/#review95730
-----------------------------------------------------------
On May 22, 2016, 11:53 p.m., Michael Pyne wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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
> -----
>
> 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
