Bug 82682 – Clean up SelectorChecker::checkOneSelector()

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: CSS

[Luke Macpherson]

Clean up SelectorChecker::checkOneSelector()

[Luke Macpherson]

Created attachment 134703 [details]
Patch

[Luke Macpherson]

Motivated by ap's comments on https://bugs.webkit.org/show_bug.cgi?id=82402

[Build Bot]

Comment on attachment 134703 [details]
Patch

Attachment 134703 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12263009

[Luke Macpherson]

Created attachment 134706 [details]
Patch

[Alexey Proskuryakov]

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?

[Luke Macpherson]

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.

[Alexey Proskuryakov]

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.

[Luke Macpherson]

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.

[Alexey Proskuryakov]

Well yes, and this is exactly the wrong thing to do to buggy code, isn't it?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

Comment on attachment 134706 [details]
Patch

r- due to ap's comment.

[Luke Macpherson]

Created attachment 136574 [details]
Merge only

[Luke Macpherson]

(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.

[Gustavo Noronha (kov)]

Comment on attachment 136574 [details]
Merge only

Attachment 136574 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12382524

[Alexey Proskuryakov]

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.

[Luke Macpherson]

(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?

[Luke Macpherson]

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.

[Luke Macpherson]

Created attachment 136584 [details]
Patch

[Kentaro Hara]

(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?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Antti Koivisto]

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.

[Brent Fulgham]

This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.