[webkit-reviews] review denied: [Bug 24021] pseudo-element styles not accessible / retrievable via DOM methods : [Attachment 50741] First steps towards exposing the standard pseudo-elements through getComputedStyle (refactor maps among Strings, PseudoTypes, and PseudoIds)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 16 09:21:22 PDT 2010
Darin Adler <darin at apple.com> has denied Jessie Berlin <jberlin at webkit.org>'s
request for review:
Bug 24021: pseudo-element styles not accessible / retrievable via DOM methods
https://bugs.webkit.org/show_bug.cgi?id=24021
Attachment 50741: First steps towards exposing the standard pseudo-elements
through getComputedStyle (refactor maps among Strings, PseudoTypes, and
PseudoIds)
https://bugs.webkit.org/attachment.cgi?id=50741&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
Looks really good!
> -CSSComputedStyleDeclaration::CSSComputedStyleDeclaration(PassRefPtr<Node> n)
> +CSSComputedStyleDeclaration::CSSComputedStyleDeclaration(PassRefPtr<Node> n,
const String& pseudoElt)
The two things I don't like about the name "pseudoElt".
1) It has an abbreviation, "elt", in the name.
2) The string is not a pseudo element. It's the name of a pseudo element or
a specifier for a pseudo element. So it's not ideal to name it "pseudo
element". Same comment about m_pseudoElt.
> + unsigned startPseudoEltName = 0;
> + if (pseudoElt.startsWith("::"))
> + startPseudoEltName = 2;
> + else if (pseudoElt.startsWith(":"))
> + startPseudoEltName = 1;
There is a slightly more efficient way to do this. Since String indexing
already does range checking, you can just check characters:
unsigned nameStart = pseudoElementSpecifier[0] == ':' ?
(pseudoElementSpecifier[1] == ':' ? 2 : 1) : 0;
You could also do the same thing with if statements. I think it's slightly
better to use array indexing and characters than startsWith and C-style
strings.
> + m_pseudoElt =
CSSSelector::pseudoIdFromPseudoType(CSSSelector::pseudoTypeFromString(
> + AtomicString(pseudoElt.substring(startPseudoEltName))));
> +
> }
Extra blank line here.
> if (renderer && hasCompositedLayer(renderer) &&
AnimationController::supportsAcceleratedAnimationOfProperty(static_cast<CSSProp
ertyID>(propertyID)))
> style =
renderer->animation()->getAnimatedStyleForRenderer(renderer);
> else
> - style = node->computedStyle();
> + style = node->computedStyle(m_pseudoElt);
Is it really OK to ignore m_psuedoElt on the AnimationController side of this
if statement? If not, could we construct a test case to demonstrate why that's
not OK?
> + static PseudoId getPseudoId(const AtomicString& value);
In WebKit code, we reserve "get" for functions with out arguments. We don't use
it for getters that return a value. Those we name with nouns or adjectives
instead. The name "value" doesn't add much here. I think we need to get clearer
on our terminology. What are the strings that specify different pseudo elements
called?
Then I noticed this declaration is for a function that you actually didn't end
up defining. Oops! Please remove it.
> +PseudoId CSSSelector::pseudoIdFromPseudoType(PseudoType pseudoType)
I think the local variable can just have the name "type". Once you've said
pseudo in pseudoId, I don't think you have to repeat it.
> + case PseudoUnknown:
> + case PseudoNotParsed:
> + default:
> + return NOPSEUDO;
> + }
It would be better to omit the default case and have an ASSERT_NOT_REACHED
outside the switch statement for two reasons:
1) If we do that, gcc will warn if we add a new pseudo type and forget to
add a case to the switch statement.
2) If the passed-in type is an illegal value, then we'll see an assert at
runtime, which could help us track down a bug.
We can still return NOPSEUDO and so have the same behavior in a release build.
> + static HashMap<AtomicStringImpl*, PseudoType>* valueToPseudoType = 0;
> + if (!valueToPseudoType) {
> + valueToPseudoType = new HashMap<AtomicStringImpl*, PseudoType>;
> + valueToPseudoType->set(active.impl(), PseudoActive);
> + valueToPseudoType->set(after.impl(), PseudoAfter);
> + valueToPseudoType->set(anyLink.impl(), PseudoAnyLink);
> + valueToPseudoType->set(autofill.impl(), PseudoAutofill);
> + valueToPseudoType->set(before.impl(), PseudoBefore);
> + valueToPseudoType->set(checked.impl(), PseudoChecked);
> + valueToPseudoType->set(fileUploadButton.impl(),
PseudoFileUploadButton);
> + valueToPseudoType->set(defaultString.impl(), PseudoDefault);
> + valueToPseudoType->set(disabled.impl(), PseudoDisabled);
> + valueToPseudoType->set(readOnly.impl(), PseudoReadOnly);
> + valueToPseudoType->set(readWrite.impl(), PseudoReadWrite);
> + valueToPseudoType->set(valid.impl(), PseudoValid);
> + valueToPseudoType->set(invalid.impl(), PseudoInvalid);
> + valueToPseudoType->set(drag.impl(), PseudoDrag);
> + valueToPseudoType->set(dragAlias.impl(), PseudoDrag);
> + valueToPseudoType->set(enabled.impl(), PseudoEnabled);
> + valueToPseudoType->set(empty.impl(), PseudoEmpty);
> + valueToPseudoType->set(firstChild.impl(), PseudoFirstChild);
> + valueToPseudoType->set(fullPageMedia.impl(), PseudoFullPageMedia);
> +#if ENABLE(DATALIST)
> + valueToPseudoType->set(inputListButton.impl(),
PseudoInputListButton);
> +#endif
> + valueToPseudoType->set(inputPlaceholder.impl(),
PseudoInputPlaceholder);
> + valueToPseudoType->set(lastChild.impl(), PseudoLastChild);
> + valueToPseudoType->set(lastOfType.impl(), PseudoLastOfType);
> + valueToPseudoType->set(onlyChild.impl(), PseudoOnlyChild);
> + valueToPseudoType->set(onlyOfType.impl(), PseudoOnlyOfType);
> + valueToPseudoType->set(firstLetter.impl(), PseudoFirstLetter);
> + valueToPseudoType->set(firstLine.impl(), PseudoFirstLine);
> + valueToPseudoType->set(firstOfType.impl(), PseudoFirstOfType);
> + valueToPseudoType->set(focus.impl(), PseudoFocus);
> + valueToPseudoType->set(hover.impl(), PseudoHover);
> + valueToPseudoType->set(indeterminate.impl(), PseudoIndeterminate);
> + valueToPseudoType->set(innerSpinButton.impl(),
PseudoInnerSpinButton);
> + valueToPseudoType->set(link.impl(), PseudoLink);
> + valueToPseudoType->set(lang.impl(), PseudoLang);
> + valueToPseudoType->set(mediaControlsPanel.impl(),
PseudoMediaControlsPanel);
> + valueToPseudoType->set(mediaControlsMuteButton.impl(),
PseudoMediaControlsMuteButton);
> + valueToPseudoType->set(mediaControlsPlayButton.impl(),
PseudoMediaControlsPlayButton);
> + valueToPseudoType->set(mediaControlsCurrentTimeDisplay.impl(),
PseudoMediaControlsCurrentTimeDisplay);
> + valueToPseudoType->set(mediaControlsTimeRemainingDisplay.impl(),
PseudoMediaControlsTimeRemainingDisplay);
> + valueToPseudoType->set(mediaControlsTimeline.impl(),
PseudoMediaControlsTimeline);
> + valueToPseudoType->set(mediaControlsVolumeSlider.impl(),
PseudoMediaControlsVolumeSlider);
> + valueToPseudoType->set(mediaControlsSeekBackButton.impl(),
PseudoMediaControlsSeekBackButton);
> + valueToPseudoType->set(mediaControlsSeekForwardButton.impl(),
PseudoMediaControlsSeekForwardButton);
> + valueToPseudoType->set(mediaControlsRewindButton.impl(),
PseudoMediaControlsRewindButton);
> + valueToPseudoType->set(mediaControlsReturnToRealtimeButton.impl(),
PseudoMediaControlsReturnToRealtimeButton);
> +
valueToPseudoType->set(mediaControlsToggleClosedCaptionsButton.impl(),
PseudoMediaControlsToggleClosedCaptions);
> + valueToPseudoType->set(mediaControlsStatusDisplay.impl(),
PseudoMediaControlsStatusDisplay);
> + valueToPseudoType->set(mediaControlsFullscreenButton.impl(),
PseudoMediaControlsFullscreenButton);
> + valueToPseudoType->set(mediaControlsTimelineContainer.impl(),
PseudoMediaControlsTimelineContainer);
> + valueToPseudoType->set(mediaControlsVolumeSliderContainer.impl(),
PseudoMediaControlsVolumeSliderContainer);
> + valueToPseudoType->set(notStr.impl(), PseudoNot);
> + valueToPseudoType->set(nthChild.impl(), PseudoNthChild);
> + valueToPseudoType->set(nthOfType.impl(), PseudoNthOfType);
> + valueToPseudoType->set(nthLastChild.impl(), PseudoNthLastChild);
> + valueToPseudoType->set(nthLastOfType.impl(), PseudoNthLastOfType);
> + valueToPseudoType->set(outerSpinButton.impl(),
PseudoOuterSpinButton);
> + valueToPseudoType->set(root.impl(), PseudoRoot);
> + valueToPseudoType->set(windowInactive.impl(), PseudoWindowInactive);
> + valueToPseudoType->set(decrement.impl(), PseudoDecrement);
> + valueToPseudoType->set(increment.impl(), PseudoIncrement);
> + valueToPseudoType->set(start.impl(), PseudoStart);
> + valueToPseudoType->set(end.impl(), PseudoEnd);
> + valueToPseudoType->set(horizontal.impl(), PseudoHorizontal);
> + valueToPseudoType->set(vertical.impl(), PseudoVertical);
> + valueToPseudoType->set(doubleButton.impl(), PseudoDoubleButton);
> + valueToPseudoType->set(singleButton.impl(), PseudoSingleButton);
> + valueToPseudoType->set(noButton.impl(), PseudoNoButton);
> + valueToPseudoType->set(optional.impl(), PseudoOptional);
> + valueToPseudoType->set(required.impl(), PseudoRequired);
> + valueToPseudoType->set(resizer.impl(), PseudoResizer);
> + valueToPseudoType->set(scrollbar.impl(), PseudoScrollbar);
> + valueToPseudoType->set(scrollbarButton.impl(),
PseudoScrollbarButton);
> + valueToPseudoType->set(scrollbarCorner.impl(),
PseudoScrollbarCorner);
> + valueToPseudoType->set(scrollbarThumb.impl(), PseudoScrollbarThumb);
> + valueToPseudoType->set(scrollbarTrack.impl(), PseudoScrollbarTrack);
> + valueToPseudoType->set(scrollbarTrackPiece.impl(),
PseudoScrollbarTrackPiece);
> + valueToPseudoType->set(cornerPresent.impl(), PseudoCornerPresent);
> + valueToPseudoType->set(searchCancelButton.impl(),
PseudoSearchCancelButton);
> + valueToPseudoType->set(searchDecoration.impl(),
PseudoSearchDecoration);
> + valueToPseudoType->set(searchResultsDecoration.impl(),
PseudoSearchResultsDecoration);
> + valueToPseudoType->set(searchResultsButton.impl(),
PseudoSearchResultsButton);
> + valueToPseudoType->set(selection.impl(), PseudoSelection);
> + valueToPseudoType->set(sliderThumb.impl(), PseudoSliderThumb);
> + valueToPseudoType->set(target.impl(), PseudoTarget);
> + valueToPseudoType->set(visited.impl(), PseudoVisited);
> + }
I sugget putting the code that populates the map into a separate function. It’s
inelegant to have it here in the middle of the function that does the work
after the map is built.
> + if (valueToPseudoType->contains(value.impl()))
> + return valueToPseudoType->get(value.impl());
> + return PseudoUnknown;
This does two hash map lookups but should only do one. Since PseudoUnknown is
not a 0 value, you have to write it with iterators:
HashMap<AtomicStringImpl*, PseudoType>::iterator slot =
valueToPseudoType->get(value.impl());
return slot == valueToPseudoType->end() ? PseudoUnknown : slot->second;
Not as easy to read, but twice as fast.
> + default:
> + break;
> + }
Again, I think it's better to not have a default here, and instead list the
other pseudo types with explicit cases so we get warnings if we forget to add a
new type here.
> + static PseudoType pseudoTypeFromString(const AtomicString&);
> + static PseudoId pseudoIdFromPseudoType(PseudoType);
Given that this is C++ and we support overloading, I'm not sure these function
names need the "from type" suffixes. I don't have better names to suggest right
away. For example pseudoTypeFromString might be better named something like
parsePseudoTypeName. And it might be OK to name pseudoIdFromPseudoType just
pseudoId.
> + PseudoId pseudoId =
CSSSelector::pseudoIdFromPseudoType(sel->pseudoType());
> + if (pseudoId == FIRST_LETTER)
> + if (Document* doc = e->document())
> + doc->setUsesFirstLetterRules(true);
Please use "document" instead of "doc".
Please use braces around multiple line if bodies. The outer one here is
multiple lines long and so needs braces.
> - virtual RenderStyle* computedStyle();
> + virtual RenderStyle* computedStyle(PseudoId pseudoElt = NOPSEUDO);
I suggest omitting the argument name here.
> - virtual RenderStyle* computedStyle();
> + virtual RenderStyle* computedStyle(PseudoId pseudoElt = NOPSEUDO);
And here.
If it's at all common to call computedStyle on Element, we could speed things
up by making it so that it's a non-virtual function call. The technique used
for functions like prefix can be used for computedStyle to make a virtual one
in the Node class and a non-virtual one in the Element class that have the same
name and look the same at the call site, yet more efficient in the class
Element.
> -PassRefPtr<CSSStyleDeclaration> DOMWindow::getComputedStyle(Element* elt,
const String&) const
> +PassRefPtr<CSSStyleDeclaration> DOMWindow::getComputedStyle(Element* elt,
const String& pseudoElt) const
> {
> if (!elt)
> return 0;
>
> - // FIXME: This needs take pseudo elements into account.
> - return computedStyle(elt);
> + return computedStyle(elt, pseudoElt);
> }
As long as you're touching this it would be good to get rid of the "elt"
abbreviation. See also my comments above about the pseudoElt name.
Most of my comments are minor, but a few are important enough that I think I'll
say review-
More information about the webkit-reviews
mailing list