Bug 29084 - getComputedStyle returns percentage values for left / right / top / bottom
: getComputedStyle returns percentage values for left / right / top / bottom
Status: ASSIGNED
: WebKit
CSS
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
: 106632 110007
  Show dependency treegraph
 
Reported: 2009-09-09 07:07 PST by
Modified: 2013-10-02 12:27 PST (History)


Attachments
test (3.64 KB, text/html)
2012-07-10 14:17 PST, Ryosuke Niwa
no flags Details
work in progress (4.55 KB, patch)
2012-07-16 14:37 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.07 KB, patch)
2013-02-12 00:33 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.29 KB, patch)
2013-02-13 01:01 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.57 KB, patch)
2013-02-13 18:01 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.32 KB, patch)
2013-02-14 21:03 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2013-02-17 17:58 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.27 KB, patch)
2013-02-18 04:19 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.91 KB, patch)
2013-02-21 01:27 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.85 KB, patch)
2013-02-21 02:21 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.85 KB, patch)
2013-02-21 02:45 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.46 KB, patch)
2013-02-21 20:07 PST, Tim 'mithro' Ansell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.80 KB, patch)
2013-02-25 18:05 PST, Tim 'mithro' Ansell
eric: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-09 07:07:17 PST
var div = document.createElement('div');
div.style.position = 'absolute';
div.style.top = '10%';
document.body.appendChild(div);
alert( window.getComputedStyle(div, null).top );

The above will alert '10%' rather than the pixel value.

Gecko / Presto will give the pixel value.

Webkit gives the pixel value for other properties (widht, height etc) and will give the pixel value for left / right / top / bottom if the set value isn't a percent, eg 10em.

The above bug exists in Safari, Chrome and Nightly r48199, although I have only tested on Windows.

Jake.
------- Comment #1 From 2011-10-19 20:52:52 PST -------
'10%' is the correct computed value - percentages are resolved at "used value" time.  However, the getComputedStyle function is a bit of a misnomer, and actually returns a mix of computed and used values.

Which properties return what is underdefined right now.  Ideally we should wait for the CSSOM spec to define this, but in the absence of that, we can look at what other browsers do.

In particular, Firefox returns used values for these properties.  I think that's useful, and we should probably match them.
------- Comment #2 From 2011-11-21 14:59:27 PST -------
> In particular, Firefox returns used values for these properties.

Yes, FF, IE9, and Opera all use the "used value" instead of the "computed value".  

> I think that's useful, and we should probably match them.

Yes, it's immensely useful, and in particular, several popular javascript libraries have bugs open for it:
 * http://bugs.jquery.com/ticket/10639
 * http://yuilibrary.com/projects/yui3/ticket/2529799
------- Comment #3 From 2011-11-21 15:48:08 PST -------
What does Trident do?
------- Comment #4 From 2011-11-21 15:56:10 PST -------
IE9 returns pixels, as Mike says in #2.
------- Comment #5 From 2011-11-21 16:00:32 PST -------
Okay, then it appears that this is a no-brainer. We should just fix it.
------- Comment #6 From 2011-11-21 16:01:03 PST -------
And IE<9 doesn't even have getComputedStyle... But the nonstandard currentStyle returns % instead of pixels (computed vs. used)
------- Comment #7 From 2011-11-21 16:04:00 PST -------
(In reply to comment #6)
> And IE<9 doesn't even have getComputedStyle... But the nonstandard currentStyle returns % instead of pixels (computed vs. used)

But that's a different API so it doesn't seem like a problem for us. I guess my only worry is that there might be some WebKit-only content that depends on this behavior but that's probably unlikely.
------- Comment #8 From 2011-11-21 16:13:01 PST -------
Sure, currentStyle is non-standard and is a different API, I was just trying to give the full Trident story :).

So I'm new to this... If I want to fix this myself, do I just go right to it in the webkit source code, paying attention to the guidelines according to the webkit.org site, or is there something else I should do first?
------- Comment #9 From 2011-11-21 18:51:50 PST -------
Bug 42799 is about the problem of pixelXXX CSS properties which seems a workaround of this bug. I think after we fix this bug we can drop the pixelXXX properties.

