Bug 86176 – ietestcenter/css3/valuesandunits/units-000.htm asserts

• Component: CSS
• Status: RESOLVED WORKSFORME

[Csaba Osztrogonác]

This test introduced in https://trac.webkit.org/changeset/116658
and asserts from the beginning. Here is the crash log from the Qt bot:
(It asserts on Mac Lion too, but there isn't a crash log on it.)
crash log for DumpRenderTree (pid 16682):
STDOUT: <empty>
STDERR: SHOULD NEVER BE REACHED
STDERR: ../../../../Source/WebCore/css/CSSPrimitiveValue.cpp(505) : double WebCore::CSSPrimitiveValue::computeLengthDouble(WebCore::RenderStyle*, WebCore::RenderStyle*, float, bool)
STDERR: 1   0x7f4832737562 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::CSSPrimitiveValue::computeLengthDouble(WebCore::RenderStyle*, WebCore::RenderStyle*, float, bool)+0x244) [0x7f4832737562]
STDERR: 2   0x7f4832737167 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(unsigned int WebCore::CSSPrimitiveValue::computeLength<unsigned int>(WebCore::RenderStyle*, WebCore::RenderStyle*, float, bool)+0x3f) [0x7f4832737167]
STDERR: 3   0x7f48327ab499 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ApplyPropertyComputeLength<unsigned int, &(WebCore::RenderStyle::borderRightWidth() const), &(WebCore::RenderStyle::setBorderRightWidth(unsigned int)), &(WebCore::RenderStyle::initialBorderWidth()), (WebCore::ComputeLengthNormal)0, (WebCore::ComputeLengthThickness)1, (WebCore::ComputeLengthSVGZoom)0>::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*)+0xd6) [0x7f48327ab499]
STDERR: 4   0x7f48327897de /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::PropertyHandler::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*) const+0x74) [0x7f48327897de]
STDERR: 5   0x7f48327fd321 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::StyleResolver::applyProperty(WebCore::CSSPropertyID, WebCore::CSSValue*)+0x263) [0x7f48327fd321]
STDERR: 6   0x7f4832814b30 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(void WebCore::StyleResolver::applyProperties<false>(WebCore::StylePropertySet const*, WebCore::StyleRule*, bool, bool, bool)+0x1e6) [0x7f4832814b30]
STDERR: 7   0x7f483280e29e /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(void WebCore::StyleResolver::applyMatchedProperties<false>(WebCore::StyleResolver::MatchResult const&, bool, int, int, bool)+0x1d8) [0x7f483280e29e]
STDERR: 8   0x7f48327fc5e9 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::StyleResolver::applyMatchedProperties(WebCore::StyleResolver::MatchResult const&)+0x3df) [0x7f48327fc5e9]
STDERR: 9   0x7f48327f6b52 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::StyleResolver::styleForElement(WebCore::Element*, WebCore::RenderStyle*, WebCore::StyleSharingBehavior, WebCore::RuleMatchingBehavior, WebCore::RenderRegion*)+0x462) [0x7f48327f6b52]
STDERR: 10  0x7f48328baa43 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Element::styleForRenderer()+0x83) [0x7f48328baa43]
STDERR: 11  0x7f4832911979 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::NodeRendererFactory::createRendererIfNeeded()+0x149) [0x7f4832911979]
STDERR: 12  0x7f48328f42ca /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Node::createRendererIfNeeded()+0x2e) [0x7f48328f42ca]
STDERR: 13  0x7f48328ba35c /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Element::attach()+0x2c) [0x7f48328ba35c]
STDERR: 14  0x7f4832b248a5 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(+0x25b48a5) [0x7f4832b248a5]
STDERR: 15  0x7f4832b24bb4 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLConstructionSite::executeQueuedTasks()+0x76) [0x7f4832b24bb4]
STDERR: 16  0x7f4832b4d72f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&)+0x145) [0x7f4832b4d72f]
STDERR: 17  0x7f4832b4d534 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&)+0x66) [0x7f4832b4d534]
STDERR: 18  0x7f4832b2c9ab /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)+0x327) [0x7f4832b2c9ab]
STDERR: 19  0x7f4832b2c271 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)+0xb1) [0x7f4832b2c271]
STDERR: 20  0x7f4832b2d0cb /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&)+0x14b) [0x7f4832b2d0cb]
STDERR: 21  0x7f48328486dc /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long)+0xca) [0x7f48328486dc]
STDERR: 22  0x7f4832ce7c4a /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentWriter::addData(char const*, unsigned long)+0xe2) [0x7f4832ce7c4a]
STDERR: 23  0x7f4832cda527 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentLoader::commitData(char const*, unsigned long)+0x1e1) [0x7f4832cda527]
STDERR: 24  0x7f483245951d /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::FrameLoaderClientQt::committedLoad(WebCore::DocumentLoader*, char const*, int)+0x43) [0x7f483245951d]
STDERR: 25  0x7f4832cda2e3 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentLoader::commitLoad(char const*, int)+0x97) [0x7f4832cda2e3]
STDERR: 26  0x7f4832cda5d1 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentLoader::receivedData(char const*, int)+0x51) [0x7f4832cda5d1]
STDERR: 27  0x7f4832d14943 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::MainResourceLoader::addData(char const*, int, bool)+0x51) [0x7f4832d14943]
STDERR: 28  0x7f4832d24649 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool)+0x5b) [0x7f4832d24649]
STDERR: 29  0x7f4832d161b2 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool)+0x1b4) [0x7f4832d161b2]
STDERR: 30  0x7f4832d25134 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)+0x90) [0x7f4832d25134]
STDERR: 31  0x7f48331e0231 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::QNetworkReplyHandler::forwardData()+0x18d) [0x7f48331e0231]

