Fix multiple border rendering issues

[Simon Fraser (smfr)]

Fix multiple border rendering issues (22746, 38787, 41301, 41302, 34307, 20495, 17468, 21835

[Simon Fraser (smfr)]

Created attachment 89994 [details]
Patch

[WebKit Review Bot]

Attachment 89994 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8436881

[Simon Fraser (smfr)]

I don't understand the Chromium link failures in DRT and the unit tests, with WebCore::findIntersection(). It's a non-inlined method in FloatPoint.h, but not reference outside WebCore.

[Eric Seidel (no email)]

I think Chromium builds webcore in pieces, but I'm not sure.  I suspect it would fix things to just mark it static or inline.

[Eric Seidel (no email)]

Comment on attachment 89994 [details]
Patch

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

I'm happy to walk through this with you at the meetup... but I suspect you'll find someone willing to empower you with an r+ before then.

> Source/WebCore/platform/graphics/FloatPoint.cpp:77
> +float findSlope(const FloatPoint& p1, const FloatPoint& p2, float& c)

Odd.  I would think we would want to make this a template and put this in some sort of LineFitting.h to be re-used by both Int and FloatPoint.  Then again, if we only need it for FloatPoint now, such doens't need to be done now and can be done when it's actually needed elsewehre.

> Source/WebCore/platform/graphics/FloatPoint.h:208
> +bool findIntersection(const FloatPoint& p1, const FloatPoint& p2, const FloatPoint& d1, const FloatPoint& d2, FloatPoint& intersection);

Ah, ignore my previous comments in the bug about wanting to inline this stuff. :)  I think Chromium will just need to fix how their .a files assemble.

> Source/WebCore/platform/graphics/IntRect.h:136
> +    IntPoint minXMinYCorner() const { return m_location; } // topLeft

If these are always topLeft, why not call it that?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1077
> +struct BorderEdge {

structs so rarely stay structs.  Generally better to use classes with everything marked as public: to avoid forward-declaration confusion.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1210
> +void RenderBoxModelObject::paintOneBorderSide(GraphicsContext* graphicsContext, const RenderStyle* style, const RoundedIntRect& outerBorder, const RoundedIntRect& innerBorder,
> +    const IntRect& sideRect, BoxSide side, BoxSide adjacentSide1, BoxSide adjacentSide2, const BorderEdge edges[], const Path* path, bool antialias, const Color* overrideColor)

Impressive. :)

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1225
> +        graphicsContext->save();

I wish we had a RIIA to help with this sort of thing.  Again, not really a comment on your change, just a general comment on the rendering code.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1384
> +    for (int i = BSTop; i <= BSLeft; ++i) {
> +        const BorderEdge& currEdge = edges[i];

I might have split this for out into a helper function.  Possibly even thrown all of those bools/ints into a helper class EdgeInformation, then this could all be a side-effect of constructing EdgeInformation from edges. :)  Or maybe eventually Edges should be a class itself with this summary information.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1415
> +            // FIXME: Path should take a RoundedIntRect directly.
> +            if (outerBorder.isRounded())
> +                path.addRoundedRect(outerBorder.rect(), outerBorder.radii().topLeft(), outerBorder.radii().topRight(), outerBorder.radii().bottomLeft(), outerBorder.radii().bottomRight());
> +            else
> +                path.addRect(outerBorder.rect());

Feels like a addRoundedRect(path, border) helper in the meanwhile.

[Simon Fraser (smfr)]

