Clean up SelectorChecker::checkOneSelector()
Created attachment 134703 [details] Patch
Motivated by ap's comments on https://bugs.webkit.org/show_bug.cgi?id=82402
Comment on attachment 134703 [details] Patch Attachment 134703 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12263009
Created attachment 134706 [details] Patch
Generally speaking, my inclination would be to fix a bug first, and clean up afterwards - it's dangerous to clean up code that's doing a wrong thing. Do you think that it's better to do it in a different order here?
One advantage of this patch is that it now has two assertions, one when falling though from an invalid enum entry, and one for a valid enum that is unhandled.That should make the cause of hitting the assertion easier to track down.
We have a repro case that can be just opened in a debugger - why is there a need to rely on indirect channels to gather information? I'd understand if this assertion was only seen on buildbots, and wasn't reproducible locally.
This patch brings the code into line with webkit style. A pleasant side effect is that it will make the code slightly easier to debug. I don't know what other justification is needed.
Well yes, and this is exactly the wrong thing to do to buggy code, isn't it?
Comment on attachment 134706 [details] Patch Attachment 134706 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12253137 New failing tests: fast/css/pseudo-empty-dynamic-empty.html css3/selectors3/html/css3-modsel-73b.html css3/selectors3/xml/css3-modsel-73.xml css3/flexbox/multiline-shrink-to-fit.html css3/selectors3/xhtml/css3-modsel-73.xml css3/selectors3/xml/css3-modsel-146a.xml fast/css/nth-child-dynamic.html css3/selectors3/html/css3-modsel-73.html fast/dom/HTMLMeterElement/meter-appearances-capacity.html css3/selectors3/xml/css3-modsel-73b.xml css3/selectors3/xhtml/css3-modsel-146a.xml css3/selectors3/html/css3-modsel-28.html fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html css3/flexbox/multiline-reverse-wrap-baseline.html css3/flexbox/multiline-line-pack.html fast/css/css3-space-in-nth-and-lang.html css3/selectors3/xml/css3-modsel-28.xml fast/css/css3-nth-child.html css3/selectors3/xhtml/css3-modsel-28.xml css3/selectors3/xhtml/css3-modsel-73b.xml
Created attachment 134740 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 134706 [details] Patch r- due to ap's comment.
Created attachment 136574 [details] Merge only
(In reply to comment #12) > (From update of attachment 134706 [details]) > r- due to ap's comment. AP is wrong. He can't point to any known bugs in the code that I'm actually changing here, and we can't stop all work on webkit because there might be a bug somewhere else in the codebase.
Comment on attachment 136574 [details] Merge only Attachment 136574 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12382524
Comment on attachment 136574 [details] Merge only It's not "might be a bug". There is a known bug, but you complicate fixing it with useless code churn. This is not a way to deal with bugs.
(In reply to comment #16) > (From update of attachment 136574 [details]) > It's not "might be a bug". There is a known bug, but you complicate fixing it with useless code churn. This is not a way to deal with bugs. I think it's important to distinguish between a bug in this code, and a bug in a caller of this code. I don't think I've seen any evidence that there is a bug here. Are you worried that stack traces will no longer match up in some internal bug reporting tool or something?
Just want to make sure it's 100% clear to any onlookers - this code cleanup is not intended to fix https://bugs.webkit.org/show_bug.cgi?id=82402 . All it does is replace the default case with a complete list of enum values, and use early returns instead of breaks in the switch.
Created attachment 136584 [details] Patch
(In reply to comment #18) > Just want to make sure it's 100% clear to any onlookers - this code cleanup is not intended to fix https://bugs.webkit.org/show_bug.cgi?id=82402 . I think this is a problem. As ap pointed out, we do not want to refactor the code before fixing a known bug (e.g. the refactoring might "hide" the bug), unless the refactoring helps us fix the bug. Would you dig into the bug first before refactoring?
Comment on attachment 136584 [details] Patch Attachment 136584 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12386306 New failing tests: fast/css/pseudo-empty-dynamic-empty.html css3/selectors3/html/css3-modsel-73b.html css3/selectors3/xml/css3-modsel-73.xml css3/flexbox/multiline-shrink-to-fit.html css3/selectors3/xhtml/css3-modsel-73.xml css3/selectors3/xml/css3-modsel-146a.xml fast/css/nth-child-dynamic.html css3/selectors3/html/css3-modsel-73.html fast/dom/HTMLMeterElement/meter-appearances-capacity.html css3/selectors3/xml/css3-modsel-73b.xml css3/selectors3/xhtml/css3-modsel-146a.xml css3/selectors3/html/css3-modsel-28.html fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html css3/flexbox/multiline-reverse-wrap-baseline.html css3/flexbox/multiline-line-pack.html fast/css/css3-space-in-nth-and-lang.html css3/selectors3/xml/css3-modsel-28.xml fast/css/css3-nth-child.html css3/selectors3/xhtml/css3-modsel-28.xml css3/selectors3/xhtml/css3-modsel-73b.xml
Created attachment 136653 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 136584 [details] Patch Alexey is right. There is an actual bug to fix. Low-value cleanup patches like this, without context (a bug fix or a new feature) are just noise.
This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.