[Csaba Osztrogonác]

I skipped it on Qt to make the bot green - https://trac.webkit.org/changeset/116734

[Alexey Proskuryakov]

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

[Yi Shen]

Created attachment 142042 [details]
Fix the assertion

[WebKit Review Bot]

Attachment 142042 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:229:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSPrimitiveValue.h:330:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 142042 [details]
Fix the assertion

Attachment 142042 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12705598

New failing tests:
inspector/profiler/heap-snapshot-comparison-expansion-preserved-when-sorting.html

[WebKit Review Bot]

Created attachment 142102 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Antti Koivisto]

Comment on attachment 142042 [details]
Fix the assertion

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:525
> +    else if (m_primitiveUnitType == CSS_VW)
> +        computedValue = getDoubleValue() * renderView->viewportSize().width();
> +    else if (m_primitiveUnitType == CSS_VH)
> +        computedValue = getDoubleValue() * renderView->viewportSize().height();
> +    else if (m_primitiveUnitType == CSS_VMIN)
> +        computedValue = getDoubleValue() * std::min(renderView->viewportSize().height(), renderView->viewportSize().width());

Why is it ok to dereference renderView without checking? It defaults to null.

[Yi Shen]

Thanks for the comment. An assertion for the renderView added above for vw/vh/vmin.

+        case CSS_VW:
+        case CSS_VH:
+        case CSS_VMIN:
+            ASSERT(renderView);
+            factor = 0.01;      
+            break;

