Introduce isSVGFontElement() and isSVGImageElement(), and use them

Introduce isSVGFontElement() and isSVGImageElement(), and use them

Let's use isFoo() to enhance readibility in svg element classes.

WebKit merge from http://trac.webkit.org/changeset/152524

BUG=

[gyuyoung-inactive, 2013-07-11 03:05:38]

Add tkent as reviewer

[tkent, 2013-07-11 04:09:43]

lgtm

However, I don't think patches to add just isFoo function is not so valuable.
Adding toFoo function is more important because it can prevent bad casts.

[commit-bot: I haz the power, 2013-07-11 04:10:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/18278003/1

[pdr., 2013-07-11 04:24:25]

What's the reason for adding these? In many cases this pattern hurts readability
because it's inconsistent to have some isXXX() and some hasTagName(XXX).

[tkent, 2013-07-11 04:31:26]

> However, I don't think patches to add just isFoo function is not so valuable.

Please remove either of 'not' :-D

[tkent, 2013-07-11 04:35:59]

On 2013/07/11 04:24:25, pdr wrote:
> What's the reason for adding these? In many cases this pattern hurts
readability
> because it's inconsistent to have some isXXX() and some hasTagName(XXX).

Probably gyuyoung wants to replace all of hasTagName(XXX) with isXXX().

[pdr., 2013-07-11 04:45:20]

On 2013/07/11 04:35:59, tkent wrote:
> On 2013/07/11 04:24:25, pdr wrote:
> > What's the reason for adding these? In many cases this pattern hurts
> readability
> > because it's inconsistent to have some isXXX() and some hasTagName(XXX).
> 
> Probably gyuyoung wants to replace all of hasTagName(XXX) with isXXX().

If that's the case, can we use a macro? If they are all done the way this patch
does them, we'll have to pull in additional #include dependencies all over and
pollute the code with extra isSVG*() functions.

[commit-bot: I haz the power, 2013-07-11 05:16:35]

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

[tkent, 2013-07-11 08:52:31]

I reconsider this CL, and this does not lgtm for now.

Using isFoo with toFoo is helpful. However isFoo without toFoo has no benefit
and has downside (require Foo.h).

[gyuyoung-inactive, 2013-07-12 01:33:26]

On 2013/07/11 08:52:31, tkent wrote:
> I reconsider this CL, and this does not lgtm for now.
> 
> Using isFoo with toFoo is helpful. However isFoo without toFoo has no benefit
> and has downside (require Foo.h).

As you thought, I want to change all hasTagName with isXXX(). HTML classes on
WebKit is adding it isXXX() and toXXX(). So, I thought SVG classes can adopt it
as well. This patch is to add isSVGFontElement() and isSVGImageElement() because
toSVGFontElement() and toSVGImageElement() were already landed.

[gyuyoung-inactive, 2013-07-12 01:35:45]

On 2013/07/12 01:33:26, gyuyoung wrote:
> On 2013/07/11 08:52:31, tkent wrote:
> > I reconsider this CL, and this does not lgtm for now.
> > 
> > Using isFoo with toFoo is helpful. However isFoo without toFoo has no
benefit
> > and has downside (require Foo.h).
> 
> As you thought, I want to change all hasTagName with isXXX(). HTML classes on
> WebKit is adding it isXXX() and toXXX(). So, I thought SVG classes can adopt
it
> as well. This patch is to add isSVGFontElement() and isSVGImageElement()
because
> toSVGFontElement() and toSVGImageElement() were already landed.

Adam also set 'lgtm' in the html case.

https://codereview.chromium.org/18205009/

[tkent, 2013-07-12 01:54:02]

On 2013/07/12 01:33:26, gyuyoung wrote:
> As you thought, I want to change all hasTagName with isXXX(). HTML classes on
> WebKit is adding it isXXX() and toXXX(). So, I thought SVG classes can adopt
it
> as well. This patch is to add isSVGFontElement() and isSVGImageElement()
because
> toSVGFontElement() and toSVGImageElement() were already landed.

SVG or not doesn't matter.
I mean the code like [1] is good, but the code like [2] is questionable.

[1]
  if (isSVGFontElement(node))
      toSVGFontElement(node)->something();

[2]
  if (isSVGFontElement(node) || isSVGImageElement(node) || ...) {
      // No cast to SVGFontElement
      ...


For example, HitTestResult.cpp contains the following code.

    if (m_innerNonSharedNode->hasTagName(embedTag)
        || m_innerNonSharedNode->hasTagName(imgTag)
        || m_innerNonSharedNode->hasTagName(inputTag)
        || m_innerNonSharedNode->hasTagName(objectTag)    
        || m_innerNonSharedNode->hasTagName(SVGNames::imageTag)
       ) {
        Element* element = toElement(m_innerNonSharedNode.get());
        urlString = element->imageSourceURL();

This code doesn't require casts to SVGImageElement, and probably we need another
virtual method like Element::isImageContainer().

HTMLTreeBuilder.cpp already has some isHTMLFooElement. I also think it was
unnecessary because HTML parser should focus on tag names, and should not know
much about element implementation classes.

[gyuyoung-inactive, 2013-07-12 04:18:21]

On 2013/07/12 01:54:02, tkent wrote:
> On 2013/07/12 01:33:26, gyuyoung wrote:
> > As you thought, I want to change all hasTagName with isXXX(). HTML classes
on
> > WebKit is adding it isXXX() and toXXX(). So, I thought SVG classes can adopt
> it
> > as well. This patch is to add isSVGFontElement() and isSVGImageElement()
> because
> > toSVGFontElement() and toSVGImageElement() were already landed.
> 
> SVG or not doesn't matter.
> I mean the code like [1] is good, but the code like [2] is questionable.
> 
> [1]
>   if (isSVGFontElement(node))
>       toSVGFontElement(node)->something();
> 
> [2]
>   if (isSVGFontElement(node) || isSVGImageElement(node) || ...) {
>       // No cast to SVGFontElement
>       ...
> 
> 
> For example, HitTestResult.cpp contains the following code.
> 
>     if (m_innerNonSharedNode->hasTagName(embedTag)
>         || m_innerNonSharedNode->hasTagName(imgTag)
>         || m_innerNonSharedNode->hasTagName(inputTag)
>         || m_innerNonSharedNode->hasTagName(objectTag)    
>         || m_innerNonSharedNode->hasTagName(SVGNames::imageTag)
>        ) {
>         Element* element = toElement(m_innerNonSharedNode.get());
>         urlString = element->imageSourceURL();
> ewkViewContextMenuClientAttachClient
> This code doesn't require casts to SVGImageElement, and probably we need
another
> virtual method like Element::isImageContainer().
> 
> HTMLTreeBuilder.cpp already has some isHTMLFooElement. I also think it was
> unnecessary because HTML parser should focus on tag names, and should not know
> much about element implementation classes.

If you think this is still questionable, I also think this again. It looks your
opinion also makes sense.

[pdr., 2013-07-15 20:50:32]

> If you think this is still questionable, I also think this again. It looks
your
> opinion also makes sense.

I'd like to propose something that might be better for both WebKit and Blink :)

What about adding these to SVGElement.h? This is already included everywhere so
no additional includes will be needed. In addition, you can generate them
dynamically using a macro so you don't need to add toSVG* to every class. You'd
just have a mapping of tag names to elements and generate the ~30 or so
functions using a macro.

[gyuyoung-inactive, 2013-07-17 01:21:27]

On 2013/07/15 20:50:32, pdr wrote:
> > If you think this is still questionable, I also think this again. It looks
> your
> > opinion also makes sense.
> 
> I'd like to propose something that might be better for both WebKit and Blink
:)
> 
> What about adding these to SVGElement.h? This is already included everywhere
so
> no additional includes will be needed. In addition, you can generate them
> dynamically using a macro so you don't need to add toSVG* to every class.
You'd
> just have a mapping of tag names to elements and generate the ~30 or so
> functions using a macro.

@pdr I just read your comment :) Your suggestion looks good idea. I will try to
make a prototype. However, I'm not sure if
we can replace all toSVG* with this. Let me check it. ;)

[gyuyoung-inactive, 2013-07-17 01:29:22]

On 2013/07/17 01:21:27, gyuyoung wrote:
 
> @pdr I just read your comment :) Your suggestion looks good idea. I will try
to
> make a prototype. However, I'm not sure if
> we can replace all toSVG* with this. Let me check it. ;)

