Tracking filter mutation via SVGElementProxy

Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}

[fs, 2016-10-10 08:11:15]

Description was changed from

==========
Tracking filter mutation via SVGElementProxy

Remove ReferenceFilterBuilder.
Don't keep PaintLayerFilterInfo around when no filter will be painted.
Drop DocumentResourceReference.

SVGElementProxy:

SVGElementProxy as ResourceDocumentClient.
Make the DocumentResource handling a detail internal to SVGElementProxy.

BUG=439970
==========

to

==========
Tracking filter mutation via SVGElementProxy

Remove ReferenceFilterBuilder.
Don't keep PaintLayerFilterInfo around when no filter will be painted.
Drop DocumentResourceReference.

SVGElementProxy:

SVGElementProxy as ResourceDocumentClient.
Make the DocumentResource handling a detail internal to SVGElementProxy.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-10-10 08:11:21]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 08:11:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 08:13:41]

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

[commit-bot: I haz the power, 2016-10-10 08:13:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2016-10-10 10:44:20]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 10:44:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 11:24:52]

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

[commit-bot: I haz the power, 2016-10-10 11:24:52]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[fs, 2016-10-11 09:25:58]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 09:26:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 09:36:47]

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

[commit-bot: I haz the power, 2016-10-11 09:36:48]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fs, 2016-10-11 10:31:58]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 10:32:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 11:56:06]

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

[commit-bot: I haz the power, 2016-10-11 11:56:07]

Dry run: This issue passed the CQ dry run.

[fs, 2016-10-11 12:30:11]

Description was changed from

==========
Tracking filter mutation via SVGElementProxy

Remove ReferenceFilterBuilder.
Don't keep PaintLayerFilterInfo around when no filter will be painted.
Drop DocumentResourceReference.

SVGElementProxy:

SVGElementProxy as ResourceDocumentClient.
Make the DocumentResource handling a detail internal to SVGElementProxy.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue.
Clients are SVGResourceClients, like before, with methods/callbacks
renamed.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-10-11 12:34:11]

Description was changed from

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue.
Clients are SVGResourceClients, like before, with methods/callbacks
renamed.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue.
Clients are SVGResourceClients, like before, with methods/callbacks
renamed.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.) In the latter case, the generation of the proxy
is bumped. This generation (id) is then used to check for changes in the
element reference. This unfortunately still require caching to be
disabled for these ComputedStyle objects.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-10-11 13:26:51]

Description was changed from

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue.
Clients are SVGResourceClients, like before, with methods/callbacks
renamed.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.) In the latter case, the generation of the proxy
is bumped. This generation (id) is then used to check for changes in the
element reference. This unfortunately still require caching to be
disabled for these ComputedStyle objects.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, in
a somewhat transparent fashion, if requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.) In the latter case, the generation of the proxy
is bumped. This generation (id) is then used to check for changes in the
element reference. This unfortunately still require caching to be
disabled for these ComputedStyle objects.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-10-18 14:52:24]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[fs, 2016-10-18 14:52:35]

fs@opera.com changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[fs, 2016-10-18 14:52:36]

I think I've noodled over this long enough, and it seems to settling as to what
"smarts" I manage to come up with... so time to hit the fan.

I've tried to shrink this to the bare essentials only, but it's still fairly
large... I left out the removal of the "hack" code, but it should be dead with
this change (i.e not called.) Will remove that if and when this has landed and
stuck.

Feel free to add additional reviewers as you see fit, I didn't think it'd be
wise to have a too long list of reviewers given the size of this change.

[commit-bot: I haz the power, 2016-10-18 14:52:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 16:08:34]

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

