Issue 423823004: Add support for SVG Clip paths in HTML

Issue 423823004: Add support for SVG Clip paths in HTML (Closed)

Created:
6 years, 4 months ago by Shanmuga Pandi

Modified:
5 years, 6 months ago

Reviewers:
krit, esprehn, Stephen White, fs, f(malita), pdr.

CC:
esprehn, darktears, blink-layers+watch_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-rendering, krit, dstockwell, eae+blinkwatch, ed+blinkwatch_opera.com, Eric Willigers, fs, gyuyoung.kim_webkit.org, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, Mike Lawther (Google), rjwright, rwlbuis, rune+blink, shans, Steve Block, Timothy Loh, zoltan1

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Register non-svg Clip-Path clients to RenderSVGResourceContainter. Make sure non-svg clip-path clients which are having clip-path reference to register with RenderSVGResourceContainer. BUG=391604

Patch Set 1 #

Patch Set 2 : Try to correct A+ mode #

Total comments: 20

Patch Set 3 : Removed unwanted code from RenderLayerClipPathInfo.* and Renderlayer.cpp and Renamed filters to svg… #

Total comments: 24

Patch Set 4 : Changed LayoutTests and Aligned with review comments #

Total comments: 9

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+469 lines, -66 lines)			Patch
A	LayoutTests/css3/clippath/clippath-animated.html	View	1 2 3	1 chunk	+47 lines, -0 lines	1 comment	Download
A +	LayoutTests/css3/clippath/clippath-animated-expected.html	View	1 2 3	1 chunk	+0 lines, -1 line	0 comments	Download
A	LayoutTests/css3/clippath/clippath-mutated.html	View	1 2 3	1 chunk	+44 lines, -0 lines	1 comment	Download
A +	LayoutTests/css3/clippath/clippath-mutated-expected.html	View	1 2 3	1 chunk	+0 lines, -1 line	1 comment	Download
A	LayoutTests/css3/clippath/clippath-reference-add-hw.html	View	1 2 3	1 chunk	+49 lines, -0 lines	0 comments	Download
A +	LayoutTests/css3/clippath/clippath-reference-add-hw-expected.html	View	1 2 3	1 chunk	+0 lines, -1 line	0 comments	Download
A	LayoutTests/css3/clippath/clippath-reference-delete-crash.html	View	1 2 3	1 chunk	+35 lines, -0 lines	0 comments	Download
A +	LayoutTests/css3/clippath/clippath-reference-delete-crash-expected.txt	View	1 2	1 chunk	+1 line, -2 lines	0 comments	Download
A	LayoutTests/css3/clippath/clippath-reference-rename.html	View	1 2 3	1 chunk	+36 lines, -0 lines	0 comments	Download
A	LayoutTests/css3/clippath/clippath-reference-rename-expected.html	View	1 2 3	1 chunk	+32 lines, -0 lines	0 comments	Download
A	LayoutTests/css3/clippath/script-tests/clippath-reference-delete-crash.js	View	1 2 3	1 chunk	+11 lines, -0 lines	1 comment	Download
M	Source/core/core.gypi	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/dom/Document.h	View	1 2 3	3 chunks	+7 lines, -4 lines	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2 3	3 chunks	+11 lines, -11 lines	0 comments	Download
M	Source/core/dom/Element.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/Element.cpp	View	1 2 3	3 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/core/dom/Node.h	View	1 2 3	3 chunks	+13 lines, -7 lines	1 comment	Download
M	Source/core/dom/Node.cpp	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/frame/FrameView.cpp	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLPlugInElement.cpp	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderLayer.h	View	1 2 3	4 chunks	+5 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderLayer.cpp	View	1 2 3	5 chunks	+35 lines, -24 lines	1 comment	Download
A	Source/core/rendering/RenderLayerClipPathInfo.h	View	1 2 3	1 chunk	+39 lines, -0 lines	2 comments	Download
A	Source/core/rendering/RenderLayerClipPathInfo.cpp	View	1 2	1 chunk	+54 lines, -0 lines	1 comment	Download
M	Source/core/rendering/RenderLayerFilterInfo.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/rendering/svg/RenderSVGResourceContainer.cpp	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGClipPathElement.h	View	1 2 3	2 chunks	+7 lines, -0 lines	0 comments	Download
M	Source/core/svg/SVGClipPathElement.cpp	View	1 2 3	2 chunks	+28 lines, -1 line	0 comments	Download

