<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127835/">https://git.reviewboard.kde.org/r/127835/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 23rd, 2016, 10:14 p.m. UTC, <b>Andrea Iacovitti</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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. :)</pre>
<br />
<p>- Michael</p>
<br />
<p>On May 22nd, 2016, 11:53 p.m. UTC, Michael Pyne wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, Andrea Iacovitti and Bernd Buschinski.</div>
<div>By Michael Pyne.</div>
<p style="color: grey;"><i>Updated May 22, 2016, 11:53 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=363378">363378</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
khtml
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In KHTML commit b52325eb49 I attempted to fix a Coverity error (CID 257928) indicating that the CSS background property parser could sometimes leak <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CSSValueImpl</code> objects.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">"currValue", the value found during this pass through the main loop</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">"value", the saved return value when it's still only a single value (assigned from currValue)</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">"values", the saved return value when it's now a list of values (assigned from value and/or currValue)</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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. ;)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/css/cssparser.cpp <span style="color: grey">(a325c60)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127835/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>