Summary: | [GTK] Fix hitting hasClass() assertion in debug with the new JS media controls | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xabier Rodríguez Calvar <calvaris> | ||||||
Component: | New Bugs | Assignee: | Xabier Rodríguez Calvar <calvaris> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | b.long, commit-queue, mrobinson, piotr.grad | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Xabier Rodríguez Calvar
2014-02-14 09:11:52 PST
Created attachment 224223 [details]
Patch
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. (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. Created attachment 224257 [details]
Patch
Comment on attachment 224257 [details] Patch Clearing flags on attachment: 224257 Committed r164277: <http://trac.webkit.org/changeset/164277> All reviewed patches have been landed. Closing bug. *** Bug 128904 has been marked as a duplicate of this bug. *** |