Messages

Total messages: 29 (0 generated)

Expand Messages | Collapse Messages

Shanmuga Pandi

Please review the changes and let me know if need any corrections.

6 years, 4 months ago (2014-07-28 13:26:25 UTC) #1

pdr.

This is a great start. Don't worry about the A+ thing Rietveld does; your patch ...

6 years, 4 months ago (2014-07-28 16:23:53 UTC) #2

f(malita)

(+senorblanco who is more familiar with the CSS filters framework) Note that the actual invalidation ...

6 years, 4 months ago (2014-07-29 01:21:16 UTC) #3

Stephen White

https://codereview.chromium.org/423823004/diff/20001/ManualTests/clip-path-animated.html File ManualTests/clip-path-animated.html (right): https://codereview.chromium.org/423823004/diff/20001/ManualTests/clip-path-animated.html#newcode1 ManualTests/clip-path-animated.html:1: <!DOCTYPE html> On 2014/07/28 16:23:52, pdr wrote: > Your ...

6 years, 4 months ago (2014-07-29 15:12:27 UTC) #4

Stephen White

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/RenderLayerClipPathInfo.h File Source/core/rendering/RenderLayerClipPathInfo.h (right): https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/RenderLayerClipPathInfo.h#newcode34 Source/core/rendering/RenderLayerClipPathInfo.h:34: #include "core/dom/Element.h" Nit: this #include is unnecessary (you'll get ...

6 years, 4 months ago (2014-07-29 15:26:18 UTC) #5

Shanmuga Pandi

Thanks for your comments. I will make the required changes and submit again

6 years, 4 months ago (2014-07-30 07:50:22 UTC) #6

Shanmuga Pandi

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/RenderLayerClipPathInfo.cpp File Source/core/rendering/RenderLayerClipPathInfo.cpp (right): https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/RenderLayerClipPathInfo.cpp#newcode41 Source/core/rendering/RenderLayerClipPathInfo.cpp:41: RenderLayerClipPathInfoMap* RenderLayerClipPathInfo::s_clipPathMap = 0; On 2014/07/28 16:23:52, pdr wrote: ...

6 years, 4 months ago (2014-07-30 11:31:02 UTC) #7

pdr.

On 2014/07/30 11:31:02, Shanmuga Pandi wrote: > https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/RenderLayerClipPathInfo.cpp > File Source/core/rendering/RenderLayerClipPathInfo.cpp (right): > > https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/RenderLayerClipPathInfo.cpp#newcode41 ...

6 years, 4 months ago (2014-07-31 05:40:37 UTC) #8

f(malita)

On 2014/07/31 05:40:37, pdr wrote: > I was thinking of merging RenderLayerFilterInfo and RenderLayerClipPathInfo into ...

6 years, 4 months ago (2014-07-31 14:54:25 UTC) #9

Stephen White

On 2014/07/31 14:54:25, Florin Malita wrote: > On 2014/07/31 05:40:37, pdr wrote: > > I ...

6 years, 4 months ago (2014-07-31 15:24:46 UTC) #10

f(malita)

On 2014/07/31 15:24:46, Stephen White wrote: > On 2014/07/31 14:54:25, Florin Malita wrote: > > ...

6 years, 4 months ago (2014-07-31 16:06:44 UTC) #11

Stephen Chennney

On 2014/07/31 16:06:44, Florin Malita wrote: > On 2014/07/31 15:24:46, Stephen White wrote: > > ...

6 years, 4 months ago (2014-07-31 17:41:32 UTC) #12

