className should be allowed to be a whitespace-only string

Classnames containing only whitespaces should be allowed. Whitespace strings are treated as empty strings in current implementation which is incorrect.

BUG=526289
Committed: https://crrev.com/16643355966bb0e8f6b42516dacff90bbf80d3f8
Cr-Commit-Position: refs/heads/master@{#354473}

[tanay.c, 2015-10-06 07:11:12]

tanay.c@samsung.com changed reviewers:
+ pdr@chromium.org, philipj@opera.com, tkent@chromium.org

[tanay.c, 2015-10-06 07:11:13]

PTAL

[tkent, 2015-10-06 07:19:02]

Please add a test.

https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1243: return i <= length;
It means this function always returns |true|. Do you really want it?

[tanay.c, 2015-10-06 09:56:14]

On 2015/10/06 07:19:02, tkent wrote:
> Please add a test.
> 
>
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Element.cpp:1243: return i <= length;
> It means this function always returns |true|. Do you really want it?

Yes , it will return |true| for all cases. There can be a separate function just
to check for all space entry, but 
it would have duplicate code. Hence, I am checking in the same function.
Also since we are taking even empty classnames there should not be any |false|
cases. Unless, I am missing something.

Added a test case. Please have a look.

[tkent, 2015-10-07 00:25:51]

On 2015/10/06 at 09:56:14, tanay.c wrote:
> On 2015/10/06 07:19:02, tkent wrote:
> > Please add a test.
> > 
> >
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > 
> >
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/dom/Element.cpp:1243: return i <= length;
> > It means this function always returns |true|. Do you really want it?
> 
> Yes , it will return |true| for all cases. There can be a separate function
just to check for all space entry, but 
> it would have duplicate code. Hence, I am checking in the same function.
> Also since we are taking even empty classnames there should not be any |false|
cases. Unless, I am missing something.

In your approach, you should remove classStringHasClassName() functions
entirely.  They have no functionality.

However, I think the current approach is not good.  if newClassString is
white-space only, we should call classChangedForElement(oldClass, *this) as
ever, and should not call classChangedForElement(oldClass, newClass, *this) for
efficiency.


Also, the CL description is not correct.

[tanay.c, 2015-10-08 12:04:53]

On 2015/10/07 00:25:51, tkent wrote:
> On 2015/10/06 at 09:56:14, tanay.c wrote:
> > On 2015/10/06 07:19:02, tkent wrote:
> > > Please add a test.
> > > 
> > >
>
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/dom/Element.cpp:1243: return i <= length;
> > > It means this function always returns |true|. Do you really want it?
> > 
> > Yes , it will return |true| for all cases. There can be a separate function
> just to check for all space entry, but 
> > it would have duplicate code. Hence, I am checking in the same function.
> > Also since we are taking even empty classnames there should not be any
|false|
> cases. Unless, I am missing something.
> 
> In your approach, you should remove classStringHasClassName() functions
> entirely.  They have no functionality.
> 
> However, I think the current approach is not good.  if newClassString is
> white-space only, we should call classChangedForElement(oldClass, *this) as
> ever, and should not call classChangedForElement(oldClass, newClass, *this)
for
> efficiency.
> 
> 
> Also, the CL description is not correct.

PTAL. The tests failing as the classList.remove() would set 
the class to a single char blank string and this is now a valid
class name. Please let me know your comments.

[tanay.c, 2015-10-08 12:07:35]

On 2015/10/08 12:04:53, tanay.c wrote:
> On 2015/10/07 00:25:51, tkent wrote:
> > On 2015/10/06 at 09:56:14, tanay.c wrote:
> > > On 2015/10/06 07:19:02, tkent wrote:
> > > > Please add a test.
> > > > 
> > > >
> >
>
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/1388903002/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/dom/Element.cpp:1243: return i <= length;
> > > > It means this function always returns |true|. Do you really want it?
> > > 
> > > Yes , it will return |true| for all cases. There can be a separate
function
> > just to check for all space entry, but 
> > > it would have duplicate code. Hence, I am checking in the same function.
> > > Also since we are taking even empty classnames there should not be any
> |false|
> > cases. Unless, I am missing something.
> > 
> > In your approach, you should remove classStringHasClassName() functions
> > entirely.  They have no functionality.
> > 
> > However, I think the current approach is not good.  if newClassString is
> > white-space only, we should call classChangedForElement(oldClass, *this) as
> > ever, and should not call classChangedForElement(oldClass, newClass, *this)
> for
> > efficiency.
> > 
> > 
> > Also, the CL description is not correct.
> 
> PTAL. The tests failing as the classList.remove() would set 
> the class to a single char blank string and this is now a valid
> class name. Please let me know your comments.

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
show the classname='' attribute retained

[tkent, 2015-10-08 23:20:12]

https://codereview.chromium.org/1388903002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1388903002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1232: static inline bool
classStringHasClassName(const CharacterType* characters, unsigned length, bool&
isWhiteSpaceOnly)
We should introduce an enum, and this function should return it.

e.g.

enum class ClassStringContent {
    Empty,
    WhiteSpaceOnly,
    HasClasses
};

template <typename CharacterType>
static inline ClassStringContent analyzeClassString(const CharacterType*
characters, unsigned length) ...

https://codereview.chromium.org/1388903002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1269: bool
isWhiteSpaceOnlyClassname;
This variable needs to be initialized, or classStringHasClassName() should set
false in i!=length case.

[tanay.c, 2015-10-16 05:30:47]

On 2015/10/08 23:20:12, tkent wrote:
>
https://codereview.chromium.org/1388903002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/1388903002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Element.cpp:1232: static inline bool
> classStringHasClassName(const CharacterType* characters, unsigned length,
bool&
> isWhiteSpaceOnly)
> We should introduce an enum, and this function should return it.
> 
> e.g.
> 
> enum class ClassStringContent {
>     Empty,
>     WhiteSpaceOnly,
>     HasClasses
> };
> 
> template <typename CharacterType>
> static inline ClassStringContent analyzeClassString(const CharacterType*
> characters, unsigned length) ...
> 
>
https://codereview.chromium.org/1388903002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Element.cpp:1269: bool
> isWhiteSpaceOnlyClassname;
> This variable needs to be initialized, or classStringHasClassName() should set
> false in i!=length case.

Thanks for the suggestions. I have made the changes. Could you please take a
look?

[tkent, 2015-10-16 05:40:32]

Almost ok.

https://codereview.chromium.org/1388903002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1388903002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:89: enum ClassStringContent {
Please use |enum class| instead of |enum|, and move this to Element.cpp.

[tanay.c, 2015-10-16 07:17:36]

PTAL.

https://codereview.chromium.org/1388903002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1388903002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:89: enum ClassStringContent {
On 2015/10/16 05:40:32, tkent wrote:
> Please use |enum class| instead of |enum|, and move this to Element.cpp.

Done.

[tkent, 2015-10-16 07:18:50]

lgtm.  Thank you for fixing this.

[tkent, 2015-10-16 07:18:55]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2015-10-16 07:19:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1388903002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1388903002/140001

[commit-bot: I haz the power, 2015-10-16 08:21:23]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-16 08:22:43]

Patchset 8 (id:??) landed as
https://crrev.com/16643355966bb0e8f6b42516dacff90bbf80d3f8
Cr-Commit-Position: refs/heads/master@{#354473}