Bug 24021 - pseudo-element styles not accessible / retrievable via DOM methods
Summary: pseudo-element styles not accessible / retrievable via DOM methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Jessie Berlin
URL: http://aetherworld.org/getComputedSty...
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2009-02-18 23:58 PST by Faruk Ates
Modified: 2010-04-19 18:01 PDT (History)
6 users (show)

See Also:


Attachments
Test case with 2 alerts showing style of element and pseudo-element (676 bytes, text/html)
2009-02-19 00:00 PST, Faruk Ates
no flags Details
A tentative idea for a fix that will expose the standard pseudo-elements through getComputedStyle (21.97 KB, patch)
2010-01-02 18:35 PST, Jessie Berlin
no flags Details | Formatted Diff | Diff
First steps towards exposing the standard pseudo-elements through getComputedStyle (24.59 KB, patch)
2010-03-06 16:28 PST, Jessie Berlin
eric: review-
Details | Formatted Diff | Diff
First steps towards exposing the standard pseudo-elements through getComputedStyle (refactor maps among Strings, PseudoTypes, and PseudoIds) (deleted)
2010-03-15 14:36 PDT, Jessie Berlin
darin: review-
Details | Formatted Diff | Diff
First steps towards exposing the standard pseudo-elements through getComputedStyle (addressed issues pointed out by Darin) (55.57 KB, patch)
2010-03-21 11:02 PDT, Jessie Berlin
no flags Details | Formatted Diff | Diff
Remove the space that was causing the test to fail on the Chromium bots. Also remove the platform specific results that were landed in the meantime. (7.85 KB, patch)
2010-04-19 17:34 PDT, Jessie Berlin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Faruk Ates 2009-02-18 23:58:47 PST
Applying styles to :before and :after pseudo-elements works, but retrieving said styles via DOM methods does not. There's no way to access them.

window.getComputedStyle(elem, pseudoElt); should allow you to specify the pseudo-element as the second param, but it has no effect in Webkit.
Comment 1 Faruk Ates 2009-02-19 00:00:06 PST
Created attachment 27779 [details]
Test case with 2 alerts showing style of element and pseudo-element

Testcase provided by Alexander Graf / http://twitter.com/aetherworld
Comment 2 Jessie Berlin 2010-01-02 18:35:40 PST
Created attachment 45754 [details]
A tentative idea for a fix that will expose the standard pseudo-elements through getComputedStyle

I would highly appreciate feedback on the general approach.

 As I write in the ChangeLog, there are several parts of this patch that I am mildly uncomfortable with:

        1) The need to duplicate the pseudo style string to pseudo style constant
           mapping that is done in CSSSelector::extractPseudoType(). I was unable
           to find a mapping between PseudoType and PseudoId and decided it made
           mores sense to map from the string itself to the PseudoId
        2) This patch won't give the desired results for the -webkit-scrollbar
           pseudo-elements.
        3) I am not sure that going directly to the renderer is correct. Looking
           in Element::computedStyle(), there seems to be an option for elements
           like HTMLOptionElement to not have renderers. I am not exactly sure
           why this is.
Comment 3 WebKit Review Bot 2010-01-02 18:37:24 PST
Attachment 45754 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:43:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1
Comment 4 mitz 2010-01-02 19:05:04 PST
Comment on attachment 45754 [details]
A tentative idea for a fix that will expose the standard pseudo-elements through getComputedStyle

