Bug 128820 – [GTK] Fix hitting hasClass() assertion in debug with the new JS media controls

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[GTK] Fix assertion hit in debug with the new JS media controls

[Xabier Rodríguez Calvar]

Created attachment 224223 [details]
Patch

[Martin Robinson]

Comment on attachment 224223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224223&action=review

> Source/WebCore/ChangeLog:4
> +        [GTK] Fix assertion hit in debug with the new JS media controls
> +        https://bugs.webkit.org/show_bug.cgi?id=128820

What assertion is hit?

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * platform/gtk/RenderThemeGtk.cpp:
> +        (WebCore::nodeHasClass): Check for hasClass() in order not to hit

This probably needs a test.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:121
> +    if (node->isElementNode()) {
> +        Element* element = toElement(node);
> +        if (element->hasClass())
> +            return element->classNames().contains(className);
> +    }
> +    return false;

Early returns would be better.

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> > Source/WebCore/ChangeLog:4
> > +        [GTK] Fix assertion hit in debug with the new JS media controls
> > +        https://bugs.webkit.org/show_bug.cgi?id=128820
> 
> What assertion is hit?

hasClass(), it is said in the changes explanation. Do you want me to be more explicit? Where exactly?

> > Source/WebCore/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * platform/gtk/RenderThemeGtk.cpp:
> > +        (WebCore::nodeHasClass): Check for hasClass() in order not to hit
> 
> This probably needs a test.

There were quite many tests crashing because of this in debug mode. Actually the bot has an early exit because of those crashes:

Exiting early after 28 crashes and 22 timeouts. 27309 tests run.
121 failures
9 new passes
11 missing results

The results can be seen at:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20WK1/r164101%20(6827)/results.html

I think the functionality is quite well covered with the current test set. I could add the info to the changelog instead. Do you still want me to add the test?

> > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:121
> > +    if (node->isElementNode()) {
> > +        Element* element = toElement(node);
> > +        if (element->hasClass())
> > +            return element->classNames().contains(className);
> > +    }
> > +    return false;
> 
> Early returns would be better.

Ok. I'll change this.

[Xabier Rodríguez Calvar]

Created attachment 224257 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 224257 [details]
Patch

Clearing flags on attachment: 224257

Committed r164277: <http://trac.webkit.org/changeset/164277>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Piotr Grad]

*** Bug 128904 has been marked as a duplicate of this bug. ***