Use new is*Element() helper functions in DOM code

Use new is*Element() helper functions in DOM code

Use new is*Element() helpers in DOM code. This makes the code clearer and
simpler. Also use the new Traversal<*Element> API when suitable.

R=eseidel, adamk
BUG=346095
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168983

[eseidel, 2014-03-10 18:17:00]

lgtm

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:271: ASSERT(!isHTMLTemplateElement(this));
This makes so much more sense!

Now if we could only get rid of the siliness in the constructors where we set
these names explicitly for each...

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/ScriptLoader...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/ScriptLoader...
Source/core/dom/ScriptLoader.cpp:285: return isHTMLScriptElement(*element);
Won't dereferencing null on the next line crash just the same as the ASSERT
would?

[commit-bot: I haz the power, 2014-03-10 18:17:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/192293002/1

[Inactive, 2014-03-10 18:21:54]

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/ScriptLoader...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/ScriptLoader...
Source/core/dom/ScriptLoader.cpp:285: return isHTMLScriptElement(*element);
On 2014/03/10 18:17:01, eseidel wrote:
> Won't dereferencing null on the next line crash just the same as the ASSERT
> would?

I would prefer hitting an assertion than actually crashing. In any case, I will
likely further refactor this in a follow-up so that isHTMLScriptLoader() takes a
reference in parameter. I just want to keep the change small.

Having this assert in place makes it very obvious the argument is expected to be
non-null and can become a reference.

[adamk, 2014-03-10 18:23:23]

https://codereview.chromium.org/192293002/diff/1/Source/build/scripts/templat...
File Source/build/scripts/templates/ElementTypeHelpers.h.tmpl (right):

https://codereview.chromium.org/192293002/diff/1/Source/build/scripts/templat...
Source/build/scripts/templates/ElementTypeHelpers.h.tmpl:30: template<typename
T> inline bool is{{tag.interface}}(const RefPtr<T>& node) { return
is{{tag.interface}}(node.get()); }
This seems possibly-dangerous, as passing a PassRefPtr into this method will
null it out (due to the RefPtr(PassRefPtr) implicit constructor), so the
conversion from

node->hasTagName(fooTag)

to

isHTMLFooElement(node)

will have a difference in behavior. I think you can make this safe at the cost
of an extra ref by using const PassRefPtr& as the type.

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2990: ASSERT(isHTMLLabelElement(this));
Why do you sometimes use "*this" and sometimes "this" when calling these
functions? I'd think you would always use the reference version...

[Inactive, 2014-03-10 18:26:08]

The CQ bit was unchecked by ch.dumez@samsung.com

[Inactive, 2014-03-10 18:28:29]

https://codereview.chromium.org/192293002/diff/1/Source/build/scripts/templat...
File Source/build/scripts/templates/ElementTypeHelpers.h.tmpl (right):

https://codereview.chromium.org/192293002/diff/1/Source/build/scripts/templat...
Source/build/scripts/templates/ElementTypeHelpers.h.tmpl:30: template<typename
T> inline bool is{{tag.interface}}(const RefPtr<T>& node) { return
is{{tag.interface}}(node.get()); }
On 2014/03/10 18:23:24, adamk wrote:
> This seems possibly-dangerous, as passing a PassRefPtr into this method will
> null it out (due to the RefPtr(PassRefPtr) implicit constructor), so the
> conversion from
> 
> node->hasTagName(fooTag)
> 
> to
> 
> isHTMLFooElement(node)
> 
> will have a difference in behavior. I think you can make this safe at the cost
> of an extra ref by using const PassRefPtr& as the type.

You are right, I believe we had the exact same discussion on the Node comparison
operators. I actually importing this line from the
DEFINE_NODE_TYPE_CASTS_WITH_FUNCTION() macro without paying enough attention. I
will fix this here and in the Node macro and reupload.

[Inactive, 2014-03-10 21:42:16]

Please take another look

https://codereview.chromium.org/192293002/diff/20001/Source/build/scripts/tem...
File Source/build/scripts/templates/ElementTypeHelpers.h.tmpl (right):

https://codereview.chromium.org/192293002/diff/20001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementTypeHelpers.h.tmpl:30: template<typename
T> inline bool is{{tag.interface}}(const PassRefPtr<T>& node) { return
is{{tag.interface}}(node.get()); }
Overloading seems to work here so I did that instead of replacing the one taking
a RefPtr. This avoids unnecessary refcounting churn.

[Inactive, 2014-03-11 21:32:12]

Adamk, does the latest patch look good to you as well?

[adamk, 2014-03-11 21:40:26]

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/192293002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2990: ASSERT(isHTMLLabelElement(this));
On 2014/03/10 18:23:24, adamk wrote:
> Why do you sometimes use "*this" and sometimes "this" when calling these
> functions? I'd think you would always use the reference version...

Still curious for an answer to this (maybe for ASSERTs you just didn't care?)

https://codereview.chromium.org/192293002/diff/20001/Source/build/scripts/tem...
File Source/build/scripts/templates/ElementTypeHelpers.h.tmpl (right):

https://codereview.chromium.org/192293002/diff/20001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementTypeHelpers.h.tmpl:30: template<typename
T> inline bool is{{tag.interface}}(const PassRefPtr<T>& node) { return
is{{tag.interface}}(node.get()); }
On 2014/03/10 21:42:16, Chris Dumez wrote:
> Overloading seems to work here so I did that instead of replacing the one
taking
> a RefPtr. This avoids unnecessary refcounting churn.

Okay, if this works properly it lgtm.

[Inactive, 2014-03-11 22:08:26]

On 2014/03/11 21:40:26, adamk wrote:
> https://codereview.chromium.org/192293002/diff/1/Source/core/dom/Element.cpp
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/192293002/diff/1/Source/core/dom/Element.cpp#...
> Source/core/dom/Element.cpp:2990: ASSERT(isHTMLLabelElement(this));
> On 2014/03/10 18:23:24, adamk wrote:
> > Why do you sometimes use "*this" and sometimes "this" when calling these
> > functions? I'd think you would always use the reference version...
> 
> Still curious for an answer to this (maybe for ASSERTs you just didn't care?)

Sorry, I did not see this question earlier.

Basically, if I know it is not null, I pass a reference to avoid the null check.
For assertions, I felt the readability mattered more than efficiency so I did
not bother passing references in assertions. I don't really mind either way
though if you feel assertions should be consistent with the rest.

[adamk, 2014-03-11 22:40:07]

On Tue, Mar 11, 2014 at 3:08 PM, <ch.dumez@samsung.com> wrote:

> On 2014/03/11 21:40:26, adamk wrote:
>
>> https://codereview.chromium.org/192293002/diff/1/Source/
>> core/dom/Element.cpp
>> File Source/core/dom/Element.cpp (right):
>>
>
>
> https://codereview.chromium.org/192293002/diff/1/Source/
> core/dom/Element.cpp#newcode2990
>
>> Source/core/dom/Element.cpp:2990: ASSERT(isHTMLLabelElement(this));
>> On 2014/03/10 18:23:24, adamk wrote:
>> > Why do you sometimes use "*this" and sometimes "this" when calling these
>> > functions? I'd think you would always use the reference version...
>>
>
>  Still curious for an answer to this (maybe for ASSERTs you just didn't
>> care?)
>>
>
> Sorry, I did not see this question earlier.
>
> Basically, if I know it is not null, I pass a reference to avoid the null
> check.
> For assertions, I felt the readability mattered more than efficiency so I
> did
> not bother passing references in assertions. I don't really mind either way
> though if you feel assertions should be consistent with the rest
>

Sounds reasonable, people just might copy-paste out of the ASSERT and into
their code. But this will only very rarely matter (where the extra branch
is a problem), so sounds fine to me.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Inactive, 2014-03-11 22:40:42]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-03-11 22:40:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/192293002/20001

[commit-bot: I haz the power, 2014-03-12 02:32:28]

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

[commit-bot: I haz the power, 2014-03-12 02:32:28]

Try jobs failed on following builders: linux_blink

[Inactive, 2014-03-12 02:34:38]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-03-12 02:34:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/192293002/20001

[commit-bot: I haz the power, 2014-03-12 06:34:53]

Change committed as 168983