Remove Element.nodeNamePreservingCase

Remove Element.nodeNamePreservingCase

There is no need for this method, we can just use tagQName.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166250

[rwlbuis, 2014-01-29 18:57:56]

This way avoids some code duplication.

[Inactive, 2014-01-29 19:02:22]

https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
File Source/core/editing/MarkupAccumulator.cpp (right):

https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
Source/core/editing/MarkupAccumulator.cpp:45: #define nodeNamePreservingCase(a)
Element::nodeName(a)
I don't like macros. Why not update the call sites instead?

[rwlbuis, 2014-01-29 19:28:54]

On 2014/01/29 19:02:22, Chris Dumez wrote:
>
https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
> File Source/core/editing/MarkupAccumulator.cpp (right):
> 
>
https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
> Source/core/editing/MarkupAccumulator.cpp:45: #define
nodeNamePreservingCase(a)
> Element::nodeName(a)
> I don't like macros. Why not update the call sites instead?

I thought some people might not like the direct syntax, but I guess we do that
in other places already (although I think it is rare). Will make a new patch.

[Inactive, 2014-01-30 01:24:08]

On 2014/01/29 19:28:54, rwlbuis wrote:
> On 2014/01/29 19:02:22, Chris Dumez wrote:
> >
>
https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
> > File Source/core/editing/MarkupAccumulator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
> > Source/core/editing/MarkupAccumulator.cpp:45: #define
> nodeNamePreservingCase(a)
> > Element::nodeName(a)
> > I don't like macros. Why not update the call sites instead?
> 
> I thought some people might not like the direct syntax, but I guess we do that
> in other places already (although I think it is rare). Will make a new patch.

When you update your patches, don't forget to write a comment and hit
publish+mail again or reviewers aren't notified you did anything.

[Inactive, 2014-01-30 01:29:33]

Don't forget to update your CL description. It still mentions adding a macro.

https://codereview.chromium.org/147593008/diff/20001/Source/core/editing/Mark...
File Source/core/editing/MarkupAccumulator.cpp (right):

https://codereview.chromium.org/147593008/diff/20001/Source/core/editing/Mark...
Source/core/editing/MarkupAccumulator.cpp:396:
result.append(element->Element::nodeName());
I don't really like this syntax. This makes the code less readable than before.

How about result.append(element->tagQName().toString()) ?

[rwlbuis, 2014-01-30 01:36:23]

On 2014/01/30 01:24:08, Chris Dumez wrote:
> On 2014/01/29 19:28:54, rwlbuis wrote:
> > On 2014/01/29 19:02:22, Chris Dumez wrote:
> > >
> >
>
https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
> > > File Source/core/editing/MarkupAccumulator.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/147593008/diff/1/Source/core/editing/MarkupAc...
> > > Source/core/editing/MarkupAccumulator.cpp:45: #define
> > nodeNamePreservingCase(a)
> > > Element::nodeName(a)
> > > I don't like macros. Why not update the call sites instead?
> > 
> > I thought some people might not like the direct syntax, but I guess we do
that
> > in other places already (although I think it is rare). Will make a new
patch.
> 
> When you update your patches, don't forget to write a comment and hit
> publish+mail again or reviewers aren't notified you did anything.

Oops, sorry!

[rwlbuis, 2014-01-30 01:39:25]

On 2014/01/30 01:29:33, Chris Dumez wrote:
> Don't forget to update your CL description. It still mentions adding a macro.

Good point, will fix!

>
https://codereview.chromium.org/147593008/diff/20001/Source/core/editing/Mark...
> File Source/core/editing/MarkupAccumulator.cpp (right):
> 
>
https://codereview.chromium.org/147593008/diff/20001/Source/core/editing/Mark...
> Source/core/editing/MarkupAccumulator.cpp:396:
> result.append(element->Element::nodeName());
> I don't really like this syntax. This makes the code less readable than
before.
> 
> How about result.append(element->tagQName().toString()) ?

That does look better, I'll make a new patch tomorrow when I am back in the
office. Thanks!

[rwlbuis, 2014-01-30 20:14:49]

The new patch now uses tagQName.

[Inactive, 2014-01-30 20:20:47]

On 2014/01/30 20:14:49, rwlbuis wrote:
> The new patch now uses tagQName.

lgtm

[commit-bot: I haz the power, 2014-01-30 20:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/147593008/60001

[commit-bot: I haz the power, 2014-01-30 23:23:48]

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

[commit-bot: I haz the power, 2014-01-30 23:23:48]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 23:23:55]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 23:26:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/147593008/60001

[commit-bot: I haz the power, 2014-01-31 02:08:29]

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

[commit-bot: I haz the power, 2014-01-31 02:08:30]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 02:09:07]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 02:12:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/147593008/60001

[commit-bot: I haz the power, 2014-01-31 05:25:13]

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

[commit-bot: I haz the power, 2014-01-31 05:25:17]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 05:25:22]

CQ bit was unchecked on CL. Ignoring.

[eseidel, 2014-01-31 05:41:00]

https://codereview.chromium.org/147593008/diff/60001/Source/core/editing/Mark...
File Source/core/editing/MarkupAccumulator.cpp (left):

https://codereview.chromium.org/147593008/diff/60001/Source/core/editing/Mark...
Source/core/editing/MarkupAccumulator.cpp:396:
result.append(element.nodeNamePreservingCase());
I see, we can't use nodeName() because it doesn't preserve case?

I think that the previous code as clearer.  Its not at all clear when reading
this why we go through tagName to get this.

[eseidel, 2014-01-31 05:41:50]

Or we could just add a static String nodeNamePreservingCase(Element*) to
MarkupAccumulator?

[Inactive, 2014-01-31 14:02:48]

I was confused by the fact that Element::nodeName() and
Element::nodeNamePreservingCase() have the same implementation. It makes it look
like nodeNamePreservingCase() is not needed. However, Element::nodeName() is
virtual and overridden in HTMLElement and its implementation does not preserve
the case.

As a consequence, I think the Element::nodeNamePreservingCase() method has some
use. Maybe we could just update Element::nodeName() to call
nodeNamePreservingCase() and avoid duplicating the method body.

[rwlbuis, 2014-01-31 16:00:26]

On 2014/01/31 05:41:50, eseidel wrote:
> Or we could just add a static String nodeNamePreservingCase(Element*) to
> MarkupAccumulator?

I like this suggestion, will make a new patch set.

[rwlbuis, 2014-01-31 19:19:53]

New patch set is ready and turning up first green results.

[eseidel, 2014-01-31 22:36:15]

lgtm

:shrug:.  This CL doesn't really seem to do much.

[commit-bot: I haz the power, 2014-01-31 22:36:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/147593008/100001

[commit-bot: I haz the power, 2014-01-31 23:39:22]

Change committed as 166250

[commit-bot: I haz the power, 2014-01-31 23:39:22]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 23:39:31]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 23:39:31]

CQ bit was unchecked on CL. Ignoring.