Using some C++11 goodness to cleanup the iterator code in SVGResourceCache.

Using some C++11 goodness to cleanup the iterator code in SVGResourceCache.

BUG=

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

[Savago, 2014-09-04 20:40:28]

a.cavalcanti@samsung.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org

[pdr., 2014-09-04 20:44:56]

On 2014/09/04 20:40:28, Savago wrote:

Not lgtm. We are not yet ready for c++11.

[pdr., 2014-09-05 04:03:50]

pdr@chromium.org changed reviewers:
+ thakis@chromium.org

[pdr., 2014-09-05 04:03:50]

Nico,
On the chromium-dev thread about c++11 features you mentioned that the Android
bots weren't yet smart enough for c++11. I was surprised to see this patch pass
the android compile. If this is still not allowed, is there any protection
against it sneaking in the tree?

[Nico, 2014-09-05 04:08:11]

On 2014/09/05 04:03:50, pdr wrote:
> Nico,
> On the chromium-dev thread about c++11 features you mentioned that the Android
> bots weren't yet smart enough for c++11.

(the aosp host compiler wasn't smart enough. Blink is only built by the target
compiler. aosp is now better, but nacl targets still don't all work yet, so
still don't use c++11 yet.)

> I was surprised to see this patch pass
> the android compile. If this is still not allowed, is there any protection
> against it sneaking in the tree?

The only protection is reviews.

[Savago, 2014-09-05 21:18:47]

While at it, what is the schedule for C++11 in chromium?

[Nico, 2014-09-05 21:20:13]

See the many threads on the topic on chromium-dev. I'll tell the list once
it's done. (Hint: It isn't, yet.)


On Fri, Sep 5, 2014 at 2:18 PM, <a.cavalcanti@samsung.com> wrote:

> While at it, what is the schedule for C++11 in chromium?
>
> https://codereview.chromium.org/543803005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Savago, 2014-09-25 20:31:50]

Good news: auto + for-each are ok on Blink now:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/EgZqphIEkS0

Could someone re-review this patch? It still applies cleanly on top of master.

[Nico, 2014-09-25 21:23:34]

I'm not sure if we should rewrite existing loops in new syntax, it makes more
sense to me to only rewrite loops in functions one touches anyhoo for other
reasons. (but if someone else disagrees and wants to lg this, that's fine with
me)

https://codereview.chromium.org/543803005/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGResourcesCache.cpp (right):

https://codereview.chromium.org/543803005/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGResourcesCache.cpp:185: for (auto& it :
cache->m_cache) {
why "it"?

[Savago, 2014-09-25 21:41:44]

https://codereview.chromium.org/543803005/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGResourcesCache.cpp (right):

https://codereview.chromium.org/543803005/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGResourcesCache.cpp:185: for (auto& it :
cache->m_cache) {
On 2014/09/25 21:23:34, Nico (away until Oct 27) wrote:
> why "it"?

The original code used 'it' (I'm assuming for 'iterator'). Any preferences for
names here?

[Nico, 2014-09-25 21:43:27]

With this patch there's no iterator any more, so "it" ain't right. No
preferences other than that.
On Sep 25, 2014 2:41 PM, <a.cavalcanti@samsung.com> wrote:

>
> https://codereview.chromium.org/543803005/diff/1/Source/
> core/rendering/svg/SVGResourcesCache.cpp
> File Source/core/rendering/svg/SVGResourcesCache.cpp (right):
>
> https://codereview.chromium.org/543803005/diff/1/Source/
> core/rendering/svg/SVGResourcesCache.cpp#newcode185
> Source/core/rendering/svg/SVGResourcesCache.cpp:185: for (auto& it :
> cache->m_cache) {
> On 2014/09/25 21:23:34, Nico (away until Oct 27) wrote:
>
>> why "it"?
>>
>
> The original code used 'it' (I'm assuming for 'iterator'). Any
> preferences for names here?
>
> https://codereview.chromium.org/543803005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Stephen Chennney, 2014-09-26 19:21:51]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2014-09-26 19:21:52]

It may be the android bots are still busted, although it's not clear to me how
this patch would cause the build failures.

And a naming nit.

https://codereview.chromium.org/543803005/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGResourcesCache.cpp (right):

https://codereview.chromium.org/543803005/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGResourcesCache.cpp:186:
i.value->resourceDestroyed(resource);
Blink standard would be to name it something that reflects what it actually is,
in this case objectResources or the like.

[Savago, 2014-09-26 20:46:32]

Ok, just uploaded an updated patch and bots are green (specially the android
ones).

[Stephen Chennney, 2014-09-26 21:09:30]

Let's try it out. LGTM.

[Stephen Chennney, 2014-09-26 21:10:13]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-09-26 21:10:47]

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

[commit-bot: I haz the power, 2014-09-26 21:10:50]

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

[commit-bot: I haz the power, 2014-09-26 21:10:51]

A disapproval has been posted by following reviewers: pdr@chromium.org.
Please make sure to get positive review before another CQ attempt.

[pdr., 2014-09-26 21:14:38]

On 2014/09/26 21:10:51, I haz the power (commit-bot) wrote:
> A disapproval has been posted by following reviewers: mailto:pdr@chromium.org.
> Please make sure to get positive review before another CQ attempt.

LGTM. Now that the first is one done, lets continue doing the others but in
larger chunks. There are roughly 1,000 iterator-based for-loops in Blink :)

[pdr., 2014-09-26 21:14:43]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-09-26 21:16:06]

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

[commit-bot: I haz the power, 2014-09-26 21:58:56]

Committed patchset #3 (id:40001) as 182794