Bug 58761

Summary: Fix multiple border rendering issues
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bdakin, dglazkov, eric, jamesr, mitz, peter, phiw2, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 17468, 20495, 21835, 22746, 41301, 41302    
Attachments:
Description Flags
Patch
none
Patch for code changes
hyatt: review+
LayoutTests patch hyatt: review+

Description Simon Fraser (smfr) 2011-04-17 22:26:45 PDT
Fix multiple border rendering issues (22746, 38787, 41301, 41302, 34307, 20495, 17468, 21835
Comment 1 Simon Fraser (smfr) 2011-04-17 22:57:27 PDT
Created attachment 89994 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-18 00:56:25 PDT
Attachment 89994 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8436881
Comment 3 Simon Fraser (smfr) 2011-04-18 08:15:05 PDT
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.
Comment 4 Eric Seidel (no email) 2011-04-18 08:48:01 PDT
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.
Comment 5 Eric Seidel (no email) 2011-04-18 09:01:30 PDT
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.
Comment 6 Simon Fraser (smfr) 2011-04-18 12:16:01 PDT
(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
Comment 7 James Robinson 2011-04-18 13:13:09 PDT
I'm applying this patch locally to see what the cr-mac failure is.
Comment 8 James Robinson 2011-04-18 13:49:22 PDT
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
Comment 9 Simon Fraser (smfr) 2011-04-18 18:48:42 PDT
Created attachment 90126 [details]
Patch for code changes
Comment 10 Simon Fraser (smfr) 2011-04-18 18:49:05 PDT
Created attachment 90127 [details]
LayoutTests patch
Comment 11 Simon Fraser (smfr) 2011-04-19 10:30:05 PDT
*** Bug 58809 has been marked as a duplicate of this bug. ***
Comment 12 Dave Hyatt 2011-04-19 11:37:49 PDT
Comment on attachment 90127 [details]
LayoutTests patch

r=me on layout tests.
Comment 13 Dave Hyatt 2011-04-19 11:39:51 PDT
Comment on attachment 90126 [details]
Patch for code changes

r=me on patch.
Comment 14 Simon Fraser (smfr) 2011-04-19 11:45:59 PDT
http://trac.webkit.org/changeset/84273
Comment 15 WebKit Review Bot 2011-04-19 11:54:07 PDT
http://trac.webkit.org/changeset/84273 might have broken Qt Linux Release minimal and Qt Linux ARMv7 Release
Comment 16 Simon Fraser (smfr) 2011-04-19 12:07:42 PDT
http://trac.webkit.org/changeset/84278