BTW, if you don't mind, I would like to land toSVG* patches as well as try this
idea. Do you agree ? ;)

[pdr., 2013-07-17 01:52:09]

On 2013/07/17 01:29:22, gyuyoung wrote:
> On 2013/07/17 01:21:27, gyuyoung wrote:
>  
> > @pdr I just read your comment :) Your suggestion looks good idea. I will try
> to
> > make a prototype. However, I'm not sure if
> > we can replace all toSVG* with this. Let me check it. ;)
> 
> BTW, if you don't mind, I would like to land toSVG* patches as well as try
this
> idea. Do you agree ? ;)

I absolutely agree! I apologize about my comment on the other bug--I misread it
to be a isSVG* change.

[gyuyoung-inactive, 2013-07-22 11:35:52]

On 2013/07/17 01:52:09, pdr wrote:
> On 2013/07/17 01:29:22, gyuyoung wrote:
> > On 2013/07/17 01:21:27, gyuyoung wrote:
> >  
> > > @pdr I just read your comment :) Your suggestion looks good idea. I will
try
> > to
> > > make a prototype. However, I'm not sure if
> > > we can replace all toSVG* with this. Let me check it. ;)
> > 
> > BTW, if you don't mind, I would like to land toSVG* patches as well as try
> this
> > idea. Do you agree ? ;)
> 
> I absolutely agree! I apologize about my comment on the other bug--I misread
it
> to be a isSVG* change.

@pdr, @tkent, how about adding isSVGChildElement() function to SVGElement.h ?
Then, we only use it if there is casting inside condition parenthesis.

