[SVG] Avoid unnecessarily populating hashmap for repeat(n) event

[SVG] Avoid unnecessarily populating hashmap for repeat(n) event

This is a follow up patch for repeat(n) animation event support.
Instead of maintaining each repeat(n) in the atomic string hashmap,
we just maintain 'repeatn' in the hashmap and store the repeatCount data with the event.

R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org, fmalita@chromium.org
BUG=

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

[pavane, 2013-09-13 17:42:44]

WIP but a working patch. I suppose no extra test cases are required.

pdr, i had to add back some code which i removed in my last patch. I guess the
FIXME there is talking about the same as you suggested on IRC. May be this is
the indication that we are moving in right direction. :)

In this patch, i tweaked Event class to make things work. My question is do we
need a separate class just for handling one event? If yes, what can be the class
name? (RepeatEvent ?)

FIXME is suggesting the name TimeEvent but it is not making sense to me.

Need your feedback.

Thanks in advance,
Pavan

[pdr., 2013-09-13 17:47:02]

On 2013/09/13 17:42:44, pavane wrote:
> WIP but a working patch. I suppose no extra test cases are required.
> 
> pdr, i had to add back some code which i removed in my last patch. I guess the
> FIXME there is talking about the same as you suggested on IRC. May be this is
> the indication that we are moving in right direction. :)

I think so too.

> In this patch, i tweaked Event class to make things work. My question is do we
> need a separate class just for handling one event? If yes, what can be the
class
> name? (RepeatEvent ?)
> 
> FIXME is suggesting the name TimeEvent but it is not making sense to me.

You are now the expert on SMIL events so don't worry too much about old FIXME's
:)

RepeatEvent sounds good to me. Can you make it an inner class?

[pavane, 2013-09-17 08:47:49]

Please have a look at this patch. I implemented RepeatEvent as a sub class of
Event.

pdr, i think C++ doesn't allow a derived class as a inner class. of course, we
can tweak it by delaying the definition of inner class but looks like compiler
is still unhappy about it. Do you think such a pattern is used anywhere? Want to
know the advantages of using such a pattern over a sub class. As i understand,
our problem here is we want to avoid extra memory usage for non-repeat events.
Please let me know if you have a better idea.

[pdr., 2013-09-17 22:40:01]

https://codereview.chromium.org/23944012/diff/10001/Source/core/dom/Event.h
File Source/core/dom/Event.h (right):

https://codereview.chromium.org/23944012/diff/10001/Source/core/dom/Event.h#n...
Source/core/dom/Event.h:182: virtual int repeat() const { return -1; }
I don't think this is necessary and I'd prefer to not bloat all events just for
this feature.

Instead, can you just implement repeat() on the derived class and use m_type to
determine whether you can static_cast to RepeatEvent? AtomicString comparisons
are super cheap.

https://codereview.chromium.org/23944012/diff/10001/Source/core/dom/Event.h#n...
Source/core/dom/Event.h:211: class RepeatEvent : public Event {
Can this be placed in SVGSMILElement?

[pavane, 2013-09-18 07:30:49]

Changes Done. Please have a look.

[pdr., 2013-09-18 18:03:44]

On 2013/09/18 07:30:49, pavane wrote:
> Changes Done. Please have a look.

Great work, I like the toRepeatEvent :)

LGTM

[commit-bot: I haz the power, 2013-09-18 18:40:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavan.e@samsung.com/23944012/15001

[commit-bot: I haz the power, 2013-09-18 21:13:11]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-09-18 22:11:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavan.e@samsung.com/23944012/15001

[commit-bot: I haz the power, 2013-09-18 23:19:58]

Change committed as 157988