On 2014/07/31 16:06:44, Florin Malita wrote:
> On 2014/07/31 15:24:46, Stephen White wrote:
> > On 2014/07/31 14:54:25, Florin Malita wrote:
> > > A somewhat related question: senorblanco, do you recall why we chose to
get
> > > RenderLayers involved in the resource invalidation business in first
place?
> > > Couldn't we just add the actual renderers as resource clients (as we do
for
> > SVG)
> > > and invalidate them directly (which I assume would implicitly invalidate
> their
> > > layer also)? I'm sure there was a reason, but it bothers me that I can't
> > recall
> > > at the moment :)
> > 
> > Which renderers do you mean? CSS filters are applied to RenderLayers, so it
> > seemed 
> > natural to let them handle invalidation when the SVG filter parameters
change.
> > But 
> > I'm not an expert in the render graph code, so if there's a better way of
> > handling 
> > this, I'm all ears.
> 
> The actual Render(Layer)Objects associated with the layer. For SVG, we
register
> the renderers for all filter/clip/mask clients with the resource and dispatch
> invalidations directly to trigger repaint/relayout. For CSS we register the
> associated RenderLayers instead, and dispatch the invalidation their way. If
we
> took the same approach as with SVG, would invalidating the actual RenderObject
> not trigger a correct layer inval/repaint implicitly? I was just wondering
> whether it was *required* to notify the layers directly -- not sure either
> approach is better than the other (and there might be reasons why they are not
> interchangeable).

The original CSS filter implementation only handled filters applied by the
compositor, which means applied on RenderLayers. I speculate that all the CSS
filter code that was later expanded to include SVG reference filters built upon
the same infrastructure. Now that we apply filters through Skia in SVG, in
theory we could probably also move all CSS filters to the same methodology and
remove the requirement that filters generate layers etc. Just wishful thinking,
maybe.

Stephen White

On 2014/07/31 16:06:44, Florin Malita wrote: > On 2014/07/31 15:24:46, Stephen White wrote: > > ...

6 years, 4 months ago (2014-07-31 18:09:52 UTC) #13

On 2014/07/31 16:06:44, Florin Malita wrote:
> On 2014/07/31 15:24:46, Stephen White wrote:
> > On 2014/07/31 14:54:25, Florin Malita wrote:
> > > A somewhat related question: senorblanco, do you recall why we chose to
get
> > > RenderLayers involved in the resource invalidation business in first
place?
> > > Couldn't we just add the actual renderers as resource clients (as we do
for
> > SVG)
> > > and invalidate them directly (which I assume would implicitly invalidate
> their
> > > layer also)? I'm sure there was a reason, but it bothers me that I can't
> > recall
> > > at the moment :)
> > 
> > Which renderers do you mean? CSS filters are applied to RenderLayers, so it
> > seemed 
> > natural to let them handle invalidation when the SVG filter parameters
change.
> > But 
> > I'm not an expert in the render graph code, so if there's a better way of
> > handling 
> > this, I'm all ears.
> 
> The actual Render(Layer)Objects associated with the layer. For SVG, we
register
> the renderers for all filter/clip/mask clients with the resource and dispatch
> invalidations directly to trigger repaint/relayout. For CSS we register the
> associated RenderLayers instead, and dispatch the invalidation their way. If
we
> took the same approach as with SVG, would invalidating the actual RenderObject
> not trigger a correct layer inval/repaint implicitly? I was just wondering
> whether it was *required* to notify the layers directly -- not sure either
> approach is better than the other (and there might be reasons why they are not
> interchangeable).

Actually, it turns out we do one or the other, depending on whether it the
filter has a renderer or not:

            if (filter->renderer())
               
toRenderSVGResourceContainer(filter->renderer())->addClientRenderLayer(m_layer);
            else
               
toSVGFilterElement(filter)->addClient(m_layer->renderer()->node());

(The latter used to use addClientRenderLayer(node), but it looks like that 
flavor is now unused, after a change by Ojan IIRC).

I tried commenting out the first part, and always using
addClient(Node), but that breaks a few layout tests:

css3/filters/effect-reference-delete.html
css3/filters/reference-filter-update-on-attribute-change.html

The latter is the most interesting one. If we could get that to work,
perhaps we could remove the requirement for RenderLayers.

Stephen White

On 2014/07/31 18:09:52, Stephen White wrote: > On 2014/07/31 16:06:44, Florin Malita wrote: > > ...

6 years, 4 months ago (2014-07-31 18:36:06 UTC) #14

