Fixed traversal bug in CSSValueList::removeAll

Fixed traversal bug in CSSValueList::removeAll

Fixed a small bug in CSSValueList::removeAll where every element after a
removed one would previously be skipped. With this patch, removeAll()
checks all elements correctly.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197699

[sashab, 2015-06-19 18:42:37]

sashab@chromium.org changed reviewers:
+ dstockwell@chromium.org, timloh@chromium.org

[Timothy Loh, 2015-06-22 01:36:27]

On 2015/06/19 18:42:37, sashab wrote:

lgtm but the comment seems weird to me, since we could remove as we go
traversing forwards if we just decremented index after removing.

Nit: I prefer reverse loops like "for (int i = x; i--; ) { .. }", but not sure
how others feel about it...

[Timothy Loh, 2015-06-22 04:07:49]

BTW this is one of the very few places where CSSValues can be mutated (excluding
builder calls like CSSValueList::append), it might make sense to try and get rid
of this :/

[sashab, 2015-06-24 01:03:07]

Sure, happy to look at removing it separately. It seems to be called from
editing code:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

What would be an alternative to this function? Delete the old CSSValueList and
make a new one with all the elements removed?

[sashab, 2015-06-24 01:03:10]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-06-24 01:03:11]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1200473002/#ps20001
(title: "Review feedback")

[sashab, 2015-06-24 01:03:17]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2015-06-24 01:03:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1200473002/20001

[commit-bot: I haz the power, 2015-06-24 01:13:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 01:13:11]

Exceeded global retry quota

[sashab, 2015-06-24 02:49:19]

Hmm.. So this code doesn't *quite* work because index is an unsigned variable.
Some suggestions on stack overflow:
http://stackoverflow.com/questions/3623263/reverse-iteration-with-an-unsigned...

Some options:
for (int index = m_values.size() - 1; index != -1 ; --index) { ... }
for (size_t index = m_values.size(); index-- > 0; )
for (size_t index = m_values.size() - 1; index < m_values.size() ; --index)

Personally I think they're all pretty hard to read, maybe the best option is to
loop through index but use index-1 inside the loop. Thoughts?

[sashab, 2015-06-24 03:01:55]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-06-24 03:01:56]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1200473002/#ps40001
(title: "Changed size_t to int")

[commit-bot: I haz the power, 2015-06-24 03:02:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1200473002/40001

[commit-bot: I haz the power, 2015-06-24 04:22:13]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197699