Positioned, replaced elements with intrinsic width keywords compute the wrong width
Created attachment 189392 [details] Patch
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 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 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).
(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 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 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.
(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.
Created attachment 189403 [details] Patch
Comment on attachment 189403 [details] Patch Attachment 189403 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16651617
Comment on attachment 189403 [details] Patch Attachment 189403 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16660585
Comment on attachment 189403 [details] Patch Attachment 189403 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16656605
Comment on attachment 189403 [details] Patch Thanks!
Committed r143539: <http://trac.webkit.org/changeset/143539>