[commit-bot: I haz the power, 2016-10-18 16:08:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2016-10-20 03:11:05]

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
(right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:118: if
(!computedStyle->hasFilter())
Looking at the comment about images in loadPendingImages, we can request a bunch
of images but only end up needing one. For example:
<style>
   div { background-image: url(a.png); }
   div { background-image: url(b.png); }
   div { background-image: none; }
</style>
<div></div>

My understanding of the new code is that we'd create a bunch of
SVGElementProxies but only resolve the used ones?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127:
referenceOperation.elementProxy().resolve(*m_document);
If the same url is requested multiple times by different references, will we
create a request for each?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:136:
SVGElementProxy* elementProxy =
Is the CSSURIValue lifetime guaranteed to last as long as the filter operations?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h
(right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h:89: void
invalidateProxyClients(InvalidationMode);
Is InvalidationMode needed (yet)?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/FilterOperation.h (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/FilterOperation.h:164: String m_url;
Is this url still needed or can we rely on m_elementProxy which also has the url
(temporarily)?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:125: for
(SVGElementProxy* proxy : m_elementProxies)
Should we save off the proxies, clear them, then issue reference change
notifications? This would protect against the case that a new proxy is
synchronously added during this loop.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:57: // tracker.
This also seems to be called from notifyFinished

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter)
incremented with each change to the proxy reference.
Are there any security issues here if a mean user spams proxies until this
overflows?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:73: void
notifyFinished(Resource*) override;
Can you add a comment here about how this is called when the referenced document
loads?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:76: void nextGeneration() {
m_generation++; }
Bikeshed nit: incrementCacheGeneration?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:82: String m_url;
Can you add a comment about how this will be cleared after the request
completes?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:90: class
SVGElementProxyTracker : public GarbageCollected<SVGElementProxyTracker> {
This name confused me for a bit because I thought it was more than just a
set+helper. WDYT of renaming to just SVGElementProxySet?

(further suggestions below, none required).

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:92: void
attachElementProxy(SVGElementProxy&);
Maybe just "add(SVGElementProxy&)"?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:97: void
invalidateProxyClients();
Maybe just "notifyThatContentChanged"?

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGResourceClient.h (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGResourceClient.h:20: virtual void
resourceReferenceChanged(SVGElementProxy*) = 0;
Are the SVGElementProxy* arguments needed? Would it be safe to use them since
the proxy could be freed when this is called?

[fs, 2016-10-20 11:27:25]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 11:27:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-10-20 11:28:00]

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
(right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:118: if
(!computedStyle->hasFilter())
On 2016/10/20 at 03:11:04, pdr. wrote:
> Looking at the comment about images in loadPendingImages, we can request a
bunch of images but only end up needing one. For example:
> <style>
>    div { background-image: url(a.png); }
>    div { background-image: url(b.png); }
>    div { background-image: none; }
> </style>
> <div></div>
> 
> My understanding of the new code is that we'd create a bunch of
SVGElementProxies but only resolve the used ones?

That is correct, yes. The option would be to create some form of placeholder
(like StylePendingImage) and return that, but seeing that an (unresolved) proxy
isn't too heavy I've opted against doing that.

(It might also be worth noting that a CSSURIValue will only create one proxy and
cache it, so createElementProxy will always return the same object. (For now,
see first parenthesis below.)

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127:
referenceOperation.elementProxy().resolve(*m_document);
On 2016/10/20 at 03:11:04, pdr. wrote:
> If the same url is requested multiple times by different references, will we
create a request for each?

No. After the proxy is resolved the first time, this is a no-op. (Barring a fix
for crbug.com/405315, which would require a reset of that state.) If multiple
CSSURIValues reference the same resource (non-local document), the MemoryCache
would provide the same Resource. (Excepting fetching restrictions which prevent
re-use.)

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:136:
SVGElementProxy* elementProxy =
On 2016/10/20 at 03:11:05, pdr. wrote:
> Is the CSSURIValue lifetime guaranteed to last as long as the filter
operations?

Both have strong references, so technically speaking the lifetime of the
CSSURIValue is irrelevant. (But in most, if not all, cases it is likely to
outlive the latter.)

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h
(right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h:89: void
invalidateProxyClients(InvalidationMode);
On 2016/10/20 at 03:11:05, pdr. wrote:
> Is InvalidationMode needed (yet)?

No. Dropped (for now.)

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/FilterOperation.h (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/FilterOperation.h:164: String m_url;
On 2016/10/20 at 03:11:05, pdr. wrote:
> Is this url still needed or can we rely on m_elementProxy which also has the
url (temporarily)?

This is the URL we need for getComputedStyle. It's the unresolved/relative URL,
while the one that is (temporarily) in m_elementProxy is a resolved/absolute
URL. (This is another thing that will likely change when crbug.com/405315 is
fixed.)

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:125: for
(SVGElementProxy* proxy : m_elementProxies)
On 2016/10/20 at 03:11:05, pdr. wrote:
> Should we save off the proxies, clear them, then issue reference change
notifications? This would protect against the case that a new proxy is
synchronously added during this loop.

Sure. Reworked.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:57: // tracker.
On 2016/10/20 at 03:11:05, pdr. wrote:
> This also seems to be called from notifyFinished

That's kind of an implementation detail that ought to be transparent to users of
this interface. Technically speaking it can also be considered covered by "the
id of an element changed". I'll add it to the list of events though.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter)
incremented with each change to the proxy reference.
On 2016/10/20 at 03:11:05, pdr. wrote:
> Are there any security issues here if a mean user spams proxies until this
overflows?

What you can achieve by that is to make two operations compare equal when they
should not. That would make the client(s) reference the wrong proxy/element (==
failed invalidations), but nothing more severe that that I can think of.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:73: void
notifyFinished(Resource*) override;
On 2016/10/20 at 03:11:05, pdr. wrote:
> Can you add a comment here about how this is called when the referenced
document loads?

Done.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:76: void nextGeneration() {
m_generation++; }
On 2016/10/20 at 03:11:05, pdr. wrote:
> Bikeshed nit: incrementCacheGeneration?

Made it incrementGeneration (there's nothing "cache" here really.)

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:82: String m_url;
On 2016/10/20 at 03:11:05, pdr. wrote:
> Can you add a comment about how this will be cleared after the request
completes?

Done.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:90: class
SVGElementProxyTracker : public GarbageCollected<SVGElementProxyTracker> {
On 2016/10/20 at 03:11:05, pdr. wrote:
> This name confused me for a bit because I thought it was more than just a
set+helper. WDYT of renaming to just SVGElementProxySet?
> 
> (further suggestions below, none required).

Renamed to SVGElementProxySet.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:92: void
attachElementProxy(SVGElementProxy&);
On 2016/10/20 at 03:11:05, pdr. wrote:
> Maybe just "add(SVGElementProxy&)"?

Done.

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:97: void
invalidateProxyClients();
On 2016/10/20 at 03:11:05, pdr. wrote:
> Maybe just "notifyThatContentChanged"?

Done (minus "That".)

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGResourceClient.h (right):

https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGResourceClient.h:20: virtual void
resourceReferenceChanged(SVGElementProxy*) = 0;
On 2016/10/20 at 03:11:05, pdr. wrote:
> Are the SVGElementProxy* arguments needed? Would it be safe to use them since
the proxy could be freed when this is called?

I guess they aren't strictly needed (yet. Will be if/when a client is registered
- and cares about - multiple proxies.)

I can drop them for now.

They are safe because the reference is GCd and on the stack.

[commit-bot: I haz the power, 2016-10-20 13:18:36]

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

[commit-bot: I haz the power, 2016-10-20 13:18:37]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-10-20 19:03:42]

Very nice! A couple more requests for comments to help future readers.

I think this is ready to go but would like someone more familiar with CSS
loading/resources/etc to review before landing. I'd recommend alancutter or
esprehn who touched similar code in ElementStyleResources.

https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:20: // A proxy to an
SVGElement. Allows access to an element with a certain 'id',
Do you mind expanding this comment to describe the overall design?

These are the notes I took while reviewing. Maybe something along these lines?
* SVGElements can be referenced using CSS (filter: url(foo.com/bar.svg#baz;)
which may require loading external resources. SVGElementProxy is a persistent
reference to an SVGElement both before and after the SVGElement has been found.
* Filtered content has |CSSURIValue|s which create |SVGElementProxy|s which also
maintain references back to the filtered content via SVGResourceClient.
* SVGElementProxy looks up SVGElements by id (for remote resources, it will
request the SVGDocument and lookup by id there).
* When SVGElementProxy finds an SVGElement, it tells the SVGElement which in
turn keeps a list of |SVGElementProxy|s referencing it.
* These bi-directional references (filtered content <-> SVGElementProxy and
SVGElementProxy <-> SVGFilterElement) are used for invalidation, change
notifications, etc.

https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter)
incremented with each change to the proxy reference.
Nit: "reference" was a little confusing to me when reviewing this because the
proxy both references and is referenced. I don't have a great suggestion for
making this clearer though. Maybe "proxied element" (and
proxiedElementChanged(), above)?

[fs, 2016-10-21 11:02:56]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 11:03:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-10-21 11:13:33]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 11:13:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-10-21 11:17:11]

fs@opera.com changed reviewers:
+ esprehn@chromium.org

[fs, 2016-10-21 11:17:12]

On 2016/10/20 at 19:03:42, pdr wrote:
> Very nice! A couple more requests for comments to help future readers.

Thanks! Comments further touched up/reworked/extended.
 
> I think this is ready to go but would like someone more familiar with CSS
loading/resources/etc to review before landing. I'd recommend alancutter or
esprehn who touched similar code in ElementStyleResources.

+esprehn for a look at these things.

https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:20: // A proxy to an
SVGElement. Allows access to an element with a certain 'id',
On 2016/10/20 at 19:03:42, pdr. wrote:
> Do you mind expanding this comment to describe the overall design?
> 
> These are the notes I took while reviewing. Maybe something along these lines?
> * SVGElements can be referenced using CSS (filter: url(foo.com/bar.svg#baz;)
which may require loading external resources. SVGElementProxy is a persistent
reference to an SVGElement both before and after the SVGElement has been found.
> * Filtered content has |CSSURIValue|s which create |SVGElementProxy|s which
also maintain references back to the filtered content via SVGResourceClient.
> * SVGElementProxy looks up SVGElements by id (for remote resources, it will
request the SVGDocument and lookup by id there).
> * When SVGElementProxy finds an SVGElement, it tells the SVGElement which in
turn keeps a list of |SVGElementProxy|s referencing it.
> * These bi-directional references (filtered content <-> SVGElementProxy and
SVGElementProxy <-> SVGFilterElement) are used for invalidation, change
notifications, etc.

Thanks, I tried to jot something down that I hope included all of the above.

https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter)
incremented with each change to the proxy reference.
On 2016/10/20 at 19:03:42, pdr. wrote:
> Nit: "reference" was a little confusing to me when reviewing this because the
proxy both references and is referenced. I don't have a great suggestion for
making this clearer though. Maybe "proxied element" (and
proxiedElementChanged(), above)?

I tried to replace "reference" with "proxied element" (or add it for
disambiguation) here and in a few other places. Also renamed the method (and the
corresponding callback on SVGResourceClient.)

[commit-bot: I haz the power, 2016-10-21 12:42:00]

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

[commit-bot: I haz the power, 2016-10-21 12:42:01]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-10-24 17:02:22]

pdr@chromium.org changed reviewers:
- chrishtr@chromium.org

[pdr., 2016-10-24 17:02:23]

LGTM here, but please wait for a second review from someone more familiar with
CSS resource tracking such as esprehn or alancutter.

[esprehn, 2016-10-25 01:18:43]

This doesn't remove the usage of scheduleSVGFilterLayerUpdateHack() in the paint
invalidation code. I'm worried we're depending on the bad side effects there,
how will this patch remove that hack?

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSURIValue.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSURIValue.cpp:19: SVGElementProxy*
CSSURIValue::createElementProxy(Document& document) const {
ensureElementProxy, this returns the same value every time after the first call.
it should probably actually return a reference?

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp
(right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp:139: if
(style.hasFilter() && style.filter().hasReferenceFilter())
Hmm, can you explain why you need to turn off matched properties caching here? I
wonder why we need a strong reference inside the style value. It seems like the
style should setup observations on the Document for things with the referenced
filter id?

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:3087: lastFilterEffect();
This is strange that it has side effects. Can we refactor it to call whatever
code has the side effects directly instead?

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/FilterOperation.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/FilterOperation.cpp:81:
m_elementProxyGeneration == other.m_elementProxyGeneration;
What changes the generation?

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/FilterOperation.h (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/FilterOperation.h:167: unsigned
m_elementProxyGeneration;
Why do we need a generation instead of a single dirty bit? I'd prefer some
setNeedsFilterUpdate() bit check instead of incrementing counters

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:41:
ThreadState::current()->registerPreFinalizer(this);
What about this feature needs a preFinalizer?

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:93: SVGElement*
element(TreeScope&);
getElement or findElement?

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:113: unsigned generation()
const { return m_generation; }
I'd prefer a dirty bit specific to the subtree or something, my experience is
the generation model is very hard to debug

[fs, 2016-10-25 15:00:53]

(Haven't uploaded new PS, will do that when I've had the time to do something
about the more structural change requests.)

On 2016/10/25 at 01:18:43, esprehn wrote:
> This doesn't remove the usage of scheduleSVGFilterLayerUpdateHack() in the
paint invalidation code. I'm worried we're depending on the bad side effects
there, how will this patch remove that hack?

All the calls leading to that method is removed in this CL. I'd planned to
remove the code in a separate CL to avoid bloating this one even more. It's a
mere cherry-pick away though. Hopefully we should be able to migrate all
resource invalidation to the same type of mechanism.

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSURIValue.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSURIValue.cpp:19: SVGElementProxy*
CSSURIValue::createElementProxy(Document& document) const {
On 2016/10/25 at 01:18:42, esprehn wrote:
> ensureElementProxy, this returns the same value every time after the first
call. it should probably actually return a reference?

Sure, done.

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp
(right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp:139: if
(style.hasFilter() && style.filter().hasReferenceFilter())
On 2016/10/25 at 01:18:42, esprehn wrote:
> Hmm, can you explain why you need to turn off matched properties caching here?

Because mutations to the DOM tree does not reflect in the MatchResults. I guess
optionally this could be handled out-of-band of the style system by having
clients register id observers?

> I wonder why we need a strong reference inside the style value. It seems like
the style should setup observations on the Document for things with the
referenced filter id?

There's only a strong reference to the proxy (and back.) The proxy is
essentially the "observation" mechanism. I guess for the actual target element
we could use IdTargetObservers and do away with the generation mechanism.

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:3087: lastFilterEffect();
On 2016/10/25 at 01:18:42, esprehn wrote:
> This is strange that it has side effects. Can we refactor it to call whatever
code has the side effects directly instead?

Sure. That would mean that every time this is called we'd have to redo it
though. lastFilterEffect() has some notion of validity, so it can avoid that.
Note that this is nothing new in this CL though, rather just removing the
indirection.

Another option would be to call mapRect on the FilterEffect returned by
lastFilterEffect().

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/FilterOperation.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/FilterOperation.cpp:81:
m_elementProxyGeneration == other.m_elementProxyGeneration;
On 2016/10/25 at 01:18:42, esprehn wrote:
> What changes the generation?

All invalidations of the proxy (SVGElementProxy::proxiedElementChanged).

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/FilterOperation.h (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/FilterOperation.h:167: unsigned
m_elementProxyGeneration;
On 2016/10/25 at 01:18:42, esprehn wrote:
> Why do we need a generation instead of a single dirty bit? I'd prefer some
setNeedsFilterUpdate() bit check instead of incrementing counters

(see similar comment in SVGElementProxy.h)

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:41:
ThreadState::current()->registerPreFinalizer(this);
On 2016/10/25 at 01:18:42, esprehn wrote:
> What about this feature needs a preFinalizer?

It's for calling removeClient on m_document (balancing the addClient on the line
above here.)

Given that ResourceClients are now weak, I suppose it's not strictly needed
(registration without unregistration feels kind of iffy though.)

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:93: SVGElement*
element(TreeScope&);
On 2016/10/25 at 01:18:42, esprehn wrote:
> getElement or findElement?

Done.

https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:113: unsigned generation()
const { return m_generation; }
On 2016/10/25 at 01:18:42, esprehn wrote:
> I'd prefer a dirty bit specific to the subtree or something, my experience is
the generation model is very hard to debug

This mechanism is only here for the case of the target element changing. With a
different mechanism (IdTargetObserver?) I think it'd be possible to eliminate
this. I don't see where we'd be served by a dirty bit, since there's no single
point of validation where we'd be resetting it.

[fs, 2016-10-31 17:06:45]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 17:07:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 17:45:46]

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

[commit-bot: I haz the power, 2016-10-31 17:45:47]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fs, 2016-11-01 12:02:34]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 12:02:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-11-01 12:05:41]

Description was changed from

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, in
a somewhat transparent fashion, if requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.) In the latter case, the generation of the proxy
is bumped. This generation (id) is then used to check for changes in the
element reference. This unfortunately still require caching to be
disabled for these ComputedStyle objects.

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-11-01 14:13:29]

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

[commit-bot: I haz the power, 2016-11-01 14:13:30]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-01 14:20:02]

I've reworked things to try and address esprehn's comments. Primarily by using
an IdTargetObserver to track the 'id' per tree scope. This allowed removing the
style caching disablement. Client registration is more complicated than I'd like
though.

PTAL.

[pdr., 2016-11-02 06:35:22]

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (left):

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:476: void
scheduleSVGFilterLayerUpdateHack(Element&);
Woooooooooooooohoooooooooooooo!!!

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:3076: lastFilterEffect();
I agree with your and Elliott's review comments about this being a little weird.
Your idea of calling mapRect on the FilterEffect sounded good to me. WDYT of
adding a TODO above lastFilterEffect to clean this up?

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: 
Can you think of any race situations from using IdTargetObserver? For example,
are the notifications limited to certain document lifecycle phases that could be
asserted for safety?

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:17: class
SVGElementProxy::IdObserver : public IdTargetObserver {
I didn't know about IdTargetObserver... Would this also be useful for other SVG
by-id tracking?

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:43: // For same document
references, the proxy will track changes that could affect
Is this comment still up to date with the IdTargetObserver change? For example,
it looks like external document mutation is supported now (though not used)?

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:47: // et.c.)
nit: etc.

[fs, 2016-11-02 14:07:42]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 14:07:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 15:36:24]

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

[commit-bot: I haz the power, 2016-11-02 15:36:25]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-02 15:49:22]

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:3076: lastFilterEffect();
On 2016/11/02 at 06:35:22, pdr. wrote:
> I agree with your and Elliott's review comments about this being a little
weird. Your idea of calling mapRect on the FilterEffect sounded good to me. WDYT
of adding a TODO above lastFilterEffect to clean this up?

After a somewhat related discussion with senorblanco, I have some other ideas
for how this could end up looking. Adding a general TODO (like above.)

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: 
On 2016/11/02 at 06:35:22, pdr. wrote:
> Can you think of any race situations from using IdTargetObserver? For example,
are the notifications limited to certain document lifecycle phases that could be
asserted for safety?

IIRC, Element::updateId is the primary hook (which is called by various DOM
mutation methods), so I guess one can't say there's any explicit lifecycle
dependencies? Implicitly though I guess we would expect
DocumentLifecycle::stateAllowsTreeMutations() to be true, so maybe that would be
something worth asserting? From the other side of things, I think we need <=
Compositing or so.

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:17: class
SVGElementProxy::IdObserver : public IdTargetObserver {
On 2016/11/02 at 06:35:22, pdr. wrote:
> I didn't know about IdTargetObserver... Would this also be useful for other
SVG by-id tracking?

Yes, I did get the same idea. I'll look in to it (if I remember.)

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right):

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:43: // For same document
references, the proxy will track changes that could affect
On 2016/11/02 at 06:35:22, pdr. wrote:
> Is this comment still up to date with the IdTargetObserver change? For
example, it looks like external document mutation is supported now (though not
used)?

Added code to make the comment correct again.

https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.h:47: // et.c.)
On 2016/11/02 at 06:35:22, pdr. wrote:
> nit: etc.

Fixed.

[pdr., 2016-11-02 20:51:30]

@esprehn, can you give this another review too?


>
https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: 
> On 2016/11/02 at 06:35:22, pdr. wrote:
> > Can you think of any race situations from using IdTargetObserver? For
example, are the notifications limited to certain document lifecycle phases that
could be asserted for safety?
> 
> IIRC, Element::updateId is the primary hook (which is called by various DOM
mutation methods), so I guess one can't say there's any explicit lifecycle
dependencies? Implicitly though I guess we would expect
DocumentLifecycle::stateAllowsTreeMutations() to be true, so maybe that would be
something worth asserting? From the other side of things, I think we need <=
Compositing or so.

Those asserts SGTM. Could you add something along those lines?

[fs, 2016-11-03 12:15:45]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 12:15:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 13:52:15]

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

[commit-bot: I haz the power, 2016-11-03 13:52:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2016-11-03 15:08:14]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 15:08:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-11-03 16:08:52]

On 2016/11/02 at 20:51:30, pdr wrote:
> @esprehn, can you give this another review too?
> 
> 
> >
https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: 
> > On 2016/11/02 at 06:35:22, pdr. wrote:
> > > Can you think of any race situations from using IdTargetObserver? For
example, are the notifications limited to certain document lifecycle phases that
could be asserted for safety?
> > 
> > IIRC, Element::updateId is the primary hook (which is called by various DOM
mutation methods), so I guess one can't say there's any explicit lifecycle
dependencies? Implicitly though I guess we would expect
DocumentLifecycle::stateAllowsTreeMutations() to be true, so maybe that would be
something worth asserting? From the other side of things, I think we need <=
Compositing or so.
> 
> Those asserts SGTM. Could you add something along those lines?

Added.

[commit-bot: I haz the power, 2016-11-03 16:46:49]

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

[commit-bot: I haz the power, 2016-11-03 16:46:51]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-04 15:24:46]

On 2016/11/02 at 20:51:30, pdr wrote:
> @esprehn, can you give this another review too?

esprehn, could you PTA(nother)L?

[esprehn, 2016-11-04 23:58:07]

lgtm w/ some nits and comments. I think that lifecycle assert might be wrong?

https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:56:
DCHECK(lifecycle().state() <= DocumentLifecycle::CompositingClean ||
I'm not sure this holds true, can't something like elementFromPoint() cause
CompositingClean but not PaintClean and then you mutate the DOM which notifies
this observer?

https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:168: if (auto observer =
m_observers.get(&treeScope))
We usually do auto* or auto& to make it clear you're not making a copy

[pdr., 2016-11-05 00:22:06]

LGTM here too

[fs, 2016-11-07 12:58:53]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 12:59:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-11-07 12:59:20]

https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right):

