Summary: | Drop unnecessary Node::toInputElement() virtual function | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, kling, koivisto | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Chris Dumez
2015-10-19 14:34:43 PDT
Created attachment 263522 [details]
Patch
Created attachment 263541 [details]
Patch
Comment on attachment 263541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263541&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:675 > Node* node = this->node(); > - if (!node) > + if (!is<HTMLInputElement>(node)) > return false; > > - HTMLInputElement* inputElement = node->toInputElement(); > - if (!inputElement) > - return false; > - > - return inputElement->shouldAppearIndeterminate(); > + return downcast<HTMLInputElement>(*node).shouldAppearIndeterminate(); I might have written it: return is<HTMLInputElement>(node) && downcast<HTMLInputElement>(*node).shouldAppearIndeterminate(); I noticed you doing something similar in a function below. > Source/WebCore/html/shadow/SliderThumbElement.cpp:575 > // Only HTMLInputElement creates SliderThumbElement instances as its shadow nodes. > // So, shadowHost() must be an HTMLInputElement. > - Element* host = shadowHost(); > - return host ? host->toInputElement() : 0; > + auto* host = shadowHost(); > + return is<HTMLInputElement>(host) ? downcast<HTMLInputElement>(host) : nullptr; It’s peculiar that this code checks is<HTMLInputElement> yet has a comment stating that the check is not needed. I’m thinking that the comment is likely correct, and we could probably write: return downcast<HTMLInputElement>(shadowHost()); But obviously it’s less risky to leave the behavior unchanged. Created attachment 263545 [details]
Patch
Comment on attachment 263541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263541&action=review >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:675 >> + return downcast<HTMLInputElement>(*node).shouldAppearIndeterminate(); > > I might have written it: > > return is<HTMLInputElement>(node) && downcast<HTMLInputElement>(*node).shouldAppearIndeterminate(); > > I noticed you doing something similar in a function below. Done before landing. >> Source/WebCore/html/shadow/SliderThumbElement.cpp:575 >> + return is<HTMLInputElement>(host) ? downcast<HTMLInputElement>(host) : nullptr; > > It’s peculiar that this code checks is<HTMLInputElement> yet has a comment stating that the check is not needed. I’m thinking that the comment is likely correct, and we could probably write: > > return downcast<HTMLInputElement>(shadowHost()); > > But obviously it’s less risky to leave the behavior unchanged. I checked that this is only used by RangeInputType so I made the change. Comment on attachment 263545 [details] Patch Clearing flags on attachment: 263545 Committed r191327: <http://trac.webkit.org/changeset/191327> All reviewed patches have been landed. Closing bug. |