On 2014/07/31 18:09:52, Stephen White wrote:
> On 2014/07/31 16:06:44, Florin Malita wrote:
> > On 2014/07/31 15:24:46, Stephen White wrote:
> > > On 2014/07/31 14:54:25, Florin Malita wrote:
> > > > A somewhat related question: senorblanco, do you recall why we chose to
> get
> > > > RenderLayers involved in the resource invalidation business in first
> place?
> > > > Couldn't we just add the actual renderers as resource clients (as we do
> for
> > > SVG)
> > > > and invalidate them directly (which I assume would implicitly invalidate
> > their
> > > > layer also)? I'm sure there was a reason, but it bothers me that I can't
> > > recall
> > > > at the moment :)
> > > 
> > > Which renderers do you mean? CSS filters are applied to RenderLayers, so
it
> > > seemed 
> > > natural to let them handle invalidation when the SVG filter parameters
> change.
> > > But 
> > > I'm not an expert in the render graph code, so if there's a better way of
> > > handling 
> > > this, I'm all ears.
> > 
> > The actual Render(Layer)Objects associated with the layer. For SVG, we
> register
> > the renderers for all filter/clip/mask clients with the resource and
dispatch
> > invalidations directly to trigger repaint/relayout. For CSS we register the
> > associated RenderLayers instead, and dispatch the invalidation their way. If
> we
> > took the same approach as with SVG, would invalidating the actual
RenderObject
> > not trigger a correct layer inval/repaint implicitly? I was just wondering
> > whether it was *required* to notify the layers directly -- not sure either
> > approach is better than the other (and there might be reasons why they are
not
> > interchangeable).
> 
> Actually, it turns out we do one or the other, depending on whether it the
> filter has a renderer or not:
> 
>             if (filter->renderer())
>                
>
toRenderSVGResourceContainer(filter->renderer())->addClientRenderLayer(m_layer);
>             else
>                
> toSVGFilterElement(filter)->addClient(m_layer->renderer()->node());
> 
> (The latter used to use addClientRenderLayer(node), but it looks like that 
> flavor is now unused, after a change by Ojan IIRC).
> 
> I tried commenting out the first part, and always using
> addClient(Node), but that breaks a few layout tests:
> 
> css3/filters/effect-reference-delete.html
> css3/filters/reference-filter-update-on-attribute-change.html
> 
> The latter is the most interesting one. If we could get that to work,
> perhaps we could remove the requirement for RenderLayers.

Ahh, it's this:

    {
        DeprecatedScheduleStyleRecalcDuringLayout
marker(renderer()->document().lifecycle());
        // It's possible for scheduleSVGFilterLayerUpdateHack to schedule a
style recalc, which
        // is a problem because this function can be called while performing
layout.
        // Presumably this represents an illegal data flow of layout or
compositing
        // information into the style system.
        toElement(renderer()->node())->scheduleSVGFilterLayerUpdateHack();
    }

moving that from RenderLayer::filterNeedsPaintInvalidation() to
RenderSVGResourceContainer::markClientForInvalidation() fixes those tests.

But I don't know if you want to inflict that hack on all SVG filters. Maybe
there's a better way of accomplishing the same thing, or at least
restricting it to those clients which need it. I think the basic problem is that
we're coercing SVG animation and/or JS changes into CSS 
style invalidations, since that's all CSS filters know about. But maybe there's
a better way to go about that.

ojan

I expect esprehn has thoughts on this since it affects the schedule layer hacks. For ...

6 years, 4 months ago (2014-07-31 18:43:49 UTC) #15

Stephen White

On 2014/07/31 18:43:49, ojan-only-code-yellow-reviews wrote: > I expect esprehn has thoughts on this since it ...

6 years, 4 months ago (2014-07-31 19:10:16 UTC) #16

Stephen White

On 2014/07/31 17:41:32, Stephen Chenney wrote: > On 2014/07/31 16:06:44, Florin Malita wrote: > > ...

6 years, 4 months ago (2014-07-31 19:32:41 UTC) #17