https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:56:
DCHECK(lifecycle().state() <= DocumentLifecycle::CompositingClean ||
On 2016/11/04 at 23:58:07, esprehn wrote:
> I'm not sure this holds true, can't something like elementFromPoint() cause
CompositingClean but not PaintClean and then you mutate the DOM which notifies
this observer?

Since both of these are not strict less/greater than I think that should be
handled, no? I guess we can settle for only the first DCHECK though, since it
ought to be strict enough.

https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:168: if (auto observer =
m_observers.get(&treeScope))
On 2016/11/04 at 23:58:07, esprehn wrote:
> We usually do auto* or auto& to make it clear you're not making a copy

Fixed.

[commit-bot: I haz the power, 2016-11-07 14:37:45]

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

[commit-bot: I haz the power, 2016-11-07 14:37:46]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-07 16:03:14]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 16:03:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 17:41:39]

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

[commit-bot: I haz the power, 2016-11-07 17:41:41]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-08 08:41:53]

The CQ bit was checked by fs@opera.com

[fs, 2016-11-08 08:41:54]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2401343002/#ps280001
(title: "Remove DeprecatedScheduleStyleRecalcDuringLayout include")

[commit-bot: I haz the power, 2016-11-08 08:42:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 08:48:44]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-11-08 08:50:35]

