Move TextStream into PAL.
Since TextStream does not abstract any platform, I'm not sure PAL is the right place for it. Seems like WTF would be a better fit.
Created attachment 317291 [details] Patch just namespace change Throwing at the bots before moving TextStream.h/cpp
Attachment 317291 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:102: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:103: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:112: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:113: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:114: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CalculationValue.cpp:32: Inner-style forward declarations are invalid. Remove this line. [build/forward_decl] [5] Total errors found: 9 in 232 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317300 [details] Patch just namespace change Hopefully fixing xcode builds
Attachment 317300 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:102: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:103: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:112: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:113: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:114: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 8 in 232 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Sam Weinig from comment #1) > Since TextStream does not abstract any platform, I'm not sure PAL is the > right place for it. Seems like WTF would be a better fit. Doh missed this comment before making the patch. I can move it into WTF instead if you feel that's a better choice. Might make the patch less massive in the end.
WTF is fine with me. Sorry for the bad direction.
Updating summary for new game plan
Created attachment 317468 [details] WIP Patch Replaced forward declarations with an include. Wanted to see if this is the way to go forward with this before continuing the move. Also wanted to verify this builds outside of WinCairo. Expecting the style checker to fail as I have not ordered the includes properly in this.
Attachment 317468 [details] did not pass style-queue: ERROR: Source/WebCore/page/scrolling/ScrollingStateNode.h:36: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/rendering/style/RenderStyleConstants.h:29: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:38: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/page/scrolling/ScrollingCoordinator.h:37: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/Color.h:38: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/FloatSize.h:32: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/IntSize.h:30: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/IntPoint.h:30: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/GraphicsLayer.h:42: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/rendering/RenderTreeAsText.h:29: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/CalculationValue.h:38: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/Length.h:32: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/dom/Position.h:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/FloatPoint.h:32: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/Path.h:37: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/filters/LightSource.h:29: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/LayoutRect.h:38: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/LayoutUnit.h:40: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/animation/TimingFunction.h:29: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/FontTaggedSettings.h:32: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/transforms/AffineTransform.h:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.h:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/GraphicsTypes.h:31: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/displaylists/DisplayList.h:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 25 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317519 [details] WIP Patch Just moving the namespace. Is building in WinCairo. Seeing what happens with the bots. Some formatting changes snuck in due to trailing spaces the IDE caught...
Created attachment 317520 [details] WIP Patch
Attachment 317520 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:102: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:103: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:112: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:113: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:114: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 8 in 120 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317528 [details] WIP patch Fixing Range compile issue on the bots
Attachment 317528 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:102: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:103: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:112: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:113: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:114: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:311: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 8 in 121 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317661 [details] Patch xcode builds should work
Attachment 317661 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/TextStream.h:36: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 256 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317663 [details] Patch
Attachment 317663 [details] did not pass style-queue: ERROR: Source/WebCore/platform/Length.cpp:32: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/text/TextStream.h:36: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 256 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317666 [details] Patch
Comment on attachment 317666 [details] Patch Attachment 317666 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4281704 New failing tests: svg/W3C-SVG-1.1/painting-stroke-04-t.svg svg/W3C-SVG-1.1/struct-group-03-t.svg transforms/svg-vs-css.xhtml svg/custom/dasharrayOrigin.svg svg/transforms/svg-css-transforms.xhtml svg/custom/invalid-dasharray.svg svg/css/composite-shadow-example.html svg/css/composite-shadow-with-opacity.html svg/filters/subRegion-two-effects.svg fast/repaint/moving-shadow-on-container.html svg/filters/subRegion-one-effect.svg svg/custom/js-repaint-rect-on-path-with-stroke.svg svg/custom/use-css-no-effect-on-shadow-tree.svg fast/repaint/moving-shadow-on-path.html svg/W3C-SVG-1.1/animate-elem-33-t.svg svg/W3C-SVG-1.1/animate-elem-78-t.svg svg/W3C-SVG-1.1/animate-elem-41-t.svg
Created attachment 317671 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 317666 [details] Patch Attachment 317666 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4281712 New failing tests: svg/filters/subRegion-one-effect.svg transforms/svg-vs-css.xhtml svg/custom/dasharrayOrigin.svg svg/custom/js-repaint-rect-on-path-with-stroke.svg svg/custom/invalid-dasharray.svg svg/css/composite-shadow-example.html svg/css/composite-shadow-with-opacity.html svg/filters/subRegion-two-effects.svg fast/repaint/moving-shadow-on-container.html svg/W3C-SVG-1.1/struct-group-03-t.svg svg/transforms/svg-css-transforms.xhtml svg/custom/use-css-no-effect-on-shadow-tree.svg fast/repaint/moving-shadow-on-path.html svg/W3C-SVG-1.1/animate-elem-78-t.svg svg/W3C-SVG-1.1/animate-elem-33-t.svg svg/W3C-SVG-1.1/painting-stroke-04-t.svg svg/W3C-SVG-1.1/animate-elem-41-t.svg
Created attachment 317672 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 317666 [details] Patch Attachment 317666 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4281714 New failing tests: svg/W3C-SVG-1.1/painting-stroke-04-t.svg svg/W3C-SVG-1.1/struct-group-03-t.svg transforms/svg-vs-css.xhtml svg/custom/dasharrayOrigin.svg svg/transforms/svg-css-transforms.xhtml svg/custom/invalid-dasharray.svg svg/css/composite-shadow-example.html svg/css/composite-shadow-with-opacity.html svg/filters/subRegion-two-effects.svg fast/repaint/moving-shadow-on-container.html svg/filters/subRegion-one-effect.svg svg/custom/js-repaint-rect-on-path-with-stroke.svg svg/custom/use-css-no-effect-on-shadow-tree.svg fast/repaint/moving-shadow-on-path.html svg/W3C-SVG-1.1/animate-elem-33-t.svg svg/W3C-SVG-1.1/animate-elem-78-t.svg svg/W3C-SVG-1.1/animate-elem-41-t.svg
Created attachment 317675 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 317666 [details] Patch Attachment 317666 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4282059 New failing tests: svg/W3C-SVG-1.1/painting-stroke-04-t.svg svg/W3C-SVG-1.1/struct-group-03-t.svg transforms/svg-vs-css.xhtml svg/custom/dasharrayOrigin.svg svg/custom/invalid-dasharray.svg svg/filters/subRegion-two-effects.svg svg/filters/subRegion-one-effect.svg svg/custom/js-repaint-rect-on-path-with-stroke.svg svg/custom/use-css-no-effect-on-shadow-tree.svg svg/W3C-SVG-1.1/animate-elem-78-t.svg
Created attachment 317678 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 317666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317666&action=review Those changes were required to get it compiled out. Since this was a straight copy of the file those changes didn't really make sense. Investigating now. > Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:41 > + ts << this; This change I also found weird. > Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:-142 > - Removing this turned the dash array declaration from [dash array={..}] to [dash array=[..]].
Created attachment 317746 [details] Patch
Created attachment 317747 [details] Patch
Created attachment 317748 [details] Patch
Comment on attachment 317748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317748&action=review Bots that were failing are coming back as green so I think this is good to review. > Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:104 > + If this isn't moved Clang comes back with an error for an unused function. Since DashArray is a typedef of a Vector it chooses Vector's operator << rather than the specialized one. Moving it before writeNameValuePair rectifies this.
Comment on attachment 317748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317748&action=review I'm not an OWNER of the WebKit subproject, but just the change in pathnames shouldn't require an OWNER. Aside from the "WTF::", it looks good to me. > Source/WebCore/dom/ViewportArguments.h:148 > +WTF::TextStream& operator<<(WTF::TextStream&, const ViewportArguments&); The WTF style is to say "using WTF::Foo;" at the bottom of WTF headers. Doing this means you don't need "WTF::" sprayed over WebCore. We chose a different style for PAL, and for now, we should keep the two styles different. We may wish to change the style for WTF some time in the future, but it should be its own discussion.
Comment on attachment 317748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317748&action=review >> Source/WebCore/dom/ViewportArguments.h:148 >> +WTF::TextStream& operator<<(WTF::TextStream&, const ViewportArguments&); > > The WTF style is to say "using WTF::Foo;" at the bottom of WTF headers. Doing this means you don't need "WTF::" sprayed over WebCore. > > We chose a different style for PAL, and for now, we should keep the two styles different. We may wish to change the style for WTF some time in the future, but it should be its own discussion. With TextStream there were a lot of header files where it was forward declared. In those cases I moved the forward declaration into a WTF namespace within the header. There is a using declaration within TextStream to put it into a global scope. That's why there are only WTF::TextStream usages within header files but not within source files. So in cases like this should it be forward declared without a namespace? Just making sure since I'll have to rework the headers here.
(In reply to Don Olmstead from comment #35) > With TextStream there were a lot of header files where it was forward > declared. In those cases I moved the forward declaration into a WTF > namespace within the header. This is the right thing to do.
(In reply to Myles C. Maxfield from comment #36) > (In reply to Don Olmstead from comment #35) > > With TextStream there were a lot of header files where it was forward > > declared. In those cases I moved the forward declaration into a WTF > > namespace within the header. > > This is the right thing to do. Then we good to go? :)
(In reply to Don Olmstead from comment #37) > (In reply to Myles C. Maxfield from comment #36) > > (In reply to Don Olmstead from comment #35) > > > With TextStream there were a lot of header files where it was forward > > > declared. In those cases I moved the forward declaration into a WTF > > > namespace within the header. > > > > This is the right thing to do. > > Then we good to go? :) Sorry, I misunderstood you. You're stating "WTF::" only in headers which forward-declare the WTF type. I think this is okay.
Comment on attachment 317748 [details] Patch Clearing flags on attachment: 317748 Committed r220503: <http://trac.webkit.org/changeset/220503>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33817318>
(In reply to WebKit Commit Bot from comment #39) > Comment on attachment 317748 [details] > Patch > > Clearing flags on attachment: 317748 > > Committed r220503: <http://trac.webkit.org/changeset/220503> This change broke the iOS Debug build: https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20%28Build%29/builds/4106
(In reply to Ryan Haddad from comment #42) > (In reply to WebKit Commit Bot from comment #39) > > Comment on attachment 317748 [details] > > Patch > > > > Clearing flags on attachment: 317748 > > > > Committed r220503: <http://trac.webkit.org/changeset/220503> > This change broke the iOS Debug build: > https://build.webkit.org/builders/ > Apple%20iOS%2010%20Simulator%20Debug%20%28Build%29/builds/4106 /Volumes/Data/slave/ios-simulator-10-debug/build/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1868:85: error: invalid operands to binary expression ('WTF::TextStream' and 'CGPoint') /Volumes/Data/slave/ios-simulator-10-debug/build/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2682:104: error: invalid operands to binary expression ('WTF::TextStream' and 'CGRect')
(In reply to Ryan Haddad from comment #42) > (In reply to WebKit Commit Bot from comment #39) > > Comment on attachment 317748 [details] > > Patch > > > > Clearing flags on attachment: 317748 > > > > Committed r220503: <http://trac.webkit.org/changeset/220503> > This change broke the iOS Debug build: > https://build.webkit.org/builders/ > Apple%20iOS%2010%20Simulator%20Debug%20%28Build%29/builds/4106 Working on a fix currently.
Created attachment 317844 [details] Fix for iOS build
Comment on attachment 317844 [details] Fix for iOS build Please create a ChangeLog entry.
Created attachment 317847 [details] Fix for iOS build Resubmitted with ChangeLog.
Comment on attachment 317847 [details] Fix for iOS build Clearing flags on attachment: 317847 Committed r220550: <http://trac.webkit.org/changeset/220550>