inline bool isSVGChildElement(Node* node, const WebCore::QualifiedName name)
{
    ASSERT_WITH_SECURITY_IMPLICATION(!node || node->isSVGElement());
    return node->hasTagName(name);
}

As below function, we use the isSVGChildElement().

void SVGGlyphElement::invalidateGlyphCache()
{
    if (fontNode && isSVGChildElement(fontNode, SVGNames::fontTag))
        toSVGFontElement(fontNode)->invalidateGlyphCache();
}

[tkent, 2013-07-22 23:44:03]

> @pdr, @tkent, how about adding isSVGChildElement() function to SVGElement.h ?
> Then, we only use it if there is casting inside condition parenthesis.
> 
> inline bool isSVGChildElement(Node* node, const WebCore::QualifiedName name)
> {
>     ASSERT_WITH_SECURITY_IMPLICATION(!node || node->isSVGElement());
>     return node->hasTagName(name);
> }
> 
> As below function, we use the isSVGChildElement().
> 
> void SVGGlyphElement::invalidateGlyphCache()
> {
>     if (fontNode && isSVGChildElement(fontNode, SVGNames::fontTag))
>         toSVGFontElement(fontNode)->invalidateGlyphCache();
> }

It looks to help nothing.  I think isFooElement function can be helpful only if
it improves encapsulation.  i.e. callsites don't need to specify fooTag.

[gyuyoung-inactive, 2013-07-23 05:56:30]

On 2013/07/22 23:44:03, tkent wrote:
> > @pdr, @tkent, how about adding isSVGChildElement() function to SVGElement.h
?
> > Then, we only use it if there is casting inside condition parenthesis.
> > 
> > inline bool isSVGChildElement(Node* node, const WebCore::QualifiedName name)
> > {
> >     ASSERT_WITH_SECURITY_IMPLICATION(!node || node->isSVGElement());
> >     return node->hasTagName(name);
> > }
> > 
> > As below function, we use the isSVGChildElement().
> > 
> > void SVGGlyphElement::invalidateGlyphCache()
> > {
> >     if (fontNode && isSVGChildElement(fontNode, SVGNames::fontTag))
> >         toSVGFontElement(fontNode)->invalidateGlyphCache();
> > }
> 
> It looks to help nothing.  I think isFooElement function can be helpful only
if
> it improves encapsulation.  i.e. callsites don't need to specify fooTag.

If so, how do you think to expand isSVG* function for other SVG classes? As you
know there are three functions in SVGElement.h.

    virtual bool isSVGStyledElement() const { return false; }
    virtual bool isSVGGraphicsElement() const { return false; }
    virtual bool isSVGSVGElement() const { return false; }


Though I don't know why only three functions were added, it looks we can add new
functions to there.

For instance,

In SVGElement.h
+    virtual bool isSVGPathElement() const { return false; }

In SVGPathElement.h
+    virtual bool isSVGPathElement() const OVERRIDE { return true; }

Then, we may simplify below line in SVGRenderTreeAsText.cpp as below,
-    } else if (svgElement->hasTagName(SVGNames::pathTag)) {
+    } else if (svgElement->isSVGPathElement() {

[tkent, 2013-07-24 02:10:42]

On 2013/07/23 05:56:30, gyuyoung wrote:
> If so, how do you think to expand isSVG* function for other SVG classes? As
you
> know there are three functions in SVGElement.h.
> 
>     virtual bool isSVGStyledElement() const { return false; }
>     virtual bool isSVGGraphicsElement() const { return false; }
>     virtual bool isSVGSVGElement() const { return false; }
> 
> 
> Though I don't know why only three functions were added, it looks we can add
new
> functions to there.

hasTagName doesn't work well for these classes because they are used for
multiple tags. Elements in core/html/shadow/ also use this pattern because we
can't use hasTagName for them at all.

Adding other isSVGFooElement member functions would be acceptable only if it
reduces the code size significantly.  However, I don't think this task is worth
to consume your time.

[gyuyoung-inactive, 2013-07-24 03:32:33]

On 2013/07/24 02:10:42, tkent wrote:
> On 2013/07/23 05:56:30, gyuyoung wrote:
> > If so, how do you think to expand isSVG* function for other SVG classes? As
> you
> > know there are three functions in SVGElement.h.
> > 
> >     virtual bool isSVGStyledElement() const { return false; }
> >     virtual bool isSVGGraphicsElement() const { return false; }
> >     virtual bool isSVGSVGElement() const { return false; }
> > 
> > 
> > Though I don't know why only three functions were added, it looks we can add
> new
> > functions to there.
> 
> hasTagName doesn't work well for these classes because they are used for
> multiple tags. Elements in core/html/shadow/ also use this pattern because we
> can't use hasTagName for them at all.
> 
> Adding other isSVGFooElement member functions would be acceptable only if it
> reduces the code size significantly.  However, I don't think this task is
worth
> to consume your time.

I see. ok, let's stop to think about adding this helper function at the moment.
Thank you for kindly reply. :)