Bug 93050 – Remove LayoutTypes abstraction

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Emil A Eklund]

LayoutTypes.h acts as an abstraction for types mapping LayoutRect/Point/Size to IntRect/Point/Size or FractionalLayoutRect/Point/Size. This abstraction should be removed now that all platforms use the FractionalLayout version (albeit with different fractions).

[Emil A Eklund]

Created attachment 172394 [details]
Patch

[WebKit Review Bot]

Attachment 172394 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/LayoutRect.h:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/LayoutUnit.h:191:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:38:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:39:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 125 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Levi Weintraub]

Comment on attachment 172394 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:366
> +    return clientWidth() - min(LayoutUnit(0), layoutOverflowRect().x() - borderLeft());

Why not just LayoutUnit()?

[Early Warning System Bot]

Comment on attachment 172394 [details]
Patch

Attachment 172394 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14726747

[Early Warning System Bot]

Comment on attachment 172394 [details]
Patch

Attachment 172394 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14675328

[WebKit Review Bot]

Comment on attachment 172394 [details]
Patch

Attachment 172394 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14745206

[Emil A Eklund]

Created attachment 172408 [details]
Patch

[WebKit Review Bot]

Attachment 172408 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/LayoutRect.h:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/LayoutUnit.h:191:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:38:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:39:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 125 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Emil A Eklund]