On 2014/07/31 17:41:32, Stephen Chenney wrote:
> On 2014/07/31 16:06:44, Florin Malita wrote:
> > On 2014/07/31 15:24:46, Stephen White wrote:
> > > On 2014/07/31 14:54:25, Florin Malita wrote:
> > > > A somewhat related question: senorblanco, do you recall why we chose to
> get
> > > > RenderLayers involved in the resource invalidation business in first
> place?
> > > > Couldn't we just add the actual renderers as resource clients (as we do
> for
> > > SVG)
> > > > and invalidate them directly (which I assume would implicitly invalidate
> > their
> > > > layer also)? I'm sure there was a reason, but it bothers me that I can't
> > > recall
> > > > at the moment :)
> > > 
> > > Which renderers do you mean? CSS filters are applied to RenderLayers, so
it
> > > seemed 
> > > natural to let them handle invalidation when the SVG filter parameters
> change.
> > > But 
> > > I'm not an expert in the render graph code, so if there's a better way of
> > > handling 
> > > this, I'm all ears.
> > 
> > The actual Render(Layer)Objects associated with the layer. For SVG, we
> register
> > the renderers for all filter/clip/mask clients with the resource and
dispatch
> > invalidations directly to trigger repaint/relayout. For CSS we register the
> > associated RenderLayers instead, and dispatch the invalidation their way. If
> we
> > took the same approach as with SVG, would invalidating the actual
RenderObject
> > not trigger a correct layer inval/repaint implicitly? I was just wondering
> > whether it was *required* to notify the layers directly -- not sure either
> > approach is better than the other (and there might be reasons why they are
not
> > interchangeable).
> 
> The original CSS filter implementation only handled filters applied by the
> compositor

Actually, the original CSS filter implementation did both compositor- and 
software-rasterized filters.

, which means applied on RenderLayers. I speculate that all the CSS
> filter code that was later expanded to include SVG reference filters built
upon
> the same infrastructure. Now that we apply filters through Skia in SVG, in
> theory we could probably also move all CSS filters to the same methodology and
> remove the requirement that filters generate layers etc. Just wishful
thinking,
> maybe.

Unfortunately not. There are still filters which must be applied in the 
compositor, in particular anything on an element which is itself accelerated: 2D
Canvas, WebGL, 3D CSS, etc. So we'll probably always have two paths for 
rendering filters (but I'm working hard to get it down from six to two :) ).

Shanmuga Pandi

I have done some more changes and change the logic to avoid the redundant code ...

6 years, 4 months ago (2014-08-01 05:20:39 UTC) #18

I have done some more changes and change the logic to avoid the redundant code
of RenderLayerFilterInfo.

Please review it. Thanks

https://codereview.chromium.org/423823004/diff/20001/ManualTests/clip-path-an...
File ManualTests/clip-path-animated.html (right):

https://codereview.chromium.org/423823004/diff/20001/ManualTests/clip-path-an...
ManualTests/clip-path-animated.html:1: <!DOCTYPE html>
On 2014/07/28 16:23:52, pdr wrote:
> Your test looks nicely written but we will need to create an automated test
for
> this. Look in the LayoutTests/svg directory for examples that have animations.
> 1s is too long for a single test so you will need to make sure the new test
> finishes as quickly as possible.

Done.

https://codereview.chromium.org/423823004/diff/20001/ManualTests/clip-path-an...
ManualTests/clip-path-animated.html:1: <!DOCTYPE html>
On 2014/07/29 15:12:26, Stephen White wrote:
> On 2014/07/28 16:23:52, pdr wrote:
> > Your test looks nicely written but we will need to create an automated test
> for
> > this. Look in the LayoutTests/svg directory for examples that have
animations.
> > 1s is too long for a single test so you will need to make sure the new test
> > finishes as quickly as possible.
> 
> +1 for using LayoutTests. You could look at css3/filters/effect-reference* for
> examples of tests for similar functionality in CSS filters, such as
> effect-reference-add-hw.html, effect-reference-delete-crash.html,
> effect-reference-rename.html, etc. Try to use a text-only test (-expected.txt)
> or a reftest (-expected.html) where possible.
> 
> You may also find the text-based repaint tests useful, e.g.
> effect-reference-repaint-*: they use
> fast/repaint/resources/text-based-repaint.js to produce a list of repaint
rects
> in text form.

Done.

https://codereview.chromium.org/423823004/diff/20001/ManualTests/clip-path-mu...
File ManualTests/clip-path-mutated.html (right):

https://codereview.chromium.org/423823004/diff/20001/ManualTests/clip-path-mu...
ManualTests/clip-path-mutated.html:33: </script>
On 2014/07/29 01:21:16, Florin Malita wrote:
> I'd like to see at least a couple of dynamic tests also:
> 
> a) clip-path property updated dynamically to reference an existing element
> b) clip-path property updated dynamically to no longer reference an existing
> element
> 

Done.

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerClipPathInfo.cpp (right):

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerClipPathInfo.cpp:2: * Copyright (c) 2014,
Google Inc. All rights reserved.
On 2014/07/28 16:23:52, pdr wrote:
> Please use the new copyright boilerplate for new files:
> http://www.chromium.org/developers/coding-style#TOC-File-headers

