Bug 110393

Summary: Positioned, replaced elements with intrinsic width keywords compute the wrong width
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: New BugsAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: eae, eric, esprehn+autocc, esprehn, hyatt, leviw, ojan.autocc, tony, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch eae: review+, eflews.bot: commit-queue-

Description Ojan Vafai 2013-02-20 15:12:43 PST
Positioned, replaced elements with intrinsic width keywords compute the wrong width
Comment 1 Ojan Vafai 2013-02-20 15:18:17 PST
Created attachment 189392 [details]
Patch
Comment 2 Emil A Eklund 2013-02-20 15:33:25 PST
Comment on attachment 189392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189392&action=review

> Source/WebCore/rendering/RenderBox.cpp:2594
> +            LayoutUnit containerLogicalWidth = 0;

This temporary variable isn't needed. If you feel it is needed for readability reason then at last match the name of the argument (availableLogicalWidth).

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>

Where did you get the number 310 from?
Comment 3 Ojan Vafai 2013-02-20 15:38:04 PST
Comment on attachment 189392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189392&action=review

>> Source/WebCore/rendering/RenderBox.cpp:2594
>> +            LayoutUnit containerLogicalWidth = 0;
> 
> This temporary variable isn't needed. If you feel it is needed for readability reason then at last match the name of the argument (availableLogicalWidth).

I think it's needed for readablity. I'll rename it.

>> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
>> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
> 
> Where did you get the number 310 from?

The intrinsic width of an iframe is 300px and it has a 10px border.
Comment 4 Tony Chang 2013-02-20 15:41:12 PST
Comment on attachment 189392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189392&action=review

> Source/WebCore/rendering/RenderBox.cpp:2617
>          }

Nit: It would be nice to remove the default: case and enumerate the remaining types (Auto, Relative, and Undefined, I think).
Comment 5 Emil A Eklund 2013-02-20 15:41:53 PST
(In reply to comment #3)
> (From update of attachment 189392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189392&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:2594
> >> +            LayoutUnit containerLogicalWidth = 0;
> > 
> > This temporary variable isn't needed. If you feel it is needed for readability reason then at last match the name of the argument (availableLogicalWidth).
> 
> I think it's needed for readablity. I'll rename it.

Fair enough.

> 
> >> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
> >> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
> > 
> > Where did you get the number 310 from?
> 
> The intrinsic width of an iframe is 300px and it has a 10px border.

Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.
Comment 6 Elliott Sprehn 2013-02-20 15:43:13 PST
Comment on attachment 189392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189392&action=review

>>>> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
>>>> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
>>> 
>>> Where did you get the number 310 from?
>> 
>> The intrinsic width of an iframe is 300px and it has a 10px border.
> 
> Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.

The whole point of these new style data-expected tests is there's no need for script running. Lets just use a comment to explain that instead.
Comment 7 Ojan Vafai 2013-02-20 15:43:55 PST
Comment on attachment 189392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189392&action=review

>>>> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
>>>> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
>>> 
>>> Where did you get the number 310 from?
>> 
>> The intrinsic width of an iframe is 300px and it has a 10px border.
> 
> Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.

It would make this test considerably more complicated. Now I need script to set an attribute, but some of these widths are different. I mean...I can do it, but it doesn't seem worth it. I could add a comment though and get the same benefit I think.
Comment 8 Emil A Eklund 2013-02-20 15:48:05 PST
(In reply to comment #6)
> > Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.
> 
> The whole point of these new style data-expected tests is there's no need for script running. Lets just use a comment to explain that instead.

It still runs a script. It just happens that you declare the expectations in html instead of js. While it might make sense in many cases it doesn't here as it provides values without any explanation and makes the test harder to understand. Perhaps we should change resources/check-layout.js to allow named constants and simple arithmetic.


(In reply to comment #7)
A comment would do nicely. Thanks.
Comment 9 Ojan Vafai 2013-02-20 16:17:40 PST
Created attachment 189403 [details]
Patch
Comment 10 EFL EWS Bot 2013-02-20 16:24:01 PST
Comment on attachment 189403 [details]
Patch

Attachment 189403 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16651617
Comment 11 Early Warning System Bot 2013-02-20 16:24:41 PST
Comment on attachment 189403 [details]
Patch

Attachment 189403 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16660585
Comment 12 Early Warning System Bot 2013-02-20 16:26:26 PST
Comment on attachment 189403 [details]
Patch

Attachment 189403 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16656605
Comment 13 Emil A Eklund 2013-02-20 16:31:36 PST
Comment on attachment 189403 [details]
Patch

Thanks!
Comment 14 Ojan Vafai 2013-02-20 17:09:39 PST
Committed r143539: <http://trac.webkit.org/changeset/143539>