Bug 175211 – [WTF] Move TextStream into WTF

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Don Olmstead]

Move TextStream into PAL.

[Sam Weinig]

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.

[Don Olmstead]

Created attachment 317291 [details]
Patch just namespace change

Throwing at the bots before moving TextStream.h/cpp

[Build Bot]

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.

[Don Olmstead]

Created attachment 317300 [details]
Patch just namespace change

Hopefully fixing xcode builds

[Build Bot]

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.

[Don Olmstead]

(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.

[Myles C. Maxfield]

WTF is fine with me. Sorry for the bad direction.

[Don Olmstead]

Updating summary for new game plan

[Don Olmstead]

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.

[Build Bot]

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.

[Don Olmstead]

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...

[Don Olmstead]

Created attachment 317520 [details]
WIP Patch

[Build Bot]

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.

[Don Olmstead]

Created attachment 317528 [details]
WIP patch

Fixing Range compile issue on the bots

[Build Bot]

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.

[Don Olmstead]

Created attachment 317661 [details]
Patch

xcode builds should work

[Build Bot]

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.

[Don Olmstead]

Created attachment 317663 [details]
Patch

[Build Bot]

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.

[Don Olmstead]

Created attachment 317666 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Don Olmstead]

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=[..]].

[Don Olmstead]

Created attachment 317746 [details]
Patch

[Don Olmstead]

Created attachment 317747 [details]
Patch

[Don Olmstead]

Created attachment 317748 [details]
Patch

[Don Olmstead]

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.

[Myles C. Maxfield]

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.

[Don Olmstead]

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.

[Myles C. Maxfield]

(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.

[Don Olmstead]

(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? :)

[Myles C. Maxfield]

(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.

[WebKit Commit Bot]

Comment on attachment 317748 [details]
Patch

Clearing flags on attachment: 317748

Committed r220503: <http://trac.webkit.org/changeset/220503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33817318>

[Ryan Haddad]

(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

[Ryan Haddad]

(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')

[Don Olmstead]

(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.

[Ross Kirsling]

Created attachment 317844 [details]
Fix for iOS build

[Myles C. Maxfield]

Comment on attachment 317844 [details]
Fix for iOS build

Please create a ChangeLog entry.

[Ross Kirsling]

Created attachment 317847 [details]
Fix for iOS build

Resubmitted with ChangeLog.

[Ryan Haddad]

Comment on attachment 317847 [details]
Fix for iOS build

Clearing flags on attachment: 317847

Committed r220550: <http://trac.webkit.org/changeset/220550>

[Ryan Haddad]

All reviewed patches have been landed.  Closing bug.