Description was changed from

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}
==========

[commit-bot: I haz the power, 2016-11-08 08:50:36]

Patchset 15 (id:??) landed as
https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}

[pdr., 2016-11-08 19:28:36]

On 2016/11/08 at 08:50:36, commit-bot wrote:
> Patchset 15 (id:??) landed as
https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
> Cr-Commit-Position: refs/heads/master@{#430550}

Thanks Fredrik!

Rest in peace, scheduleSVGFilterLayerUpdateHack

[pdr., 2016-11-09 00:47:08]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2482353002/ by pdr@chromium.org.

The reason for reverting is: There are several reports of issues from this
patch. Lets go ahead and roll out for now.

BUG=663362,663444,663473,663362.

[fs, 2016-11-09 11:43:42]

On 2016/11/09 at 00:47:08, pdr wrote:
> A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2482353002/ by pdr@chromium.org.
> 
> The reason for reverting is: There are several reports of issues from this
patch. Lets go ahead and roll out for now.
> 
> BUG=663362,663444,663473,663362.

Thanks. I've been suffering from limited (network) access recently, which has
severely reduced my response time =(.

[fs, 2016-11-09 17:22:37]

Description was changed from

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}
==========

to

==========
Reland of "Tracking reference filter mutation via SVGElementProxy"

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}
==========

