Bug 179633 - [css-grid] Refactoring and new namespace with grid related utility functions
Summary: [css-grid] Refactoring and new namespace with grid related utility functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-13 13:48 PST by Javier Fernandez
Modified: 2017-11-22 15:47 PST (History)
7 users (show)

See Also:


Attachments
Patch (28.19 KB, patch)
2017-11-13 13:49 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (30.67 KB, patch)
2017-11-13 13:55 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (28.27 KB, patch)
2017-11-20 14:12 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (30.29 KB, patch)
2017-11-21 06:33 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (31.34 KB, patch)
2017-11-21 07:21 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (31.87 KB, patch)
2017-11-22 12:36 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2017-11-13 13:48:21 PST
[css-grid] Refactoring and new Grid utility class
Comment 1 Javier Fernandez 2017-11-13 13:49:59 PST
Created attachment 326794 [details]
Patch
Comment 2 Javier Fernandez 2017-11-13 13:55:08 PST
Created attachment 326795 [details]
Patch
Comment 3 Javier Fernandez 2017-11-20 14:12:04 PST
Created attachment 327373 [details]
Patch
Comment 4 Darin Adler 2017-11-20 16:22:48 PST
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.
Comment 5 Javier Fernandez 2017-11-21 06:33:35 PST
Created attachment 327406 [details]
Patch
Comment 6 Darin Adler 2017-11-21 07:09:30 PST
> 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.
Comment 7 Darin Adler 2017-11-21 07:10:07 PST
Or you could check in a separate patch with just those includes.
Comment 8 Javier Fernandez 2017-11-21 07:17:30 PST
(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.
Comment 9 Javier Fernandez 2017-11-21 07:21:45 PST
Created attachment 327409 [details]
Patch

Added additional header include directive to fix unified build.
Comment 10 Michael Catanzaro 2017-11-21 07:53:09 PST
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.
Comment 11 Darin Adler 2017-11-21 08:43:05 PST
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.
Comment 12 Javier Fernandez 2017-11-22 12:36:03 PST
Created attachment 327469 [details]
Patch

Added additional header include directive to fix unified build on win.
Comment 13 WebKit Commit Bot 2017-11-22 15:46:19 PST
Comment on attachment 327469 [details]
Patch

Clearing flags on attachment: 327469

Committed r225104: <https://trac.webkit.org/changeset/225104>
Comment 14 WebKit Commit Bot 2017-11-22 15:46:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-11-22 15:47:19 PST
<rdar://problem/35670404>