Summary: | [css-grid] Refactoring and new namespace with grid related utility functions | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Javier Fernandez <jfernandez> | ||||||||||||||
Component: | New Bugs | Assignee: | Javier Fernandez <jfernandez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, jfernandez, mcatanzaro, rego, svillar, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Javier Fernandez
2017-11-13 13:48:21 PST
Created attachment 326794 [details]
Patch
Created attachment 326795 [details]
Patch
Created attachment 327373 [details]
Patch
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. Created attachment 327406 [details]
Patch
> 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. Or you could check in a separate patch with just those includes. (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. Created attachment 327409 [details]
Patch
Added additional header include directive to fix unified build.
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. 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. Created attachment 327469 [details]
Patch
Added additional header include directive to fix unified build on win.
Comment on attachment 327469 [details] Patch Clearing flags on attachment: 327469 Committed r225104: <https://trac.webkit.org/changeset/225104> All reviewed patches have been landed. Closing bug. |