(In reply to comment #5)
> (From update of attachment 89994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89994&action=review
> 
> I'm happy to walk through this with you at the meetup... but I suspect you'll find someone willing to empower you with an r+ before then.
> 
> > Source/WebCore/platform/graphics/FloatPoint.cpp:77
> > +float findSlope(const FloatPoint& p1, const FloatPoint& p2, float& c)
> 
> Odd.  I would think we would want to make this a template and put this in some sort of LineFitting.h to be re-used by both Int and FloatPoint.  Then again, if we only need it for FloatPoint now, such doens't need to be done now and can be done when it's actually needed elsewehre.

https://bugs.webkit.org/show_bug.cgi?id=58808

> > Source/WebCore/platform/graphics/FloatPoint.h:208
> > +bool findIntersection(const FloatPoint& p1, const FloatPoint& p2, const FloatPoint& d1, const FloatPoint& d2, FloatPoint& intersection);
> 
> Ah, ignore my previous comments in the bug about wanting to inline this stuff. :)  I think Chromium will just need to fix how their .a files assemble.
> 
> > Source/WebCore/platform/graphics/IntRect.h:136
> > +    IntPoint minXMinYCorner() const { return m_location; } // topLeft
> 
> If these are always topLeft, why not call it that?

They aren't in vertical text. Will adjust the comments.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1077
> > +struct BorderEdge {
> 
> structs so rarely stay structs.  Generally better to use classes with everything marked as public: to avoid forward-declaration confusion.

Will fix.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1210
> > +void RenderBoxModelObject::paintOneBorderSide(GraphicsContext* graphicsContext, const RenderStyle* style, const RoundedIntRect& outerBorder, const RoundedIntRect& innerBorder,
> > +    const IntRect& sideRect, BoxSide side, BoxSide adjacentSide1, BoxSide adjacentSide2, const BorderEdge edges[], const Path* path, bool antialias, const Color* overrideColor)
> 
> Impressive. :)
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1225
> > +        graphicsContext->save();
> 
> I wish we had a RIIA to help with this sort of thing.  Again, not really a comment on your change, just a general comment on the rendering code.

https://bugs.webkit.org/show_bug.cgi?id=58807

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1384
> > +    for (int i = BSTop; i <= BSLeft; ++i) {
> > +        const BorderEdge& currEdge = edges[i];
> 
> I might have split this for out into a helper function.  Possibly even thrown all of those bools/ints into a helper class EdgeInformation, then this could all be a side-effect of constructing EdgeInformation from edges. :)  Or maybe eventually Edges should be a class itself with this summary information.

Hmm, it's only used once. I don't really see the utility of making this more complex.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1415
> > +            // FIXME: Path should take a RoundedIntRect directly.
> > +            if (outerBorder.isRounded())
> > +                path.addRoundedRect(outerBorder.rect(), outerBorder.radii().topLeft(), outerBorder.radii().topRight(), outerBorder.radii().bottomLeft(), outerBorder.radii().bottomRight());
> > +            else
> > +                path.addRect(outerBorder.rect());
> 
> Feels like a addRoundedRect(path, border) helper in the meanwhile.

Yep. https://bugs.webkit.org/show_bug.cgi?id=58809

[James Robinson]

I'm applying this patch locally to see what the cr-mac failure is.

[James Robinson]

Output from my Snow Leopard chromium build (fails in release only):


    /Developer/usr/bin/gcc-4.2 -x c++ -arch i386 -fmessage-length=0 -pipe -Wno-trigraphs -fno-exceptions -fno-rtti -O3 -Werror -Wnewline-eof -DCHROMIUM_BUILD -DENABLE_REMOTING=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 "-DWEBCORE_NAVIGATOR_VENDOR=\"Google Inc.\"" -DWEBCORE_NAVIGATOR_PLATFORM="MacIntel" -DScrollbarPrefsObserver=ChromiumWebCoreObjCScrollbarPrefsObserver -DWebCoreRenderThemeNotificationObserver=ChromiumWebCoreObjCWebCoreRenderThemeNotificationObserver -DWebFontCache=ChromiumWebCoreObjCWebFontCache -DENABLE_WEBGL=1 -DENABLE_3D_RENDERING=1 -DENABLE_ACCELERATED_2D_CANVAS=1 -DENABLE_BLOB=1 -DENABLE_BLOB_SLICE=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CLIENT_BASED_GEOLOCATION=1 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATABASE=1 -DENABLE_DATAGRID=0 -DENABLE_DATA_TRANSFER_ITEMS=1 -DENABLE_DEVICE_ORIENTATION=1 -DENABLE_DIRECTORY_UPLOAD=1 -DENABLE_DOM_STORAGE=1 -DENABLE_EVENTSOURCE=1 -DENABLE_JAVASCRIPT_I18N_API=1 -DENABLE_FILE_SYSTEM=1 -DENABLE_FILTERS=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GEOLOCATION=1 -DENABLE_GESTURE_RECOGNIZER=1 -DENABLE_ICONDATABASE=0 -DENABLE_INDEXED_DATABASE=1 -DENABLE_INPUT_SPEECH=1 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_JSC_MULTIPLE_THREADS=0 -DENABLE_LEVELDB=1 -DENABLE_LINK_PREFETCH=1 -DENABLE_MATHML=0 -DENABLE_MEDIA_STATISTICS=1 -DENABLE_MEDIA_STREAM=1 -DENABLE_METER_TAG=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_OFFLINE_WEB_APPLICATIONS=1 -DENABLE_OPENTYPE_SANITIZER=1 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_PAGE_VISIBILITY_API=0 -DENABLE_PROGRESS_TAG=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=0 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_SVG=1 -DENABLE_SVG_ANIMATION=1 -DENABLE_SVG_AS_IMAGE=1 -DENABLE_SVG_FONTS=1 -DENABLE_SVG_FOREIGN_OBJECT=1 -DENABLE_SVG_USE=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_V8_SCRIPT_DEBUG_SERVER=1 -DENABLE_VIDEO=1 -DENABLE_WEB_AUDIO=0 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DENABLE_XHR_RESPONSE_BLOB=1 -DENABLE_XPATH=1 -DENABLE_XSLT=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DWTF_USE_WEBP=1 -DWTF_USE_WEBKIT_IMAGE_DECODERS=1 -DBUILDING_CHROMIUM__=1 -DUSE_SYSTEM_MALLOC=1 -DWTF_USE_NEW_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DSK_BUILD_NO_IMAGE_ENCODE -DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h" -DGR_AGGRESSIVE_SHADER_OPTS=1 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -isysroot /Developer/SDKs/MacOSX10.5.sdk -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -mmacosx-version-min=10.5 -gdwarf-2 -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -F/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/Release -F/Developer/SDKs/MacOSX10.5.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks -I/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/Release/include -I../../WebKit/chromium/third_party/icu/public/common -I../../WebKit/chromium/third_party/icu/public/i18n -I../../../WebKitLibraries -I../../WebKit/chromium/gpu -I/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/WebCore.build/DerivedSources/Release -I../platform/graphics/cocoa -I../platform/graphics/cg -I.. -I../.. -I../accessibility -I../accessibility/chromium -I../bindings -I../bindings/generic -I../bindings/v8 -I../bindings/v8/custom -I../bindings/v8/specialization -I../bridge -I../bridge/jni -I../bridge/jni/v8 -I../css -I../dom -I../dom/default -I../editing -I../fileapi -I../history -I../html -I../html/canvas -I../html/parser -I../html/shadow -I../inspector -I../loader -I../loader/appcache -I../loader/archive -I../loader/cache -I../loader/icon -I../mathml -I../notifications -I../page -I../page/animation -I../page/chromium -I../platform -I../platform/animation -I../platform/audio -I../platform/audio/chromium -I../platform/chromium -I../platform/graphics -I../platform/graphics/chromium -I../platform/graphics/filters -I../platform/graphics/gpu -I../platform/graphics/opentype -I../platform/graphics/skia -I../platform/graphics/transforms -I../platform/image-decoders -I../platform/image-decoders/bmp -I../platform/image-decoders/gif -I../platform/image-decoders/ico -I../platform/image-decoders/jpeg -I../platform/image-decoders/png -I../platform/image-decoders/skia -I../platform/image-decoders/xbm -I../platform/image-decoders/webp -I../platform/image-encoders/skia -I../platform/leveldb -I../platform/mock -I../platform/network -I../platform/network/chromium -I../platform/sql -I../platform/text -I../platform/text/transcoder -I../plugins -I../plugins/chromium -I../rendering -I../rendering/style -I../rendering/svg -I../storage -I../storage/chromium -I../svg -I../svg/animation -I../svg/graphics -I../svg/graphics/filters -I../svg/properties -I../../ThirdParty/glu -I../webaudio -I../websockets -I../workers -I../xml -I../platform/audio/mac -I../platform/cocoa -I../platform/graphics/mac -I../platform/mac -I../platform/text/mac -I../../WebKit/chromium/third_party/angle/include/GLSLANG -I/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/DerivedSources/Release/webkit -I/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/DerivedSources/Release/webkit/bindings -I../../JavaScriptCore -I../../JavaScriptCore/wtf -I../../WebKit/chromium -I../../WebKit/chromium/skia/config -I../../WebKit/chromium/third_party/skia/include/config -I../../WebKit/chromium/third_party/skia/include/core -I../../WebKit/chromium/third_party/skia/include/effects -I../../WebKit/chromium/third_party/skia/include/pdf -I../../WebKit/chromium/third_party/skia/include/gpu -I../../WebKit/chromium/third_party/skia/include/ports -I../../WebKit/chromium/third_party/skia/gpu/include -I../../WebKit/chromium/skia/ext -I../../WebKit/chromium/third_party/iccjpeg -I../../WebKit/chromium/third_party/libwebp -I../../WebKit/chromium/third_party/libpng -I../../WebKit/chromium/third_party/zlib -I../../WebKit/chromium/third_party/libxml/mac/include -I../../WebKit/chromium/third_party/libxml/src/include -I../../WebKit/chromium/third_party/libxslt -I../../WebKit/chromium/third_party/npapi -I../../WebKit/chromium/third_party/npapi/bindings -I../../WebKit/chromium/third_party/ots/include -I../../WebKit/chromium/third_party/sqlite -I../../WebKit/chromium/third_party/libjpeg_turbo -I../../WebKit/chromium/v8/include -I../../WebKit/chromium/third_party/leveldb/include -I/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/WebCore.build/Release/webcore_platform.build/DerivedSources/i386 -I/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/WebCore.build/Release/webcore_platform.build/DerivedSources -include /var/folders/++/++-NQU++6+0++4RjPqRgNE++KL6/-Caches-/com.apple.Xcode.22898/SharedPrecompiledHeaders/WebCorePrefix-cwtzftagjhaclifkastdzarpedbh/WebCorePrefix.h -c /usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/FloatPoint.cpp -o /usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../../WebKit/chromium/xcodebuild/WebCore.build/Release/webcore_platform.build/Objects-normal/i386/FloatPoint.o

cc1plus: warnings being treated as errors
/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/FloatPoint.cpp: In function 'bool WebCore::findIntersection(const WebCore::FloatPoint&, const WebCore::FloatPoint&, const WebCore::FloatPoint&, const WebCore::FloatPoint&, WebCore::FloatPoint&)':
/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/FloatPoint.cpp:90: warning: 'pOffset' may be used uninitialized in this function
/usr/local/home/jamesr/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/FloatPoint.cpp:93: warning: 'dOffset' may be used uninitialized in this function
{standard input}:152:non-relocatable subtraction expression, "LC0" minus "L00000000001$pb"
{standard input}:152:symbol: "LC0" can't be undefined in a subtraction expression
{standard input}:unknown:Undefined local symbol LC0

[Simon Fraser (smfr)]

Created attachment 90126 [details]
Patch for code changes

[Simon Fraser (smfr)]

Created attachment 90127 [details]
LayoutTests patch

[Simon Fraser (smfr)]

*** Bug 58809 has been marked as a duplicate of this bug. ***

[Dave Hyatt]

Comment on attachment 90127 [details]
LayoutTests patch

r=me on layout tests.

[Dave Hyatt]

Comment on attachment 90126 [details]
Patch for code changes

r=me on patch.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/84273

[WebKit Review Bot]

http://trac.webkit.org/changeset/84273 might have broken Qt Linux Release minimal and Qt Linux ARMv7 Release

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/84278