>      if (renderer && hasCompositedLayer(renderer) && AnimationController::supportsAcceleratedAnimationOfProperty(static_cast<CSSPropertyID>(propertyID)))
>          style = renderer->animation()->getAnimatedStyleForRenderer(renderer);
> -    else
> -       style = node->computedStyle();
> +    else {
> +        style = node->computedStyle();
> +        if (m_pseudoStyle != NOPSEUDO)
> +            style = node->renderer()->getCachedPseudoStyle(m_pseudoStyle);

Note that this code accessing node->renderer() is in the else clause of “if (renderer && …”. One common way that the renderer can be null is if the node has (or descends from an element that has) 'display: none;'. It looks like in the pseudo-element case, you are calling Node::computedStyle() only to throw away the result. Is there a side effect to calling computedStyle() that you are trying to achieve?

For several properties, the code that follows in this method gets the computed value from the renderer, if there is one (e.g. CSSPropertyHeight). In order for this code to return the correct result, you should set 'renderer' to the renderer of the pseudo-element in question. For example, consider

<style>
    div { height: 100px; }
    div:before { display: block; height: 20px; content: "Before"; }
</style>
<div id="target"></div>

You want getComputedStyle(document.getElementById("target"), ":before").height to be "20px", not "100px".

> +    DEFINE_STATIC_LOCAL(AtomicString, after, (":after"));> +    DEFINE_STATIC_LOCAL(AtomicString, sliderThumb, (":-webkit-slider-thumb"));
> +
> +    if (value == after)
> +        return AFTER;> +    if (value == sliderThumb)
> +        return SLIDER_THUMB;

Maybe there is a way to leverage existing code to do this, but if there isn’t, perhaps a HashMap<AtomicStringImpl*, PseudoId> would be better?
Comment 5 Jessie Berlin 2010-01-07 16:05:27 PST
(In reply to comment #4)
> (From update of attachment 45754 [details])
> >      if (renderer && hasCompositedLayer(renderer) && AnimationController::supportsAcceleratedAnimationOfProperty(static_cast<CSSPropertyID>(propertyID)))
> >          style = renderer->animation()->getAnimatedStyleForRenderer(renderer);
> > -    else
> > -       style = node->computedStyle();
> > +    else {
> > +        style = node->computedStyle();
> > +        if (m_pseudoStyle != NOPSEUDO)
> > +            style = node->renderer()->getCachedPseudoStyle(m_pseudoStyle);
> 
> Note that this code accessing node->renderer() is in the else clause of “if
> (renderer && …”. One common way that the renderer can be null is if the node
> has (or descends from an element that has) 'display: none;'.

I wrongly assumed that getComputedStyle would not need to work when display was none. Rethinking that.

> It looks like in
> the pseudo-element case, you are calling Node::computedStyle() only to throw
> away the result. Is there a side effect to calling computedStyle() that you are
> trying to achieve?

No, calling it only to throw away the result was a mistake. It should have been guarded with an if statement.

> 
> For several properties, the code that follows in this method gets the computed
> value from the renderer, if there is one (e.g. CSSPropertyHeight). In order for
> this code to return the correct result, you should set 'renderer' to the
> renderer of the pseudo-element in question. 

Is there a way to access the 'renderer' of the pseudo-element? Since there aren't actual renderers for the pseudo-elements (correct me if I am wrong) why would the RenderStyle that is returned by the getCachedPseudoStyle or getUncachedPseudosStyle for the RenderObject of the element not be the correct style?

> For example, consider
> 
> <style>
>     div { height: 100px; }
>     div:before { display: block; height: 20px; content: "Before"; }
> </style>
> <div id="target"></div>
> 
> You want getComputedStyle(document.getElementById("target"), ":before").height
> to be "20px", not "100px".
> 

Question about this. The method I was using doesn't work for height, but it does work for color (set color to blue in div { ... } and red in div:before { ... } and the getComputedStyle(document.getElementById("target"), ":before").color will report blue. Why would there be RenderStyle that has the color of the psuedo-element but the height of the element?

> > +    DEFINE_STATIC_LOCAL(AtomicString, after, (":after"));
> …
> > +    DEFINE_STATIC_LOCAL(AtomicString, sliderThumb, (":-webkit-slider-thumb"));
> > +
> > +    if (value == after)
> > +        return AFTER;
> …
> > +    if (value == sliderThumb)
> > +        return SLIDER_THUMB;
> 
> Maybe there is a way to leverage existing code to do this, but if there isn’t,
> perhaps a HashMap<AtomicStringImpl*, PseudoId> would be better?

Ok.
Comment 6 Jessie Berlin 2010-01-07 16:07:27 PST
Comment on attachment 45754 [details]
A tentative idea for a fix that will expose the standard pseudo-elements through getComputedStyle

(rethinking approach)
Comment 7 mitz 2010-01-09 20:41:47 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 45754 [details] [details])
> > >      if (renderer && hasCompositedLayer(renderer) && AnimationController::supportsAcceleratedAnimationOfProperty(static_cast<CSSPropertyID>(propertyID)))
> > >          style = renderer->animation()->getAnimatedStyleForRenderer(renderer);
> > > -    else
> > > -       style = node->computedStyle();
> > > +    else {
> > > +        style = node->computedStyle();
> > > +        if (m_pseudoStyle != NOPSEUDO)
> > > +            style = node->renderer()->getCachedPseudoStyle(m_pseudoStyle);
> > 
> > Note that this code accessing node->renderer() is in the else clause of “if
> > (renderer && …”. One common way that the renderer can be null is if the node
> > has (or descends from an element that has) 'display: none;'.
> 
> I wrongly assumed that getComputedStyle would not need to work when display was
> none. Rethinking that.

For actual elements, it works (there is no renderer, but style resolution is still possible; this was required for compatibility with Firefox). For pseudo-elements, you can start by implementing it only for displayed elements. Maybe see what other browsers do in this case.

> > For several properties, the code that follows in this method gets the computed
> > value from the renderer, if there is one (e.g. CSSPropertyHeight). In order for
> > this code to return the correct result, you should set 'renderer' to the
> > renderer of the pseudo-element in question. 
> 
> Is there a way to access the 'renderer' of the pseudo-element?

There is no general mechanism for doing that. For each pseudo-element, the relevant (actual element) renderer obviously knows how to get to the pseudo-element renderer, if there is one. For :before and :after content, for example, or for :-webkit-file-upload-button, there is one. For :first-line, there isn’t.

> Since there
> aren't actual renderers for the pseudo-elements (correct me if I am wrong) why
> would the RenderStyle that is returned by the getCachedPseudoStyle or
> getUncachedPseudosStyle for the RenderObject of the element not be the correct
> style?

The difference between returning values from the RenderStyle and returning values from the renderer is only for length properties that can be 'auto', percentages or relative; in that case, only the renderer, having been laid out, knows the actual computed pixel value. In my example from comment #5, if the rule for div:before specified "height: 20%;", you would still want the computed style to be "20px", not "20%".

> > For example, consider
> > 
> > <style>
> >     div { height: 100px; }
> >     div:before { display: block; height: 20px; content: "Before"; }
> > </style>
> > <div id="target"></div>
> > 
> > You want getComputedStyle(document.getElementById("target"), ":before").height
> > to be "20px", not "100px".
> > 
> 
> Question about this. The method I was using doesn't work for height, but it
> does work for color (set color to blue in div { ... } and red in div:before {
> ... } and the getComputedStyle(document.getElementById("target"),
> ":before").color will report blue. Why would there be RenderStyle that has the
> color of the psuedo-element but the height of the element?

The RenderStyle would have the height of the pseudo-element, as a (possibly auto, relative or percent) length, but in your patch, we would return a pixel value taken from the 'renderer' variable, which was the renderer for the (actual) element, and hence it would be "100px".

Just setting renderer to 0 (and thus returning all values from the RenderStyle) in the pseudo-element case would be a good start. You could figure out how to get the renderer for the various pseudo-elements later.
Comment 8 Jessie Berlin 2010-01-22 07:25:11 PST
Sorry for the late reply on this.

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 45754 [details] [details] [details])
> > > >      if (renderer && hasCompositedLayer(renderer) && AnimationController::supportsAcceleratedAnimationOfProperty(static_cast<CSSPropertyID>(propertyID)))
> > > >          style = renderer->animation()->getAnimatedStyleForRenderer(renderer);
> > > > -    else
> > > > -       style = node->computedStyle();
> > > > +    else {
> > > > +        style = node->computedStyle();
> > > > +        if (m_pseudoStyle != NOPSEUDO)
> > > > +            style = node->renderer()->getCachedPseudoStyle(m_pseudoStyle);
> > > 
> > > Note that this code accessing node->renderer() is in the else clause of “if
> > > (renderer && …”. One common way that the renderer can be null is if the node
> > > has (or descends from an element that has) 'display: none;'.
> > 
> > I wrongly assumed that getComputedStyle would not need to work when display was
> > none. Rethinking that.
> 
> For actual elements, it works (there is no renderer, but style resolution is
> still possible; this was required for compatibility with Firefox). For
> pseudo-elements, you can start by implementing it only for displayed elements.
> Maybe see what other browsers do in this case.

I will take your advice here and start by trying to implement it only for displayed elements.

> 
> > > For several properties, the code that follows in this method gets the computed
> > > value from the renderer, if there is one (e.g. CSSPropertyHeight). In order for
> > > this code to return the correct result, you should set 'renderer' to the
> > > renderer of the pseudo-element in question. 
> > 
> > Is there a way to access the 'renderer' of the pseudo-element?
> 
> There is no general mechanism for doing that. For each pseudo-element, the
> relevant (actual element) renderer obviously knows how to get to the
> pseudo-element renderer, if there is one. For :before and :after content, for
> example, or for :-webkit-file-upload-button, there is one. For :first-line,
> there isn’t.

I can see throughout the code that there are these special cases for FIRST_LINE and FIRST_LINE_INHERITED. Why is this?

> 
> > Since there
> > aren't actual renderers for the pseudo-elements (correct me if I am wrong) why
> > would the RenderStyle that is returned by the getCachedPseudoStyle or
> > getUncachedPseudosStyle for the RenderObject of the element not be the correct
> > style?
> 
> The difference between returning values from the RenderStyle and returning
> values from the renderer is only for length properties that can be 'auto',
> percentages or relative; in that case, only the renderer, having been laid out,
> knows the actual computed pixel value. In my example from comment #5, if the
> rule for div:before specified "height: 20%;", you would still want the computed
> style to be "20px", not "20%".

Interestingly, Firefox gets this incorrect as well (returns 100px in the case below).

> 
> > > For example, consider
> > > 
> > > <style>
> > >     div { height: 100px; }
> > >     div:before { display: block; height: 20px; content: "Before"; }
> > > </style>
> > > <div id="target"></div>
> > > 
> > > You want getComputedStyle(document.getElementById("target"), ":before").height
> > > to be "20px", not "100px".
> > > 
> > 
> > Question about this. The method I was using doesn't work for height, but it
> > does work for color (set color to blue in div { ... } and red in div:before {
> > ... } and the getComputedStyle(document.getElementById("target"),
> > ":before").color will report blue. Why would there be RenderStyle that has the
> > color of the psuedo-element but the height of the element?
> 
> The RenderStyle would have the height of the pseudo-element, as a (possibly
> auto, relative or percent) length, but in your patch, we would return a pixel
> value taken from the 'renderer' variable, which was the renderer for the
> (actual) element, and hence it would be "100px".
> 
> Just setting renderer to 0 (and thus returning all values from the RenderStyle)
> in the pseudo-element case would be a good start. You could figure out how to
> get the renderer for the various pseudo-elements later.

When you mean setting the renderer to 0, are you referring to simply getting the RenderStyle from the Element and asking that RenderStyle for the cached pseudo style? That still gets the height wrong.
Comment 9 Jessie Berlin 2010-03-06 16:28:49 PST
Created attachment 50157 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle

Following Dan's suggestion in comment #7 to start by just using the cached Pseudo Style returned from the RenderStyle of the element on which the pseudo-element is defined.

I am currently investigating how to get the renderer for the pseudo-element so as to get the 'length' properties correct. Is there any point at which a render style is created for a pseudo-element with the correct 'length' properties? Even explicitly using the renderer()->firstLineStyle() does not return a RenderStyle with the correct 'length' properties (testing with height), so I am wondering if the correct RenderStyle is ever created ...

Preliminary (but not thorough) testing indicates that this patch returns the same 'length' properties as Firefox does.

For the ':selection' pseudo-element, getting the cached Pseudo Style directly from the renderer of the element on which the pseudo-element is defined seems to work for properties like color, but still does not work for 'length' properties.
Comment 10 mitz 2010-03-13 14:23:48 PST
Comment on attachment 50157 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle

I think it would be better to factor out existing code that maps from the pseudo-element strings to PseudoTypes (you’ll just need to strip the one or two leading colons), and similarly share the code that maps PseudoType to PseudoId in checkOneSelector.

Seems like CSSComputedStyleDeclaration::getPseudoId() should not be exposed outside CSSComputedStyleDeclaration.cpp. The one call site can just pass the string, and the CSSComputedStyleDeclaration constructor could do the mapping (using the aforementioned refactored code) internally.
Comment 11 Eric Seidel (no email) 2010-03-15 13:01:01 PDT
Comment on attachment 50157 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle

Mitz's comment sounds like an r-.  Please feel free to reset the flags if I've misinterpreted.
Comment 12 Jessie Berlin 2010-03-15 14:36:47 PDT
Created attachment 50741 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle (refactor maps among Strings, PseudoTypes, and PseudoIds)

Refactored the map between PseudoTypes and PseudoIds as Dan suggested so that CSSSelector and CSSComputedStyleDeclaration both use the same mechanism for mapping between strings and PseudoTypes (pseudoTypeFromString) and CSSStyleSelector and CSSComputedStyleDeclaration both use the same mechanism for mapping between PseudoTypes and PseudoIds (pseudoIdFromPseudoType).
Comment 13 Darin Adler 2010-03-16 09:21:23 PDT
Comment on attachment 50741 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle (refactor maps among Strings, PseudoTypes, and PseudoIds)

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<CSSPropertyID>(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-
Comment 14 Jessie Berlin 2010-03-21 10:59:34 PDT
Sorry it took me so long to get back on this. My comments below reflect the changes I made in the patch that I will be posting momentarily.

(In reply to comment #13)
> (From update of attachment 50741 [details])
> Looks really good!

Thanks for looking at this :)

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

I was trying to come up with a naming convention to span all uses, and here is what I ended up with:

1. Strings like "first-letter", ":first-letter" or "::first-letter" are names, the first because it really is the name, and the latter two because in the spec, the pseudo-elements are referred to like 'the ":first-letter" pseudo-element'.
2. A member variable that refers to PseudoId for the pseudo-element is a specifier because it specifies which of the possible pseudo-elements is being applied to the element.

so the above m_pseudoElt becomes m_pseudoElementSpecifier.

Does that work?

> 
> > +    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;
> 

Done.

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

Removed.

> 
> >      if (renderer && hasCompositedLayer(renderer) && AnimationController::supportsAcceleratedAnimationOfProperty(static_cast<CSSPropertyID>(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?
> 

I have changed it so that we don't ignore the m_pseudoElementSpecifier on the AnimationController side (and added a corresponding test to getComputedStyle-with-pseudo-element.html). However, the implementation requires that the animation be run at least once so that there is a cachedPseudoStyle.

I believe this is similar to the situation I described in the FIXME in Element.cpp for the ':selection' pseudo-element. In the coming iterations on getComputedStyle, I will be trying to get properties from the renderer that actually corresponds to the pseudo-element. The difficult part so far for me is figuring out how to get that renderer.

Should I create a separate bug for this?

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

Done.

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

Done.

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

Done.

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

Done (refactored into nameToPseudoTypeMap()).

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

Done.

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

Done.

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

Done (changed pseudoTypeFromString to parsePseudoType and pseudoIdFromPseudoType to 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".

Done.

> 
> Please use braces around multiple line if bodies. The outer one here is
> multiple lines long and so needs braces.

Done.

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

Done.

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

I am not really sure what the qualifications are for a call to be considered common in WebKit, but just setting a hit count breakpoint and navigating to a few of the sites I visit daily results in it getting called on Element about 1000 times per page load and more than that to load the inspector (whereas it is only called on Node a few times), so I went ahead and did this.

How do you normally determine whether a call is considered common in WebKit?

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

I understand why the pseudoElt name isn't very helpful, but I was a bit hesitant to replace it with something more descriptive because the name pseudoElt in this case comes directly from the DOM Level 2 Style Interface Specification (http://www.w3.org/TR/DOM-Level-2-Style/css.html) and is used in other places in DOMWindow to represent the name of the pseudo-element (like getMatchedCSSRules).

Do we usually keep their naming schemes in cases like these?

> 
> Most of my comments are minor, but a few are important enough that I think I'll
> say review-
Comment 15 Jessie Berlin 2010-03-21 11:02:18 PDT
Created attachment 51251 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle (addressed issues pointed out by Darin)
Comment 16 Dave Hyatt 2010-04-15 10:59:49 PDT
Comment on attachment 51251 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle (addressed issues pointed out by Darin) 

r=me

Looks great.  Nice to have this fixed.
Comment 17 Jessie Berlin 2010-04-19 06:30:18 PDT
Comment on attachment 51251 [details]
First steps towards exposing the standard pseudo-elements through getComputedStyle (addressed issues pointed out by Darin) 

Thanks! Committed in r57809
http://trac.webkit.org/changeset/57809
Comment 18 WebKit Review Bot 2010-04-19 06:48:21 PDT
http://trac.webkit.org/changeset/57809 might have broken Chromium Mac Release
Comment 19 James Robinson 2010-04-19 12:37:35 PDT
Leading whitespace in the string:
<div id="testHardwareAcceleratedCompositing"> This should be at full opacity.</div>
causes this to fail on the Chromium bots.  Is the space before the " This" intentional here?  I'm surprised that this would be different in chromium.
Comment 20 Jessie Berlin 2010-04-19 17:34:35 PDT
Created attachment 53748 [details]
Remove the space that was causing the test to fail on the Chromium bots. Also remove the platform specific results that were landed in the meantime.

Sorry about that, the space was not intentional.
Comment 21 mitz 2010-04-19 17:40:45 PDT
Comment on attachment 53748 [details]
Remove the space that was causing the test to fail on the Chromium bots. Also remove the platform specific results that were landed in the meantime.

r=me if a review request was intended
Comment 22 Jessie Berlin 2010-04-19 18:01:21 PDT
(In reply to comment #21)
> (From update of attachment 53748 [details])
> r=me if a review request was intended

Yes, thank you. Committed in r57861 http://trac.webkit.org/changeset/57861