The current draft CSSOM says getComputedStyle() should return the resolved values, and for width, height etc., "if the property applies to the element or pseudo-element and the resolved value of the 'display' property is not none, the resolved value is the used value. Otherwise the resolved value is the computed value."
------- Comment #10 From 2011-11-21 19:11:35 PST -------
That makes sense. If it's display none or disconnected, pixel values (used values) aren't possible nor easy to correctly resolve, and so therefore should report computed values (cascaded values or whatever you want to call it).
------- Comment #11 From 2011-11-21 19:56:38 PST -------
(In reply to comment #10)
> That makes sense. If it's display none or disconnected, pixel values (used values) aren't possible nor easy to correctly resolve, and so therefore should report computed values (cascaded values or whatever you want to call it).

Sorry I made a mistake that thought the list of properties that getComputedStyle() may return the used value included left/right/top/bottom. Actually the draft (http://dev.w3.org/csswg/cssom/#resolved-value) implies for left/right/top/bottom getComputedStyle() should return the computed style.
------- Comment #12 From 2011-11-21 20:07:00 PST -------
I'm sorry for causing any confusion. This is perhaps the correct behavior, but then this exposes the fact that although top/left/right/bottom follow the draft spec, margin*, padding*, border* do not and FF, IE9, and Opera do follow the spec for margin*, padding*, and border*.  

Can we at least then bring those properties in line with the other browsers, as it would also match the draft?

Shall I create another ticket for those properties separately?
------- Comment #13 From 2011-11-21 20:19:38 PST -------
(In reply to comment #12)
> I'm sorry for causing any confusion. This is perhaps the correct behavior, but then this exposes the fact that although top/left/right/bottom follow the draft spec, margin*, padding*, border* do not and FF, IE9, and Opera do follow the spec for margin*, padding*, and border*.  

I meant padding and margin, not border. 

> 
> Can we at least then bring those properties in line with the other browsers, as it would also match the draft?
> 
> Shall I create another ticket for those properties separately?
------- Comment #14 From 2011-11-21 20:31:20 PST -------
(In reply to comment #12)
> Shall I create another ticket for those properties separately?

I prefer another bug for margin, padding, width/height because they should have no controversy.
------- Comment #15 From 2011-11-22 11:07:58 PST -------
If IE, FF, & Opera all behave the same, then that must be the behavior we match, not the spec. In fact, we should amend the spec to match the reality.
------- Comment #16 From 2011-11-22 16:58:16 PST -------
Just to be complete here, this fiddle shows it all:

http://jsfiddle.net/u4F8m/5/embedded/result/

IE9, O, FF all have "used values" for the properties listed.

Webkit has "computed values" for margin-*,top/left/right/bottom

margin-* is clearly a bug, as it neither matches the other browsers nor the spec.

top/left/right/bottom match the spec, and not the other browsers, and I believe the spec should be amended.
------- Comment #17 From 2011-11-23 17:38:52 PST -------
Looks like this behavior is intentional, and was "fixed" to do this behavior in: https://bugs.webkit.org/show_bug.cgi?id=13343

Can someone please weigh in here? It seems as if CSS2.1 and CSS3 are in disagreement.
------- Comment #18 From 2012-03-17 14:02:18 PST -------
So I posted on www-style about this, and it seemed as if "used value" is the way to go here. I submitted a bug to the editor of CSSOM about amending the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16389

Although the spec currently says computed value for top/left/bottom/right, I think changing to used value is the right move, for the reasons I mention in this thread, on www-style, and in the CSSOM bug report. 

This change would be a big win for interopability and for JS libs that attempt to provide interoperability like jQuery and YUI. 

Thoughts?
------- Comment #19 From 2012-03-19 08:37:04 PST -------
(In reply to comment #18)
> So I posted on www-style about this, and it seemed as if "used value" is the way to go here. I submitted a bug to the editor of CSSOM about amending the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16389
> 
> Although the spec currently says computed value for top/left/bottom/right, I think changing to used value is the right move, for the reasons I mention in this thread, on www-style, and in the CSSOM bug report. 
> 
> This change would be a big win for interopability and for JS libs that attempt to provide interoperability like jQuery and YUI. 
> 
> Thoughts?

I say do it.  You've interacted on the list and submitted a bug report against the spec, which has an active editor who will take care of it.  It seems clear that the right solution is to fix the spec to match browsers here, and we should adjust ourselves to be in line with that.
------- Comment #20 From 2012-06-22 05:33:14 PST -------
I know I'd previously said I'd like to take a shot on this bug, but it's a bit over my head at the moment. 

In the meantime, I'd just like to ping this ticket with an update on the frequency with which this is reported as a bug to the jQuery bug tracker, in an attempt to get the priority ticket of this raised, and to get someone more familiar with the source to work on it:

http://bugs.jquery.com/ticket/11932
http://bugs.jquery.com/ticket/11954
http://bugs.jquery.com/ticket/9505
------- Comment #21 From 2012-06-22 14:05:46 PST -------
I'm sorry, what we're supposed to do here? Is the percentage values correct or incorrect?
------- Comment #22 From 2012-06-22 14:11:35 PST -------
top/left/right/bottom should return pixel values, even if percentage is specified, to match the other browsers. Yes, this is against the current spec, but there has been significant discussion on the W3C ML about it, and a ticket filed against the spec to amend.

Thanks for your reply!
------- Comment #23 From 2012-07-06 22:55:36 PST -------
I think we just need to tweak getPositionOffsetValue in http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp

I'll get to get to it next week.
------- Comment #24 From 2012-07-09 18:28:29 PST -------
(In reply to comment #23)
> I think we just need to tweak getPositionOffsetValue in http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp
> 
> I'll get to get to it next week.

Looks that way. That's so much for getting to this!
------- Comment #25 From 2012-07-10 14:17:03 PST -------
Created an attachment (id=151525) [details]
test
------- Comment #26 From 2012-07-11 18:20:10 PST -------
Those tests look good to me.

If you allow me to digress for a moment, what's interesting is that running them in the different browsers show just how divergent the implementation is amongst browsers:

Opera 11.6: Passes all tests as written
FF13: Converts "auto" to pixels so fails when expecting "auto".
IE9/IE10: incorrectly accounts for padding so fails on the padding tests.
Webkit: doesn't convert to pixels so fails when expecting pixels.

Now, if we really wanted to go all the way with this, I personally believe that the FF behavior is most useful, and most accurately fits the idea of returning used value (that is, converting "auto" to actual pixels, even if not specified"). 

However, to achieve the most consistency is to expect "auto", as you have written here.
------- Comment #27 From 2012-07-11 20:01:06 PST -------
(In reply to comment #26)
> Opera 11.6: Passes all tests as written
> FF13: Converts "auto" to pixels so fails when expecting "auto".
> IE9/IE10: incorrectly accounts for padding so fails on the padding tests.
> Webkit: doesn't convert to pixels so fails when expecting pixels.
> 
> Now, if we really wanted to go all the way with this, I personally believe that the FF behavior is most useful, and most accurately fits the idea of returning used value (that is, converting "auto" to actual pixels, even if not specified"). 
> 
> However, to achieve the most consistency is to expect "auto", as you have written here.

Yeah, I'm not sure what the correct "expected" behavior is. Intuitively FF or Opera seem to do well but given that IE and Opera both return "auto" for unspecified values, returning "auto" here seems like a good idea.

We need some tests for vertical writing mode and RTL pages.
------- Comment #28 From 2012-07-11 20:17:06 PST -------
According to http://www.w3.org/TR/CSS2/cascade.html#used-value, " The used value is the result of taking the computed value and resolving any remaining dependencies into an absolute value." and then subsequently "A used value is in principle the value used for rendering,"

Is "auto" an absolute value or the value used for rendering? I don't personally believe so.
Is "auto" a useful value to return? I personally don't believe so.
Is "auto" consistent with MOST of the other browsers? yes.

What I really care about here is that the percentage stuff gets fixed. Perhaps we can save any decision about "auto" for another ticket?
------- Comment #29 From 2012-07-11 20:34:05 PST -------
(In reply to comment #28)
> According to http://www.w3.org/TR/CSS2/cascade.html#used-value, " The used value is the result of taking the computed value and resolving any remaining dependencies into an absolute value." and then subsequently "A used value is in principle the value used for rendering,"
> 
> Is "auto" an absolute value or the value used for rendering? I don't personally believe so.
> Is "auto" a useful value to return? I personally don't believe so.
> Is "auto" consistent with MOST of the other browsers? yes.
> 
> What I really care about here is that the percentage stuff gets fixed. Perhaps we can save any decision about "auto" for another ticket?

Let's add this information on https://www.w3.org/Bugs/Public/show_bug.cgi?id=16389 and see what others think. We should at least provide the relevant information there before deciding what to do. e.g. other browser vendors might be in the process of changing their behaviors.
------- Comment #30 From 2012-07-11 20:40:38 PST -------
I added the info to the ticket. I'll also post about this on www-style to see if I can get an answer.
------- Comment #31 From 2012-07-11 23:41:40 PST -------
posted on the ML about this: http://lists.w3.org/Archives/Public/www-style/2012Jul/0277.html
------- Comment #32 From 2012-07-12 05:30:00 PST -------
This pretty much says it all: http://lists.w3.org/Archives/Public/www-style/2012Jul/0284.html

That is: FF13 is the correct behavior, "auto" should be converted to pixel.
------- Comment #33 From 2012-07-16 14:37:46 PST -------
Created an attachment (id=152616) [details]
work in progress
------- Comment #34 From 2012-08-01 09:00:20 PST -------
by the way, this was added to the CSSOM spec on 7/17/12: http://dev.w3.org/csswg/cssom/#resolved-values
------- Comment #35 From 2012-11-24 20:38:04 PST -------
(From update of attachment 152616 [details])
Attachment 152616 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14991111
------- Comment #36 From 2012-11-24 20:38:27 PST -------
(From update of attachment 152616 [details])
Attachment 152616 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14981219
------- Comment #37 From 2012-11-24 20:43:24 PST -------
(From update of attachment 152616 [details])
Attachment 152616 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14975800
------- Comment #38 From 2012-11-24 20:48:36 PST -------
(From update of attachment 152616 [details])
Attachment 152616 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14970948
------- Comment #39 From 2012-11-24 20:55:06 PST -------
(From update of attachment 152616 [details])
Attachment 152616 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14989147
------- Comment #40 From 2012-11-24 21:03:31 PST -------
(From update of attachment 152616 [details])
Attachment 152616 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14986153
------- Comment #41 From 2012-11-24 21:14:15 PST -------
(From update of attachment 152616 [details])
Attachment 152616 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14983188
------- Comment #42 From 2012-12-28 11:03:43 PST -------
Is there anything I can do to help move this along besides actually authoring a patch myself? I'd like to get involved but don't know per se where to start.
------- Comment #43 From 2013-02-12 00:33:21 PST -------
Created an attachment (id=187794) [details]
Patch
------- Comment #44 From 2013-02-12 01:40:13 PST -------
(From update of attachment 187794 [details])
Attachment 187794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16493740

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
fast/css/getComputedStyle/computed-style.html
svg/css/getComputedStyle-basic.xhtml
jquery/offset.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/hover-affects-child.html
------- Comment #45 From 2013-02-12 15:22:40 PST -------
(From update of attachment 187794 [details])
Attachment 187794 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16517398

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
jquery/offset.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/hover-affects-child.html
------- Comment #46 From 2013-02-12 23:56:23 PST -------
(From update of attachment 187794 [details])
Attachment 187794 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16531400

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
jquery/offset.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/hover-affects-child.html
------- Comment #47 From 2013-02-13 01:01:38 PST -------
Created an attachment (id=188032) [details]
Patch
------- Comment #48 From 2013-02-13 01:05:43 PST -------
Attachment 188032 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSCalculationValue.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1
Source/WebCore/css/CSSCalculationValue.cpp:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1533:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #49 From 2013-02-13 01:28:31 PST -------
(From update of attachment 188032 [details])
Attachment 188032 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16530034

New failing tests:
animations/duplicated-keyframes-name.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/fill-mode-multiple-keyframes.html
animations/missing-keyframe-properties-repeating.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/animation-direction-reverse-timing-functions.html
animations/change-keyframes-name.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/animation-direction-reverse-non-hardware.html
animations/missing-keyframe-properties-timing-function.html
animations/missing-values-first-keyframe.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/change-one-anim.html
animations/generic-from-to.html
------- Comment #50 From 2013-02-13 02:21:06 PST -------
(From update of attachment 188032 [details])
Attachment 188032 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16536050

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
animations/duplicated-keyframes-name.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/fill-mode-multiple-keyframes.html
animations/missing-keyframe-properties-repeating.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/animation-direction-reverse-timing-functions.html
animations/change-keyframes-name.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/animation-direction-reverse-non-hardware.html
animations/missing-keyframe-properties-timing-function.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/change-one-anim.html
animations/generic-from-to.html
------- Comment #51 From 2013-02-13 06:06:41 PST -------
(From update of attachment 188032 [details])
Attachment 188032 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16527146

New failing tests:
animations/duplicated-keyframes-name.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/fill-mode-multiple-keyframes.html
animations/missing-keyframe-properties-repeating.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/animation-direction-reverse-timing-functions.html
animations/change-keyframes-name.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/animation-direction-reverse-non-hardware.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/change-one-anim.html
animations/generic-from-to.html
------- Comment #52 From 2013-02-13 18:01:11 PST -------
Created an attachment (id=188242) [details]
Patch
------- Comment #53 From 2013-02-13 18:07:11 PST -------
Attachment 188242 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSCalculationValue.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1
Source/WebCore/css/CSSCalculationValue.cpp:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1533:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #54 From 2013-02-13 19:04:04 PST -------
(From update of attachment 188242 [details])
Attachment 188242 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16570144

New failing tests:
animations/duplicated-keyframes-name.html
animations/multiple-animations.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/multiple-animations-timing-function.html
animations/fill-mode-multiple-keyframes.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/multiple-keyframes.html
animations/simultaneous-start-left.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/missing-keyframe-properties-timing-function.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/negative-delay.html
animations/change-one-anim.html
animations/generic-from-to.html
------- Comment #55 From 2013-02-14 21:03:28 PST -------
Created an attachment (id=188473) [details]
Patch
------- Comment #56 From 2013-02-14 22:39:09 PST -------
(From update of attachment 188473 [details])
Attachment 188473 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16559807

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
editing/pasteboard/drag-drop-input-textarea.html
editing/deleting/delete-all-text-in-text-field-assertion.html
editing/shadow/insertorderedlist-crash.html
css3/flexbox/css-properties.html
editing/pasteboard/drop-text-events.html
editing/pasteboard/copy-crash.html
fast/css-grid-layout/grid-columns-rows-get-set.html
animations/animation-border-overflow.html
editing/selection/delete-word-granularity-text-control.html
editing/input/set-value-on-input-and-delete.html
editing/selection/drag-text-delay.html
editing/pasteboard/drag-drop-url-text.html
editing/pasteboard/paste-plaintext-nowrap.html
editing/input/ime-composition-clearpreedit.html
editing/style/style-3998892-fix.html
fast/css/getComputedStyle/computed-style-display-none.html
editing/style/apple-style-editable-mix.html
editing/pasteboard/drop-inputtext-acquires-style.html
editing/style/apply-style-join-child-text-nodes-crash.html
editing/style/iframe-onload-crash-mac.html
editing/shadow/bold-twice-in-shadow.html
editing/pasteboard/copy-paste-first-line-in-textarea.html
fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
fast/css/getComputedStyle/computed-style-without-renderer.html
editing/pasteboard/paste-placeholder-input.html
editing/deleting/5290534.html
editing/pasteboard/drag-drop-input-in-svg.svg
editing/style/table-selection.html
fast/css/counters/counter-after-style-crash.html
------- Comment #57 From 2013-02-14 23:38:09 PST -------
(From update of attachment 188473 [details])
Attachment 188473 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16538990

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
editing/pasteboard/4944770-2.html
editing/deleting/delete-all-text-in-text-field-assertion.html
editing/style/apply-style-join-child-text-nodes-crash.html
css3/flexbox/css-properties.html
fast/css/lang-selector-empty-attribute.xhtml
fast/css/hover-affects-child.html
animations/animation-add-events-in-handler.html
editing/style/table-selection.html
fast/css-grid-layout/grid-columns-rows-get-set.html
editing/selection/delete-word-granularity-text-control.html
editing/input/set-value-on-input-and-delete.html
editing/input/ime-composition-clearpreedit.html
fast/css/getComputedStyle/computed-style-display-none.html
editing/pasteboard/copy-display-none.html
editing/style/iframe-onload-crash-mac.html
http/tests/misc/acid3.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
editing/pasteboard/4944770-1.html
editing/deleting/5290534.html
editing/pasteboard/copy-crash.html
editing/pasteboard/4641033.html
------- Comment #58 From 2013-02-15 07:22:35 PST -------
Hi Tim,

There are a couple of things I think you are doing wrong on this bug.


Firstly, you need to run the test suite locally rather than relying on the bots to find failures. It is possible to get chromium-linux to build and run the tests cleanly on your own machine if you run linux. I believe the same is true of the Mac build. You need to invest a bit of time in this upfront it will mean you can be productive on this and other bugs.

Secondly, your patch needs to have tests of its own. I haven't looked closely at the change you're making but it definitely needs tests.

We remove code rather than comment it out. 

Your change needs a detailed changelog as it is substantial and requires explanation, both overall and for particular changes like commenting out asserts!

Finally unnecessary whitespace changes can often result in a r- on their own.
------- Comment #59 From 2013-02-15 12:21:01 PST -------
Attachment 188473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSCalculationValue.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1
Source/WebCore/css/CSSCalculationValue.cpp:207:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:597:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:598:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1531:  max_width is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1532:  max_height is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1558:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #60 From 2013-02-15 15:57:19 PST -------
(From update of attachment 188473 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=188473&action=review

Thanks for tackling this!

The change log for this patch is unacceptably empty. You need to explain what you are doing and why, not just cite the name of the bug.

Also, patches that fix bugs need to include test cases that show the bug is fixed and that would fail without the bug fix.

> Source/WebCore/css/CSSCalculationValue.cpp:188
> +//            ASSERT_NOT_REACHED();

I assume this was included with the patch by accident, since the change log doesn’t mention it. If not, we should discuss why you made this change. And it’s definitely incorrect to change it by commenting out the assertion.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:582
> -static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLength(const Length& length, const RenderStyle* style)
> +static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLengthOld(const Length& length, const RenderStyle* style)

You can’t just add the word old to the end of the function without explaining it, at least in change log. Please explain your rationale here.
------- Comment #61 From 2013-02-15 16:01:20 PST -------
(From update of attachment 188473 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=188473&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:582
>> +static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLengthOld(const Length& length, const RenderStyle* style)
> 
> You can’t just add the word old to the end of the function without explaining it, at least in change log. Please explain your rationale here.

Once we can clearly explain and understand the difference between the old and new versions of this function, we can presumably choose an appropriate name for the old one, other than “old”. Unless the plan is to eliminate the old function entirely, in which case we need to at least explain that direction in this patch here.
------- Comment #62 From 2013-02-17 17:57:56 PST -------
Sorry, this patch wasn't yet ready for review. I'll mark if review? when it's ready.
------- Comment #63 From 2013-02-17 17:58:15 PST -------
Created an attachment (id=188785) [details]
Patch
------- Comment #64 From 2013-02-17 18:02:06 PST -------
(From update of attachment 188785 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=188785&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

We definitely need a new layout test for this. r- due to lack of tests.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:669
> +                maxWidth = renderer->containingBlock()->availableWidth();
> +                break;
> +            case CSSPropertyTop:
> +            case CSSPropertyBottom:
> +                maxWidth = renderer->containingBlock()->availableHeight();

Does this work in vertical writing mode? We need a test.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:672
> +            default: {
> +            }}

Please replace {} with a break.
------- Comment #65 From 2013-02-17 18:03:02 PST -------
Totally reworked the patch to be much less invasive. Still need to fix the remaining tests which fail because getComputedStyle now always returns pixels.
------- Comment #66 From 2013-02-18 02:54:16 PST -------
(From update of attachment 188785 [details])
Attachment 188785 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16613098

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/hover-affects-child.html
jquery/offset.html
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
------- Comment #67 From 2013-02-18 04:19:29 PST -------
Created an attachment (id=188845) [details]
Patch
------- Comment #68 From 2013-02-18 17:55:32 PST -------
(From update of attachment 188845 [details])
Attachment 188845 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16614449

New failing tests:
editing/pasteboard/paste-match-style-002.html
animations/multiple-animations.html
fast/css-generated-content/pseudo-animation.html
editing/execCommand/query-font-size-with-typing-style.html
animations/longhand-timing-function.html
animations/missing-from-to.html
editing/execCommand/infinite-recursion-computeRectForRepaint.html
animations/fill-mode-removed.html
animations/multiple-animations-timing-function.html
animations/fill-mode-multiple-keyframes.html
fast/css-generated-content/pseudo-transition.html
animations/change-keyframes.html
animations/multiple-keyframes.html
editing/pasteboard/paste-match-style-001.html
animations/simultaneous-start-left.html
animations/timing-functions.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
editing/style/remove-underline-after-paragraph.html
animations/missing-keyframe-properties-timing-function.html
animations/fill-mode-missing-from-to-keyframes.html
editing/style/remove-underline-after-paragraph-in-bold.html
animations/missing-keyframe-properties.html
editing/inserting/insert-div-024.html
animations/animation-direction-reverse-fill-mode.html
editing/inserting/insert-div-026.html
animations/change-one-anim.html
animations/generic-from-to.html
------- Comment #69 From 2013-02-19 10:57:01 PST -------
(From update of attachment 188845 [details])
Attachment 188845 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16612895

New failing tests:
animations/multiple-animations.html
fast/css-generated-content/pseudo-animation.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/fill-mode-reverse.html
animations/fill-mode-removed.html
animations/multiple-animations-timing-function.html
animations/fill-mode-multiple-keyframes.html
fast/css-generated-content/pseudo-transition.html
animations/change-keyframes.html
animations/multiple-keyframes.html
animations/simultaneous-start-left.html
animations/timing-functions.html
animations/fill-mode.html
fast/css/getComputedStyle/computed-style-negative-top.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/missing-keyframe-properties-timing-function.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/animation-direction-reverse-fill-mode.html
animations/generic-from-to.html
------- Comment #70 From 2013-02-21 01:27:30 PST -------
Created an attachment (id=189471) [details]
Patch
------- Comment #71 From 2013-02-21 01:30:20 PST -------
Third attempt at trying to get this right.
------- Comment #72 From 2013-02-21 01:36:21 PST -------
(From update of attachment 189471 [details])
Attachment 189471 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16652878
------- Comment #73 From 2013-02-21 01:37:48 PST -------
(From update of attachment 189471 [details])
Attachment 189471 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16653906
------- Comment #74 From 2013-02-21 01:39:35 PST -------
(From update of attachment 189471 [details])
Attachment 189471 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16650929
------- Comment #75 From 2013-02-21 01:43:07 PST -------
(From update of attachment 189471 [details])
Attachment 189471 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16647933
------- Comment #76 From 2013-02-21 01:52:15 PST -------
(From update of attachment 189471 [details])
Attachment 189471 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16651904
------- Comment #77 From 2013-02-21 01:56:38 PST -------
(From update of attachment 189471 [details])
Attachment 189471 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16653907
------- Comment #78 From 2013-02-21 02:21:16 PST -------
Created an attachment (id=189487) [details]
Patch
------- Comment #79 From 2013-02-21 02:45:44 PST -------
Created an attachment (id=189490) [details]
Patch
------- Comment #80 From 2013-02-21 02:47:14 PST -------
Less crashy version of the patch.
------- Comment #81 From 2013-02-21 03:22:21 PST -------
(From update of attachment 189490 [details])
Attachment 189490 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16688051

New failing tests:
fast/css/getComputedStyle/getComputedStyle-zoom-and-background-size.html
jquery/offset.html
------- Comment #82 From 2013-02-21 03:30:57 PST -------
(From update of attachment 189490 [details])
Attachment 189490 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16656954

New failing tests:
fast/css/getComputedStyle/getComputedStyle-zoom-and-background-size.html
jquery/offset.html
------- Comment #83 From 2013-02-21 03:35:49 PST -------
(From update of attachment 189490 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=189490&action=review

> Source/WebCore/ChangeLog:7
> +

You need to explain the change here. You also need to give the link to the spec where this is specified.
------- Comment #84 From 2013-02-21 15:55:42 PST -------
Added the following description; will upload again once I get the final two tests to pass.

         Fixing getComputedStyle to return pixel values for left / right / top / bottom
+
+        The returned object in CSS 2.1 "used values" (while CSS 2.0 used the "computed values"). 
+        http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSview-getComputedStyle
+
+        The "used value" of any CSS property is the final value of that property
+        after all calculations have been performed. Dimensions are all in pixels.
+        http://www.w3.org/TR/CSS2/cascade.html#used-value
+
+        This is now consistent with both release Firefox and IE9.
+
------- Comment #85 From 2013-02-21 20:07:56 PST -------
Created an attachment (id=189665) [details]
Patch
------- Comment #86 From 2013-02-21 20:09:41 PST -------
This patch should finally be ready for review. The tests have been extended quite a bit and now they should also all pass.

Could you guys please take a look?
------- Comment #87 From 2013-02-21 22:33:52 PST -------
(From update of attachment 189665 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=189665&action=review

Looks pretty good, just a couple things. I take it you tested this in Firefox and got the same results?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:640
> +    // If the element is not displayed; return the "computed value" rather then the "used value".

Can you put a link to the spec here? That's super useful in the code. :)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:657
> +    renderView->layout();

This is wrong, you just want to change isLayoutDependentProperty to include these properties.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:680
> +            return zoomAdjustedPixelValue(renderer->containingBlock()->clientHeight() - (box->offsetTop()+box->offsetHeight()) - box->marginBottom(), style);

containingBLock->clientHeight(), You already know the containing block from above, asking for it again is expensive. Also add a space around the +, I completely missed that there's addition in there at first.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:684
> +            return zoomAdjustedPixelValue(renderer->containingBlock()->clientWidth() - (box->offsetLeft()+box->offsetWidth()) - box->marginRight(), style);

Same
------- Comment #88 From 2013-02-22 01:46:25 PST -------
(From update of attachment 189665 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=189665&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:651
> +        switch (propertyID) {
>          case CSSPropertyLeft:
> -            l = style->left();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->left(), style);
>          case CSSPropertyRight:
> -            l = style->right();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->right(), style);
>          case CSSPropertyTop:
> -            l = style->top();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->top(), style);
>          case CSSPropertyBottom:
> -            l = style->bottom();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->bottom(), style);
>          default:

For the "in-code documentation" sake, it could be good to turn this into a static function.
E.g.: 
if (!renderView || !renderer || !renderer->isBox())
    return positionOffsetStyleValueForProperty(propertyID, style);

I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:673
> +        switch (propertyID) {
> +        case CSSPropertyTop:
> +            return zoomAdjustedPixelValue(box->relativePositionOffset().height(), style);
> +        case CSSPropertyBottom:
> +            return zoomAdjustedPixelValue(-(box->relativePositionOffset().height()), style);
> +        case CSSPropertyLeft:
> +            return zoomAdjustedPixelValue(box->relativePositionOffset().width(), style);
> +        case CSSPropertyRight:
> +            return zoomAdjustedPixelValue(-(box->relativePositionOffset().width()), style);
> +        default:
> +            ASSERT_NOT_REACHED();
> +        }

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:678
> +        switch (propertyID) {
> +        case CSSPropertyTop:
> +            return zoomAdjustedPixelValue(box->offsetTop() - box->marginTop(), style);

Ditto.

> LayoutTests/fast/css/getComputedStyle/computed-style-negative-top.html:41
> -        result.appendChild(document.createTextNode("Test succeeded! Top is " + style.top + "."));
> +        result.appendChild(document.createTextNode("Test succeeded! top is " + style.top + "."));
> +    else
> +        result.appendChild(document.createTextNode("Test failed! top is " + style.top + "."));
> +
> +    result.appendChild(document.createElement('br'));
> +
> +    if (style.left == "-2px")
> +        result.appendChild(document.createTextNode("Test succeeded! left is " + style.left + "."));
> +    else
> +        result.appendChild(document.createTextNode("Test failed! left is " + style.left + "."));
> +
> +    result.appendChild(document.createElement('br'));
> +
> +    if (style.bottom == "1px")
> +        result.appendChild(document.createTextNode("Test succeeded! bottom is " + style.bottom + "."));
> +    else
> +        result.appendChild(document.createTextNode("Test failed! bottom is " + style.bottom + "."));
> +
> +    result.appendChild(document.createElement('br'));
> +
> +    if (style.right == "2px")
> +        result.appendChild(document.createTextNode("Test succeeded! right is " + style.right + "."));

It is a good opportunity to convert this test to js-test-pre.js
------- Comment #89 From 2013-02-24 15:53:27 PST -------
(From update of attachment 189665 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=189665&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:651
>>          default:
> 
> For the "in-code documentation" sake, it could be good to turn this into a static function.
> E.g.: 
> if (!renderView || !renderer || !renderer->isBox())
>     return positionOffsetStyleValueForProperty(propertyID, style);
> 
> I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.

I'm not sure what you are asking here? Changing the code to

switch(propertyID) {
case CSSPropertyLeft:
  if (!renderView || !renderer || !renderer->isBox()) {
    return zoomAdjustedPixelValueForLength(style->bottom(), style);
  } else if (box->isRelPositioned() || !containingBlock) {
    return zoomAdjustedPixelValue(box->relativePositionOffset().width(), style);
  } else {
    return zoomAdjustedPixelValue(box->offsetTop() - box->marginTop(), style);
  }
  ASSERT_NOT_REACHED();

case CSSPropertyRight:
  if (!renderView || !renderer || !renderer->isBox()) {
    return zoomAdjustedPixelValueForLength(style->right(), style);
  } else if (box->isRelPositioned() || !containingBlock) {
    return zoomAdjustedPixelValue(-(box->relativePositionOffset().width()), style);
  } else {
    return zoomAdjustedPixelValue(containingBlock()->clientWidth() - (box->offsetLeft()+box->offsetWidth()) - box->marginRight(), style);
  }
  ASSERT_NOT_REACHED();
....

??
------- Comment #90 From 2013-02-24 22:52:54 PST -------
No no no!! That'd be worse!

Which part was not clear?
------- Comment #91 From 2013-02-25 01:49:11 PST -------
You said;

> For the "in-code documentation" sake, it could be good to turn this into a static function.
> E.g.: 
> if (!renderView || !renderer || !renderer->isBox())
>     return positionOffsetStyleValueForProperty(propertyID, style);
> 
> I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.

I don't understand which code you are indicating to become the static function...
------- Comment #92 From 2013-02-25 02:42:20 PST -------
(In reply to comment #91)
> You said;
> 
> > For the "in-code documentation" sake, it could be good to turn this into a static function.
> > E.g.: 
> > if (!renderView || !renderer || !renderer->isBox())
> >     return positionOffsetStyleValueForProperty(propertyID, style);
> > 
> > I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.
> 
> I don't understand which code you are indicating to become the static function...

Okay. They were two separate comment.

1) If you move each switch-case into its own separate static function, you will make the code of getPositionOffsetValue() easier to follow. Even more so if you can find a good name for each group.

2) By copying zoomAdjustedPixelValueForLength to each case instead of applying it to a Length value, you make the code less flexible. Every future change will have to update 4 sites.

For example
    static inline PassRefPtr<CSSValue>positionOffsetStyleValueForProperty(CSSPropertyID propertyID, RenderStyle* style)
    {
        Length length;
        switch(propertyID) {
        case CSSPropertyLeft:
            length = style->left()
            break;
        [...]
        }
        return zoomAdjustedPixelValueForLength(length, style);
    }

Then, the code simply becomes
    if (!renderView || !renderer || !renderer->isBox())
        return positionOffsetStyleValueForProperty(propertyID, style);


This is only my opinion regarding readability of this section. I don't feel strongly about those changes so feel free to ignore the comments if you don't think this would improve the code.
------- Comment #93 From 2013-02-25 18:05:13 PST -------
Created an attachment (id=190170) [details]
Patch
------- Comment #94 From 2013-02-25 18:26:14 PST -------
Please take another look.
------- Comment #95 From 2013-03-04 17:10:48 PST -------
Ping?
------- Comment #96 From 2013-03-08 13:17:27 PST -------
(From update of attachment 190170 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=190170&action=review

OK.  This change looks great.  My only concern is about testing.  There are a lot of cases here.  I'm not sure you covered position: sticky.  You shoudl be aware of the many types of lenghts:
enum LengthType {
    Auto, Relative, Percent, Fixed,
    Intrinsic, MinIntrinsic,
    MinContent, MaxContent, FillAvailable, FitContent,
    Calculated,
    ViewportPercentageWidth, ViewportPercentageHeight, ViewportPercentageMin, ViewportPercentageMax,
    Undefined
};

You shouldn't need to care about those, but its good to knwo it exists.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:641
> +    // See http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSview-getComputedStyle

Maybe you meant this?
http://dev.w3.org/csswg/cssom/#widl-Window-getComputedStyle-CSSStyleDeclaration-Element-elt

If the property applies to a positioned element and the resolved value of the 'display' property is not none, the resolved value is the used value. Otherwise the resolved value is the computed value.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:645
> +            return zoomAdjustedPixelValueForLength(style->left(), style);

I might have moved these switch statements into helper functions.  Then this code looks like:

if (!renderView || !renderer || !renderer->isBox())
    return zoomAdjustedPixelValueForLength(lengthForProperty(propertyId), style);

One could do similar for the other switches, presumably:
relativePositionForProperty(propertyId, box);
and
positionForProperty(propertyId, box, containingBlock);

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-661
> -        else if (l.isViewportPercentage())

Do we have test coverage for this behavior?  You appear to be removing it in the display: none case.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:659
> +    if (box->isRelPositioned() || !containingBlock) {

I don't believe we can hit this case?  You will always have a containing block if you're in the DOM tree, I think?  Unless we're talking about <html style='position: relative'>?  If the element isn't in the tree, it can't have a renderer.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:687
> +    return 0;

We need tests requesting top/right/bottom/left on elements which are not positioned.
------- Comment #97 From 2013-03-08 13:20:40 PST -------
(From update of attachment 190170 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=190170&action=review

> Source/WebCore/ChangeLog:13
> +        This is now consistent with both release Firefox and IE9.

You might add more links/documetnation here.  Your ChangeLog is sorta your "advertisement" for your change.  Tells all the "whys" and how we'd be fools to not accept your amazing change. :)
------- Comment #98 From 2013-04-08 21:51:30 PST -------
This issue has been migrated to blink at http://code.google.com/p/chromium/issues/detail?id=178030

We will look at back porting the changes once we have committed to change to blink.
------- Comment #99 From 2013-04-09 01:47:29 PST -------
It doesn't seem to be http://code.google.com/p/chromium/issues/detail?id=178030, did you make a typo in the number?
------- Comment #100 From 2013-04-09 02:13:39 PST -------
Sorry - you are right. The correct bug is 
https://code.google.com/p/chromium/issues/detail?id=229280