Done.

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerClipPathInfo.cpp:41:
RenderLayerClipPathInfoMap* RenderLayerClipPathInfo::s_clipPathMap = 0;
On 2014/07/28 16:23:52, pdr wrote:
> This patch looks like it's nearly the exact same as RenderLayerFilterInfo
except
> for clip paths. Can we share some of this logic instead of copy pasta?

I have removed unnecessary hashmap code, and added member variable of this to
RenderLayer

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerClipPathInfo.h (right):

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerClipPathInfo.h:34: #include
"core/dom/Element.h"
On 2014/07/29 15:26:18, Stephen White wrote:
> Nit: this #include is unnecessary (you'll get it via SVGClipPathElement,
below).

Done.

https://codereview.chromium.org/423823004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerClipPathInfo.h:36: #include
"core/rendering/ClipPathOperation.h"
On 2014/07/29 15:26:18, Stephen White wrote:
> Looks like you could forward-declare ClipPathOperation instead of #including
it
> here (looks like RenderLayerFilterInfo.h has the same issues).

Done.

pdr.

This is not exactly the direction I had in mind but I think it looks ...

6 years, 4 months ago (2014-08-04 17:06:32 UTC) #19

Shanmuga Pandi

https://codereview.chromium.org/423823004/diff/40001/LayoutTests/css3/clippath/clippath-animated.html File LayoutTests/css3/clippath/clippath-animated.html (right): https://codereview.chromium.org/423823004/diff/40001/LayoutTests/css3/clippath/clippath-animated.html#newcode1 LayoutTests/css3/clippath/clippath-animated.html:1: <html> On 2014/08/04 17:06:31, pdr wrote: > This test ...

6 years, 4 months ago (2014-08-05 11:30:35 UTC) #20

f(malita)

https://codereview.chromium.org/423823004/diff/40001/Source/core/dom/Node.h File Source/core/dom/Node.h (right): https://codereview.chromium.org/423823004/diff/40001/Source/core/dom/Node.h#newcode710 Source/core/dom/Node.h:710: SVGEffectsNeedsLayerUpdateFlag = 1 << 13, "SVGEffectsNeedLayerUpdateFlag" (subj-verb agreement). https://codereview.chromium.org/423823004/diff/40001/Source/core/rendering/RenderLayer.h ...

6 years, 4 months ago (2014-08-05 14:13:08 UTC) #21

Shanmuga Pandi

6 years, 4 months ago (2014-08-05 14:19:56 UTC) #22

pdr.

On 2014/08/05 14:19:56, Shanmuga Pandi wrote: > https://codereview.chromium.org/423823004/diff/40001/LayoutTests/css3/clippath/clippath-animated.html > File LayoutTests/css3/clippath/clippath-animated.html (right): > > https://codereview.chromium.org/423823004/diff/40001/LayoutTests/css3/clippath/clippath-animated.html#newcode1 ...

6 years, 4 months ago (2014-08-05 20:00:31 UTC) #23

Shanmuga Pandi

I have uploaded new patch based on review comments, and added proper layout tests. Please ...

6 years, 4 months ago (2014-08-06 14:51:34 UTC) #24

esprehn

This patch is not correct, you don't handle Shadow DOM correctly and we should not ...

6 years, 4 months ago (2014-08-06 17:25:10 UTC) #25

pdr.

@Shamuga, I think we may need to set this patch aside because fixing the scheduleSVGEffectsLayerUpdateHack ...

6 years, 4 months ago (2014-08-06 21:38:01 UTC) #26

Shanmuga Pandi

6 years, 4 months ago (2014-08-07 13:45:10 UTC) #29

On 2014/08/06 21:38:01, pdr wrote:
> @Shamuga, I think we may need to set this patch aside because fixing the
> scheduleSVGEffectsLayerUpdateHack issue is not an appropriate project for a
> newer engineer to Blink. Would you be interested in working on a smaller but
> important feature such as crbug.com/280423 and then maybe returning to a
larger
> project like this?

@All,
Thanks for your time and review.

@pdr
 I am very much new to SVG. If you like to share some knowledge/hint about
crbug.com/280423, it will be really helpful and will try to proceed. Thanks for
your support.

Expand Messages | Collapse Messages