[fs, 2016-11-09 17:23:20]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 17:23:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-11-09 17:25:47]

On 2016/11/09 at 00:47:08, pdr wrote:
> A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2482353002/ by pdr@chromium.org.
> 
> The reason for reverting is: There are several reports of issues from this
patch. Lets go ahead and roll out for now.
> 
> BUG=663362,663444,663473,663362.

Looks like all of these was the same underlying cause - a bug in client
unregistration. Hopefully fixed now (SVGElementProxy::removeClient), and I
simplified local vs. remote handling a bit in addClient/removeClient while at
it. Added two tests based on the bug reports (different crash stacks so figured
it'd be good to keep one of each just in case.)

Permission to reland?

[pdr., 2016-11-09 18:42:23]

On 2016/11/09 at 17:25:47, fs wrote:
> On 2016/11/09 at 00:47:08, pdr wrote:
> > A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2482353002/ by pdr@chromium.org.
> > 
> > The reason for reverting is: There are several reports of issues from this
patch. Lets go ahead and roll out for now.
> > 
> > BUG=663362,663444,663473,663362.
> 
> Looks like all of these was the same underlying cause - a bug in client
unregistration. Hopefully fixed now (SVGElementProxy::removeClient), and I
simplified local vs. remote handling a bit in addClient/removeClient while at
it. Added two tests based on the bug reports (different crash stacks so figured
it'd be good to keep one of each just in case.)
> 
> Permission to reland?

LGTM to reland. Do you mind creating a new review for it though, and post a link
here?

Sorry about making you go through the CQ twice :( I think the two reviews will
help keep this separated if it has to be rolled out again.

[commit-bot: I haz the power, 2016-11-09 19:24:48]

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

[commit-bot: I haz the power, 2016-11-09 19:24:50]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-10 09:07:01]

On 2016/11/09 at 18:42:23, pdr wrote:
> On 2016/11/09 at 17:25:47, fs wrote:
> > On 2016/11/09 at 00:47:08, pdr wrote:
> > > A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2482353002/ by pdr@chromium.org.
> > > 
> > > The reason for reverting is: There are several reports of issues from this
patch. Lets go ahead and roll out for now.
> > > 
> > > BUG=663362,663444,663473,663362.
> > 
> > Looks like all of these was the same underlying cause - a bug in client
unregistration. Hopefully fixed now (SVGElementProxy::removeClient), and I
simplified local vs. remote handling a bit in addClient/removeClient while at
it. Added two tests based on the bug reports (different crash stacks so figured
it'd be good to keep one of each just in case.)
> > 
> > Permission to reland?
> 
> LGTM to reland. Do you mind creating a new review for it though, and post a
link here?
> 
> Sorry about making you go through the CQ twice :( I think the two reviews will
help keep this separated if it has to be rolled out again.

Done here: https://codereview.chromium.org/2490163002

[fs, 2016-11-10 09:07:44]

Description was changed from

==========
Reland of "Tracking reference filter mutation via SVGElementProxy"

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}
==========

to

==========
Tracking reference filter mutation via SVGElementProxy

This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.

Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.

Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)

BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}
==========

[fs, 2016-11-10 09:08:04]

Patchset #17 (id:320001) has been deleted