(In reply to comment #3)
> (From update of attachment 172394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172394&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:366
> > +    return clientWidth() - min(LayoutUnit(0), layoutOverflowRect().x() - borderLeft());
> 
> Why not just LayoutUnit()?

No reason, expanded the ZERO_LAYOUT_UNIT define which was LayoutUnit(0). Changed to LayoutUnit().

[EFL EWS Bot]

Comment on attachment 172408 [details]
Patch

Attachment 172408 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14682292

[Build Bot]

Comment on attachment 172408 [details]
Patch

Attachment 172408 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14690297

[WebKit Review Bot]

Comment on attachment 172408 [details]
Patch

Attachment 172408 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14728695

[Emil A Eklund]

r133536 added a new dependency on LayoutTypes.h (which is why the mac and cr-linux bots fails), will upload a new patch momentarily.

[Emil A Eklund]

Created attachment 172430 [details]
Patch

[WebKit Review Bot]

Attachment 172430 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/LayoutRect.h:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/LayoutUnit.h:191:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:38:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:39:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 126 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Emil A Eklund]

The style failure is to be expected, the other bots should all pass assuming they run before anyone else adds a new dependency...

[EFL EWS Bot]

Comment on attachment 172430 [details]
Patch

Attachment 172430 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14755036

[EFL EWS Bot]

Comment on attachment 172430 [details]
Patch

Attachment 172430 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14665446

[Build Bot]

Comment on attachment 172430 [details]
Patch

Attachment 172430 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14717942

New failing tests:
fast/css/text-overflow-ellipsis-bidi.html
fast/multicol/span/span-as-immediate-child-property-removal.html
fast/forms/basic-textareas-quirks.html
fast/forms/input-readonly-dimmed.html
fast/replaced/width100percent-textarea.html
fast/forms/input-text-scroll-left-on-blur.html
fast/css/text-overflow-input.html
svg/text/text-overflow-ellipsis-svgfont.html
fast/css/vertical-text-overflow-ellipsis-text-align-center.html
fast/css/text-overflow-ellipsis-strict.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html
fast/forms/input-readonly-autoscroll.html
fast/css/text-overflow-ellipsis.html
fast/forms/input-disabled-color.html
fast/multicol/span/anonymous-split-block-crash.html
fast/forms/search-rtl.html
fast/forms/basic-textareas.html

[WebKit Review Bot]

Comment on attachment 172430 [details]
Patch

Attachment 172430 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14738186

New failing tests:
fast/css/vertical-text-overflow-ellipsis-text-align-center.html
fast/multicol/span/anonymous-split-block-crash.html
fast/css/text-overflow-ellipsis-text-align-center.html
svg/text/text-overflow-ellipsis-svgfont.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

[WebKit Review Bot]

Comment on attachment 172430 [details]
Patch

Attachment 172430 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14691331

New failing tests:
fast/css/vertical-text-overflow-ellipsis-text-align-center.html
fast/multicol/span/anonymous-split-block-crash.html
fast/css/text-overflow-ellipsis-text-align-center.html
svg/text/text-overflow-ellipsis-svgfont.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

[Levi Weintraub]

(In reply to comment #21)
> (From update of attachment 172430 [details])
> Attachment 172430 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14691331
> 
> New failing tests:
> fast/css/vertical-text-overflow-ellipsis-text-align-center.html
> fast/multicol/span/anonymous-split-block-crash.html
> fast/css/text-overflow-ellipsis-text-align-center.html
> svg/text/text-overflow-ellipsis-svgfont.html
> fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

Failed tests? That seems very curious.

[Emil A Eklund]

(In reply to comment #22)
> Failed tests? That seems very curious.

Seems unlikely, rerunning all tests now just to make sure.

[Levi Weintraub]

(In reply to comment #23)
> (In reply to comment #22)
> > Failed tests? That seems very curious.
> 
> Seems unlikely, rerunning all tests now just to make sure.

I was thinking the same thing, though the multicol tests are notoriously finicky with changes in LayoutUnits.

[Emil A Eklund]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Failed tests? That seems very curious.
> > 
> > Seems unlikely, rerunning all tests now just to make sure.
> 
> I was thinking the same thing, though the multicol tests are notoriously finicky with changes in LayoutUnits.

The failures are all due to the inline enclosingLayoutRect function in LayoutTypesInlineMethods.h masking a function of the same name LayoutRect.cpp.

The LayoutTypesInlineMethods versions is an alias for enclosingIntRect, this is likely incorrect but I'll make that change in a separate patch.

[Emil A Eklund]

Created attachment 172641 [details]
Patch

[WebKit Review Bot]

Attachment 172641 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/LayoutRect.h:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/LayoutUnit.h:191:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:38:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:39:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 123 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 172641 [details]
Patch

Attachment 172641 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14744515

[Emil A Eklund]

The cr-linux bot failed to remove the old FractionalLayoutUnit.h file even though it is marked as deleted in the patch :( It is almost like the bots don't like me.

[Emil A Eklund]

Created attachment 172658 [details]
Patch

[WebKit Review Bot]

Attachment 172658 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/LayoutRect.h:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/LayoutUnit.h:191:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:38:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:39:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 123 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 172658 [details]
Patch

Attachment 172658 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14755305

[EFL EWS Bot]

Comment on attachment 172658 [details]
Patch

Attachment 172658 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14745534

[EFL EWS Bot]

Comment on attachment 172658 [details]
Patch

Attachment 172658 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14748467

[Build Bot]

Comment on attachment 172658 [details]
Patch

Attachment 172658 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14762159

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/clip/overflow-border-radius-composited.html
compositing/tiling/tile-cache-zoomed.html
fast/transforms/rotated-transform-affects-scrolling-1.html
transforms/2d/hindi-rotated.html
fast/clip/overflow-border-radius-transformed.html
fast/forms/validation-message-appearance.html

[Build Bot]

Comment on attachment 172658 [details]
Patch

Attachment 172658 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14762172

New failing tests:
fast/transforms/rotated-transform-affects-scrolling-2.html
fast/clip/overflow-border-radius-composited.html
compositing/tiling/tile-cache-zoomed.html
fast/transforms/rotated-transform-affects-scrolling-1.html
transforms/2d/hindi-rotated.html
fast/clip/overflow-border-radius-transformed.html
fast/forms/validation-message-appearance.html

[Emil A Eklund]

Created attachment 172815 [details]
Patch

[WebKit Review Bot]

Attachment 172815 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/LayoutRect.h:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/LayoutUnit.h:191:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:38:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutRect.cpp:39:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/LayoutBoxExtent.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 124 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 172815 [details]
Patch

Attachment 172815 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14761371

[WebKit Review Bot]

Comment on attachment 172815 [details]
Patch

Attachment 172815 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14759414

[Emil A Eklund]

Passed on mac now and the cr-linux bot is broken again (didn't remove old files). Please review.

[Levi Weintraub]

Comment on attachment 172815 [details]
Patch

You may want to warn the troopers before trying to land this. I suspect there'll need to be some clobbers.

[Emil A Eklund]

Committed r133779: <http://trac.webkit.org/changeset/133779>