Bug 179633 – [css-grid] Refactoring and new namespace with grid related utility functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Javier Fernandez]

[css-grid] Refactoring and new Grid utility class

[Javier Fernandez]

Created attachment 326794 [details]
Patch

[Javier Fernandez]

Created attachment 326795 [details]
Patch

[Javier Fernandez]

Created attachment 327373 [details]
Patch

[Darin Adler]

Comment on attachment 327373 [details]
Patch

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

The name "Utils" goes against WebKit programming tradition; we try to steer clear of the concept "utilities" and we also don’t abbreviate words with things like "utils" if we can avoid it. It’s also not best style to use a class just as a namespace for some functions. I suggest just naming this GridLayout, or if that really seems wrong, GridLayoutFunctions (as with LengthFunctions). And we should use a namespace rather than a class.

Otherwise this seems fine, but please upload a version that builds.

> Source/WebCore/rendering/GridLayoutUtils.cpp:25
> + */
> +#include "config.h"

Missing blank line.

[Javier Fernandez]

Created attachment 327406 [details]
Patch

[Darin Adler]

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1335:122: error: invalid use of incomplete type ‘class WebCore::LayoutState’

This means RenderBlockLineLayout.cpp is missing include of "LayoutState.h", hidden by unified build file grouping.

> Source/WebCore/rendering/RootInlineBox.cpp:339:44: error: invalid use of incomplete type ‘class WebCore::LayoutState’

Same problem in RootInlineBox.cpp.

If you add those two includes to your patch, should fix the build errors.

[Darin Adler]

Or you could check in a separate patch with just those includes.

[Javier Fernandez]

(In reply to Darin Adler from comment #7)
> Or you could check in a separate patch with just those includes.

Adding an include to LayoutState.h in the mentioned files indeed solves the compilation errors. However, I wonder why only fail with my patch. Would adding a new file to the Source.txt file cause such errors ? Should we expect more like that in the future ? Is expectable that we will have to add additional headers as we made changes in the Source.txt, which may lead to changes in the compilation units ?

I can add those extra includes in this patch, but such changes seems totally unrelated to the patch.

[Javier Fernandez]

Created attachment 327409 [details]
Patch

Added additional header include directive to fix unified build.

[Michael Catanzaro]

Yes, adding a new source file changes the groupings of which files build together, so failures like this are expected. Of course, these missing includes were preexisting bugs that could have been mitigated if a tool like include-what-you-use was integrated with the style checker.

[Darin Adler]

Windows build shows additional errors of the same type due to the unified build.

rendering/RenderMediaControls.cpp(45): error C2065: 'MediaSliderThumb': undeclared identifier
rendering/RenderMediaControls.cpp(48): error C2065: 'MediaVolumeSliderThumb': undeclared identifier
rendering/RenderMediaControls.cpp(51): error C2065: 'MediaFullScreenVolumeSliderThumb': undeclared identifier

In this case the missing header is MediaControlElementTypes.h.

[Javier Fernandez]

Created attachment 327469 [details]
Patch

Added additional header include directive to fix unified build on win.

[WebKit Commit Bot]

Comment on attachment 327469 [details]
Patch

Clearing flags on attachment: 327469

Committed r225104: <https://trac.webkit.org/changeset/225104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35670404>