[SVG] Refactoring the logic to handle eventBase dependency

[SVG] Refactoring the logic to handle eventBase dependency

Our problem here is we want the dependent element to get notified whenever a change occurs on eventBase.
The current logic to handle this scenario is very inefficient as it is re-connecting the conditions on every loop.
Instead, the same can be handled using SVGDocumentExtensions pattern to improve efficiency.

syncBases and eventBases are functionally different hence separating their code paths to handle them properly.

R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org, fmalita@chromium.org, rob.buis@samsung.com
BUG=323915
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164697

[pdr., 2014-01-06 18:32:24]

At a high level I think this is a good direction. Looking forward to this patch
after you've worked through the remaining issues!

[pavane, 2014-01-06 19:55:19]

Changes Done.

Please have a look at this patch

[pavane, 2014-01-06 19:59:35]

Those layout tests are failing because of neglecting the fact that eventBase can
be other than SVGSMILElement. So, we should not be maintaining the list in
SVGSMILElement. Hence, moved the logic to SVGDocumentExtensions.

[pdr., 2014-01-07 00:25:27]

Looking good. Lets do one more round.

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:199:
document().accessSVGExtensions()->removeAllTargetReferencesForElement(this);
This will also clear the event base conditions we added. It may be a good idea
to refactor this to be called clearResourceAndEventBaseReferences().

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:507:
disconnectSyncBaseConditions();
Why disconnect sync base but not disconnect event base conditions? (here, and
elsewhere)

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:608: if (!eventBase) {
Can you add an assert here that we don't add a pending resource twice?

ASSERT(!document().accessSVGExtensions()->isElementPendingResource(this,
condition.m_baseID));

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:609:
document().accessSVGExtensions()->addPendingResource(condition.m_baseID, this);
Can you assert that m_baseID is not empty?

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:615:
document().accessSVGExtensions()->addElementReferencingTarget(this,
toSVGElement(eventBase));
Here you add an element referencing a target but nowhere is this removed! We
should probably remove this at some point, maybe in
disconnectEventBaseConditions?

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.h (right):

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.h:201: Vector<Condition> m_conditions;
You've done a lot of good work in this patch to separate sync base conditions
and event base conditions, but you're still storing them both in one vector.
Would it be cleaner to separate this into m_syncBaseConditions and
m_eventBaseConditions?

[pavane, 2014-01-07 18:20:58]

Changes done.

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:199:
document().accessSVGExtensions()->removeAllTargetReferencesForElement(this);
On 2014/01/07 00:25:28, pdr wrote:
> This will also clear the event base conditions we added. It may be a good idea
> to refactor this to be called clearResourceAndEventBaseReferences().

Done.

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:507:
disconnectSyncBaseConditions();
On 2014/01/07 00:25:28, pdr wrote:
> Why disconnect sync base but not disconnect event base conditions? (here, and
> elsewhere)

event base functionality is based on adding event listeners and firing the
events at appropriate times. So, only dependency is that eventBase should be
present. For achieving this we use addElementReferencingTarget(). If the
eventBase is removed, it adds itself to pending resources automatically.

whereas sync bases are sensitive to attribute changes also. So, we may have to
disconnect and reconnect on several other conditions

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:608: if (!eventBase) {
On 2014/01/07 00:25:28, pdr wrote:
> Can you add an assert here that we don't add a pending resource twice?
> 
> ASSERT(!document().accessSVGExtensions()->isElementPendingResource(this,
> condition.m_baseID));

Done.

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:609:
document().accessSVGExtensions()->addPendingResource(condition.m_baseID, this);
On 2014/01/07 00:25:28, pdr wrote:
> Can you assert that m_baseID is not empty?

Done.

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.cpp:615:
document().accessSVGExtensions()->addElementReferencingTarget(this,
toSVGElement(eventBase));
On 2014/01/07 00:25:28, pdr wrote:
> Here you add an element referencing a target but nowhere is this removed! We
> should probably remove this at some point, maybe in
> disconnectEventBaseConditions?

I think it is handled already. We are calling disconnectEventBaseConditions only
on element removal. So, removing the references is already handled there by
clearResourceAndEventBaseReferences()

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
File Source/core/svg/animation/SVGSMILElement.h (right):

https://codereview.chromium.org/102353003/diff/200001/Source/core/svg/animati...
Source/core/svg/animation/SVGSMILElement.h:201: Vector<Condition> m_conditions;
On 2014/01/07 00:25:28, pdr wrote:
> You've done a lot of good work in this patch to separate sync base conditions
> and event base conditions, but you're still storing them both in one vector.
> Would it be cleaner to separate this into m_syncBaseConditions and
> m_eventBaseConditions?

There is partial code of accesskey currently. Should i remove it completely? I
feel it is better to put it as a follow up patch.

[pdr., 2014-01-07 18:46:40]

LGTM

[commit-bot: I haz the power, 2014-01-07 18:48:10]

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

[commit-bot: I haz the power, 2014-01-07 18:48:16]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 933.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
359bae831d37bfcc9a222f32ed70d0adb2bf36cb..a53b2e2631c825f6b300b8e100f784ad23cf715d
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -933,9 +933,6 @@ crbug.com/331576 [ Mac ]
fullscreen/parent-flow-inline-with-block-child.html [ P
 
 Bug(esprehn) fast/dom/HTMLMeterElement/meter-element.html [ NeedsRebaseline ]
 
-# This test will pass once we implement SVG animation events using
SVGDocumentExtensions
-crbug.com/323915 svg/animations/repeatn-remove-add-animation.html [ Failure ]
-
 crbug.com/328112 http/tests/inspector/resource-tree/resource-tree-reload.html [
Pass Failure ]
 
 crbug.com/326524
virtual/threaded/compositing/visibility/visibility-simple-video-layer.html [
Pass ImageOnlyFailure ]

[commit-bot: I haz the power, 2014-01-07 18:49:02]

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

[commit-bot: I haz the power, 2014-01-07 18:49:08]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 933.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
359bae831d37bfcc9a222f32ed70d0adb2bf36cb..a53b2e2631c825f6b300b8e100f784ad23cf715d
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -933,9 +933,6 @@ crbug.com/331576 [ Mac ]
fullscreen/parent-flow-inline-with-block-child.html [ P
 
 Bug(esprehn) fast/dom/HTMLMeterElement/meter-element.html [ NeedsRebaseline ]
 
-# This test will pass once we implement SVG animation events using
SVGDocumentExtensions
-crbug.com/323915 svg/animations/repeatn-remove-add-animation.html [ Failure ]
-
 crbug.com/328112 http/tests/inspector/resource-tree/resource-tree-reload.html [
Pass Failure ]
 
 crbug.com/326524
virtual/threaded/compositing/visibility/visibility-simple-video-layer.html [
Pass ImageOnlyFailure ]

[commit-bot: I haz the power, 2014-01-07 19:03:40]

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

[commit-bot: I haz the power, 2014-01-07 20:47:08]

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

[pdr., 2014-01-07 21:34:08]

On 2014/01/07 20:47:08, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

This looks like a legitimate compile failure to me.

[pavane, 2014-01-08 09:45:53]

On 2014/01/07 21:34:08, pdr wrote:
> On 2014/01/07 20:47:08, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_blink for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
> 
> This looks like a legitimate compile failure to me.

Changes Done.

[commit-bot: I haz the power, 2014-01-08 09:46:13]

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

[commit-bot: I haz the power, 2014-01-08 12:32:15]

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

[commit-bot: I haz the power, 2014-01-08 14:03:56]

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

[commit-bot: I haz the power, 2014-01-08 16:40:05]

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

[commit-bot: I haz the power, 2014-01-08 17:02:10]

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

[commit-bot: I haz the power, 2014-01-08 19:21:29]

Change committed as 164697