(In reply to comment #7)
> (From update of attachment 142042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142042&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:525
> > +    else if (m_primitiveUnitType == CSS_VW)
> > +        computedValue = getDoubleValue() * renderView->viewportSize().width();
> > +    else if (m_primitiveUnitType == CSS_VH)
> > +        computedValue = getDoubleValue() * renderView->viewportSize().height();
> > +    else if (m_primitiveUnitType == CSS_VMIN)
> > +        computedValue = getDoubleValue() * std::min(renderView->viewportSize().height(), renderView->viewportSize().width());
> 
> Why is it ok to dereference renderView without checking? It defaults to null.

[Yi Shen]

Created attachment 142258 [details]
Retry the chromium test

[WebKit Review Bot]

Attachment 142258 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:229:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSPrimitiveValue.h:330:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yi Shen]

Add Dave Tharp's comment here for discussion.

1.  A fundamental question about the spec: it states: "The viewport-percentage lengths are relative to the size of the initial containing block. When the height or width of the viewport is changed,
they are scaled accordingly."   Your fix seems to reference the viewport,
which seems to be the size of the viewable area of the page.  The spec's wording ("initial containing block") led me to believe the size should be relative to the parent element.  For example in the units-000.htm test, the parent div is 1 inch wide so the "vw" div should be 100% of 1 inch wide.  Your code makes it as wide as the entire viewport.  I am not sure which is correct at this point...  do you have a good argument as to why vw should be based on the viewport and not parent?

2.  A code issue:  I see you put an assert in to check for valid renderView pointer.  This will only catch a NULL pointer in a debug build.
 Later in the code you de-reference renderView without a check.  This is likely to segfault in a non-debug build.

[Yi Shen]

(In reply to comment #11)
> Add Dave Tharp's comment here for discussion.
> 1.  A fundamental question about the spec: it states: "The viewport-percentage lengths are relative to the size of the initial containing block. When the height or width of the viewport is changed,
> they are scaled accordingly."   Your fix seems to reference the viewport,
> which seems to be the size of the viewable area of the page.  The spec's wording ("initial containing block") led me to believe the size should be relative to the parent element.  For example in the units-000.htm test, the parent div is 1 inch wide so the "vw" div should be 100% of 1 inch wide.  Your code makes it as wide as the entire viewport.  I am not sure which is correct at this point...  do you have a good argument as to why vw should be based on the viewport and not parent?
hmmm. I checked the existing implementation for vh/vw/vmin in WebCore/css/SytleBuilder.cpp and LengthFunctions.cpp, and it seems they all reference the viewport size. But I aggree that the units-000.htm expects the vm/vw/wmin should be based on the parent, rather than the viewport. 

Dave, feel free to upload your patch if it goes the correct solution :) 

> 2.  A code issue:  I see you put an assert in to check for valid renderView pointer.  This will only catch a NULL pointer in a debug build.
>  Later in the code you de-reference renderView without a check.  This is likely to segfault in a non-debug build.
agree, it should return -1.0 when renderview is NUll. Thanks Antti's comments also :)

[Dave Tharp]

(In reply to comment #12)
> (In reply to comment #11)
> > Add Dave Tharp's comment here for discussion.
> > 1.  A fundamental question about the spec: it states: "The viewport-percentage lengths are relative to the size of the initial containing block. When the height or width of the viewport is changed,
> > they are scaled accordingly."   Your fix seems to reference the viewport,
> > which seems to be the size of the viewable area of the page.  The spec's wording ("initial containing block") led me to believe the size should be relative to the parent element.  For example in the units-000.htm test, the parent div is 1 inch wide so the "vw" div should be 100% of 1 inch wide.  Your code makes it as wide as the entire viewport.  I am not sure which is correct at this point...  do you have a good argument as to why vw should be based on the viewport and not parent?
> hmmm. I checked the existing implementation for vh/vw/vmin in WebCore/css/SytleBuilder.cpp and LengthFunctions.cpp, and it seems they all reference the viewport size. But I aggree that the units-000.htm expects the vm/vw/wmin should be based on the parent, rather than the viewport. 
> 
> Dave, feel free to upload your patch if it goes the correct solution :) 
> 
> > 2.  A code issue:  I see you put an assert in to check for valid renderView pointer.  This will only catch a NULL pointer in a debug build.
> >  Later in the code you de-reference renderView without a check.  This is likely to segfault in a non-debug build.
> agree, it should return -1.0 when renderview is NUll. Thanks Antti's comments also :)

I'm close to finishing my patch... I found a few stragglers where I have to find the parentStyle to pass down.  I'm running out of time for today, but will post my patch tomorrow morning (PDT) for sure.

[Dave Tharp]

Created attachment 142801 [details]
Patch

[Dave Tharp]

(In reply to comment #14)
> Created an attachment (id=142801) [details]
> Patch
Ok, better late than never, right? :)

I am not sure my approach was the right one, so I'm ready for any comments/criticisms from the reviewers. I touched alot of code, but I have tested on several platforms and all seems to be well.  The original assert is handled, and the IETC units tests pass for the vh, vw and vmin cases (as well as the new tests I wrote for each of these specifically).

[Early Warning System Bot]

Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12723786

[Early Warning System Bot]

Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12725632

[WebKit Review Bot]

Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724604

[Build Bot]

Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12719977

[Dave Tharp]

Created attachment 142820 [details]
Patch

[Dave Tharp]

(In reply to comment #20)
> Created an attachment (id=142820) [details]
> Patch
Fixed new instance of usage of computeLength() to include parentStyle pointer.

[WebKit Review Bot]

Attachment 142820 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:99:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. animations/animation-api-1.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3762:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/compiler-script-mapping.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3764:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/cross-frame-access-put.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3766:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. compositing/geometry/object-clip-rects-assertion.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3767:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/appcache/appcache-iframe-manifests.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3768:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/network/async-xhr-json-mime-type.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3769:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/network/network-cachedresources-with-same-urls.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3770:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/network/network-content-replacement-xhr.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3771:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/escape-form-data-field-names.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3772:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/feed-urls-from-remote.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3773:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/referrer-policy-redirect-link.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3774:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. platform/chromium/virtual/gpu/fast/canvas/imagedata-contains-uint8clampedarray.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3775:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes-repaint.html  [test/expectations] [5]
Total errors found: 13 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 142820 [details]
Patch

Attachment 142820 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12719995

[Dave Tharp]

Created attachment 143085 [details]
Patch

[Dave Tharp]

(In reply to comment #24)
> Created an attachment (id=143085) [details]
> Patch
Fixes mac build failure.

[WebKit Review Bot]

Attachment 143085 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/efl/test_expectations.txt:6:  fast/dom/shadow/content-element-api.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:7:  fast/dom/shadow/content-element-outside-shadow.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:38:  editing/pasteboard/drag-files-to-editable-element.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:42:  fast/events/drop-handler-should-not-stop-navigate.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:43:  fast/events/drop-with-file-paths.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:44:  fast/files/file-reader-directory-crash.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:119:  fast/events/show-modal-dialog-onblur-onfocus.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:120:  fast/harness/show-modal-dialog.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:286:  fast/dom/title-directionality.html is also in a Skipped file.  [test/expectations] [5]
WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist
WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist
Total errors found: 9 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 143085 [details]
Patch

Attachment 143085 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12731745

[Dave Tharp]

Created attachment 143395 [details]
Patch

[Dave Tharp]

(In reply to comment #28)
> Created an attachment (id=143395) [details]
> Patch
Re-baselined + Yet another attempt to solve the mac build issue. (I don't have a Mac to test on yet :(   )

[Dave Tharp]

Notes on Bug Fix:
- The assert was caused by vw, vh, and vmin not being handled in CSSPrimitiveValue::computeLength().
- Could have fixed the assert by stubbing the handling of vw, vh and vmin, but both Yi Shen and myself decided to go ahead and implement these units correctly in CSSPrimitiveValue::computeLength().
- There is some debate on what "correctly" is in this context :)  Yi's approach bases vw, vh, and vmin on the viewport size.  My approach bases these units on the size of the containing parent. The spec ( http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths ) says : "The viewport-percentage lengths are relative to the size of the initial containing block". Also, the test that highlights this issue ( http://samples.msdn.microsoft.com/ietestcenter/css3/css_harness.htm?type=values&url=units-000 ) appears to be  written to expect the vw, vh and vmin uints to be relative to the parent.
- I ended up having to push parentStyle down into CSSPrimitiveValue::computeLength() (added a method argument). This ended up blooming out and touching more code than I would have hoped for.  If there is a better way to get parentStyle in CSSPrimitiveValue::computeLength(), I am listening :)

[Jessie Berlin]

(In reply to comment #30)
> Notes on Bug Fix:
> - The assert was caused by vw, vh, and vmin not being handled in CSSPrimitiveValue::computeLength().
> - Could have fixed the assert by stubbing the handling of vw, vh and vmin, but both Yi Shen and myself decided to go ahead and implement these units correctly in CSSPrimitiveValue::computeLength().
> - There is some debate on what "correctly" is in this context :)  Yi's approach bases vw, vh, and vmin on the viewport size.  My approach bases these units on the size of the containing parent. The spec ( http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths ) says : "The viewport-percentage lengths are relative to the size of the initial containing block". Also, the test that highlights this issue ( http://samples.msdn.microsoft.com/ietestcenter/css3/css_harness.htm?type=values&url=units-000 ) appears to be  written to expect the vw, vh and vmin uints to be relative to the parent.
> - I ended up having to push parentStyle down into CSSPrimitiveValue::computeLength() (added a method argument). This ended up blooming out and touching more code than I would have hoped for.  If there is a better way to get parentStyle in CSSPrimitiveValue::computeLength(), I am listening :)

Note: this test also asserts on Windows. I have added it to the Skipped list for now (see http://trac.webkit.org/changeset/118458), but please take it off the Skipped list when you fix this bug.

[Zan Dobersek]

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

[Beth Dakin]

Here are a few thoughts.

It is a bit of a shame to pass around that extra parentStyle pointer so much. But I suppose there is already a lot of that. We could consider passing an IntSize instead to represent the parentSize. I'm not totally sure that's worth it though because then the call-sites will be messier, and we'll be incurring the overhead of creating that object in a bunch of cases where it's not needed.

I do have a higher-level question though. I'm still a little confused reading through this bug about why we're adding support for these units. Does supporting them fix the assertion? Do we want to support them because we think they are valuable? Or supported in other browsers? Just generally wondering how to assess the value of something that also creates some overhead with all the RenderStyle passing.

Antti would be a good person to weigh in here, too.

[Dave Tharp]

(In reply to comment #33)
> Here are a few thoughts.
> 
> It is a bit of a shame to pass around that extra parentStyle pointer so much. But I suppose there is already a lot of that. We could consider passing an IntSize instead to represent the parentSize. I'm not totally sure that's worth it though because then the call-sites will be messier, and we'll be incurring the overhead of creating that object in a bunch of cases where it's not needed.
> 
> I do have a higher-level question though. I'm still a little confused reading through this bug about why we're adding support for these units. Does supporting them fix the assertion? Do we want to support them because we think they are valuable? Or supported in other browsers? Just generally wondering how to assess the value of something that also creates some overhead with all the RenderStyle passing.
> 
> Antti would be a good person to weigh in here, too.

It appears that the only other browser that supports these units is IE10. The reason I decided to attempt an implementation is that running this test : http://samples.msdn.microsoft.com/ietestcenter/css3/valuesandunits/units-000.htm causes an assertion in debug builds. In general I hate when html causes code to break :)  Yes, I could have just worked around the actual assertion, but the test itself would remain in failure state, so implementing the units seemed to be the right thing to do. So do directly answer your question: Yes, adding support for these units does fix the assertion.  But, I could fix the assertion by simply stubbing the case for the units out... but that seemed like a weak fix (yi shen took the same approach, but ended up with a different solution)
I agree that the overall value of adding support for these units is debatable, given the amount of code I had to touch.  I don't think the use of them in the wild is high. However, they are useful in my opinion.  I guess that value judgement is up to you reviewers :)
Regarding Antii, I did try to get him to look at this when I first posted it. His response was something like "this is a HUGE patch. I'm not going to look at it now".  I didn't want to ask him if that meant he queued it or he was just punting it...  so I waited a few days to see if he picked it up. So after a few days I looked for another reviewer :)

[Zan Dobersek]

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

[Beth Dakin]

Comment on attachment 143395 [details]
Patch

Seems like these units would be nice to have, so I am going to go ahead and r+. The added complexity is not ideal, but there is no obvious way around that.

[WebKit Review Bot]

Comment on attachment 143395 [details]
Patch

Rejecting attachment 143395 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
fl/TestExpectations.rej
patching file LayoutTests/platform/mac/Skipped
Hunk #1 FAILED at 834.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/Skipped.rej
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 2428.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Beth Dakin']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13094201

[Tom Zakrajsek]

Committed r121285: <http://trac.webkit.org/changeset/121285>

[Darin Adler]

Comment on attachment 143395 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        lengths are "relative to the size of the initial containing block", which I read to be
> +        the size of the parent element. Since parentStyle was not available in computeLengthDouble,

But “size of the initial containing block” is not the same thing as “size of the parent element”. The definition of initial containing block is here <http://www.w3.org/TR/CSS21/visudet.html#containing-block-details>. It’s the “size of the containing block in which the root element lives”.

[Dave Tharp]

(In reply to comment #39)
> (From update of attachment 143395 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143395&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        lengths are "relative to the size of the initial containing block", which I read to be
> > +        the size of the parent element. Since parentStyle was not available in computeLengthDouble,
> 
> But “size of the initial containing block” is not the same thing as “size of the parent element”. The definition of initial containing block is here <http://www.w3.org/TR/CSS21/visudet.html#containing-block-details>. It’s the “size of the containing block in which the root element lives”.

Ah.  Ok, rolling the patch out and will re-visit the implementation. Thanks!

[WebKit Review Bot]

Re-opened since this is blocked by 90014

[Csaba Osztrogonác]

Comment on attachment 143395 [details]
Patch

It is obsoloete, because it was rolled out by http://trac.webkit.org/changeset/121289

[Csaba Osztrogonác]

The bug is still valid. Is anybody working on it?

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Appears to have been fixed in bug 109229.