WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
16708
CSS: Slow parsing of rgb() with percent values
https://bugs.webkit.org/show_bug.cgi?id=16708
Summary
CSS: Slow parsing of rgb() with percent values
Eric Seidel (no email)
Reported
2008-01-02 05:52:18 PST
Safari renders 3d canvas demo too slowly
http://canvex.lazyilluminati.com/misc/3d.html
Shipping 3.0.4 says 4.2 fps on my machine. We're probably much better in TOT, as I expect the slowdown here is JS. A resolution to this bug would be to make sure that this is already covered by sunspider.
Attachments
proposed patch
(9.33 KB, patch)
2011-03-11 06:20 PST
,
Andras Becsi
kling
: review-
Details
Formatted Diff
Diff
proposed patch
(10.09 KB, patch)
2011-03-12 09:38 PST
,
Andras Becsi
no flags
Details
Formatted Diff
Diff
proposed patch
(10.09 KB, patch)
2011-03-12 09:55 PST
,
Andras Becsi
no flags
Details
Formatted Diff
Diff
updated patch
(10.16 KB, patch)
2011-03-16 06:44 PDT
,
Andras Becsi
no flags
Details
Formatted Diff
Diff
updated patch
(9.82 KB, patch)
2011-03-18 09:47 PDT
,
Andras Becsi
darin
: review-
Details
Formatted Diff
Diff
proposed patch v2
(10.93 KB, patch)
2011-03-21 09:15 PDT
,
Andras Becsi
no flags
Details
Formatted Diff
Diff
proposed patch v2
(10.91 KB, patch)
2011-03-21 09:32 PDT
,
Andras Becsi
no flags
Details
Formatted Diff
Diff
proposed patch v2 with layout test
(10.92 KB, patch)
2011-03-21 09:44 PDT
,
Andras Becsi
no flags
Details
Formatted Diff
Diff
proposed patch v3
(13.20 KB, patch)
2011-03-22 11:38 PDT
,
Andras Becsi
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2008-01-02 06:13:00 PST
I only see 7.6fps on TOT.
Eric Seidel (no email)
Comment 2
2008-01-02 07:02:51 PST
I took a shark sample (which I can send to anyone who cares). High level: 13.5% 13.5% WebCore WebCore::CanvasRenderingContext2D::fill() 5.9% 5.9% JavaScriptCore KJS::BracketAccessorNode::evaluateToNumber(KJS::ExecState*) 5.6% 5.6% WebCore WebCore::ScrollView::updateContents(WebCore::IntRect const&, bool) 3.2% 3.2% JavaScriptCore kjs_dtoa 2.9% 2.9% JavaScriptCore WTF::fastMalloc(unsigned long) 2.5% 2.5% JavaScriptCore KJS::ArrayInstance::getOwnPropertySlot(KJS::ExecState*, unsigned, KJS::PropertySlot&) 2.4% 2.4% JavaScriptCore kjs_strtod 2.4% 2.4% WebCore WebCore::CanvasStyle::applyFillColor(WebCore::GraphicsContext*) 2.2% 2.2% JavaScriptCore WTF::fastFree(void*) 2.0% 2.0% WebCore cssyyparse(void*) 1.9% 1.9% WebCore WebCore::CSSParser::lex() 1.9% 1.9% JavaScriptCore KJS::AddNode::evaluate(KJS::ExecState*) 1.9% 1.9% JavaScriptCore quorem 1.6% 1.6% JavaScriptCore diff 1.6% 1.6% JavaScriptCore KJS::ActivationImp::~ActivationImp [in-charge]() 1.6% 1.6% JavaScriptCore KJS::ArrayInstance::mark() 1.5% 1.5% JavaScriptCore void* KJS::Collector::heapAllocate<(KJS::Collector::HeapType)1>(unsigned long) 1.4% 1.4% JavaScriptCore KJS::LocalVarAccessNode::evaluate(KJS::ExecState*) 1.3% 1.3% JavaScriptCore KJS::MultNode::evaluateToNumber(KJS::ExecState*) 1.3% 1.3% JavaScriptCore KJS::FunctionBodyNode::execute(KJS::ExecState*) 1.2% 1.2% JavaScriptCore unsigned long KJS::Collector::sweep<(KJS::Collector::HeapType)1>(bool) 1.1% 1.1% WebCore WebCore::HTMLCanvasElement::paint(WebCore::GraphicsContext*, WebCore::IntRect const&) 1.0% 1.0% WebCore WebCore::ScrollView::visibleContentRect() const 1.0% 1.0% JavaScriptCore KJS::NumberImp::toNumber(KJS::ExecState*) const 0.9% 0.9% WebCore WebCore::CSSParser::text(int*) 0.9% 0.9% JavaScriptCore KJS::AssignBracketNode::evaluate(KJS::ExecState*) 0.9% 0.9% JavaScriptCore void* KJS::Collector::heapAllocate<(KJS::Collector::HeapType)0>(unsigned long) 0.9% 0.9% JavaScriptCore KJS::ImmediateNumberNode::evaluate(KJS::ExecState*) 0.8% 0.8% JavaScriptCore KJS::BracketAccessorNode::evaluate(KJS::ExecState*) 0.8% 0.8% WebCore WebCore::Path::clear() 0.8% 0.8% JavaScriptCore WTF::fastRealloc(void*, unsigned long) 0.7% 0.7% JavaScriptCore KJS::ArrayInstance::put(KJS::ExecState*, unsigned, KJS::JSValue*, int) 0.7% 0.7% WebKit -[WebHTMLView visibleRect] 0.7% 0.7% JavaScriptCore unsigned long KJS::Collector::sweep<(KJS::Collector::HeapType)0>(bool) 0.7% 0.7% JavaScriptCore KJS::JSObject::toObject(KJS::ExecState*) const 0.7% 0.7% JavaScriptCore WTF::TCMalloc_Central_FreeList::RemoveRange(void**, void**, int*) 0.7% 0.7% WebCore WebCore::ScrollView::getDocumentView() const 0.7% 0.7% JavaScriptCore KJS::AddNumbersNode::evaluate(KJS::ExecState*) 0.7% 0.7% WebCore WebCore::DeprecatedStringData::makeAscii() 0.6% 0.6% JavaScriptCore KJS::jsNumberCell(double) 0.6% 0.6% JavaScriptCore KJS::ExecState::ExecState[not-in-charge](KJS::JSGlobalObject*, KJS::JSObject*, KJS::FunctionBodyNode*, KJS::ExecState*, KJS::FunctionImp*, KJS::List const&) 0.6% 0.6% JavaScriptCore KJS::UString::UString[not-in-charge](KJS::UString const&, KJS::UString const&) 0.5% 0.5% JavaScriptCore KJS::FunctionCallResolveNode::evaluate(KJS::ExecState*) 0.5% 0.5% JavaScriptCore KJS::AddNumbersNode::evaluateToNumber(KJS::ExecState*) 0.5% 0.5% WebCore WebCore::CSSParser::setupParser(char const*, WebCore::String const&, char const*) 0.5% 0.5% JavaScriptCore KJS::ExprStatementNode::execute(KJS::ExecState*) 0.5% 0.5% JavaScriptCore KJS::Lookup::findEntry(KJS::HashTable const*, KJS::Identifier const&) 0.5% 0.5% JavaScriptCore KJS::FunctionCallDotNode::evaluate(KJS::ExecState*) 0.5% 0.5% JavaScriptCore KJS::MultNode::evaluate(KJS::ExecState*) 0.5% 0.5% JavaScriptCore KJS::PropertyMap::mark() const 0.4% 0.4% JavaScriptCore KJS::FunctionImp::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.4% 0.4% JavaScriptCore KJS::DivNode::evaluate(KJS::ExecState*) 0.4% 0.4% JavaScriptCore KJS::ArgumentListNode::evaluateList(KJS::ExecState*, KJS::List&) 0.4% 0.4% WebCore WebCore::StringImpl::lower() const 0.4% 0.4% JavaScriptCore KJS::LocalVarAccessNode::evaluateToNumber(KJS::ExecState*) 0.4% 0.4% JavaScriptCore KJS::BlockNode::execute(KJS::ExecState*) 0.4% 0.4% JavaScriptCore KJS::AddNode::evaluateToNumber(KJS::ExecState*) 0.4% 0.4% JavaScriptCore pow5mult 0.3% 0.3% JavaScriptCore WTF::TCMalloc_Central_FreeList::FetchFromSpansSafe() 0.3% 0.3% WebCore WebCore::CSSParser::lex(void*) 0.3% 0.3% JavaScriptCore KJS::StringImp::~StringImp [in-charge]() 0.3% 0.3% JavaScriptCore KJS::AssignLocalVarNode::evaluate(KJS::ExecState*) 0.3% 0.3% JavaScriptCore KJS::VarStatementNode::execute(KJS::ExecState*) 0.3% 0.3% WebCore WebCore::GraphicsContext::fillRect(WebCore::IntRect const&, WebCore::Color const&) 0.3% 0.3% JavaScriptCore WTF::TCMalloc_Central_FreeList::ReleaseListToSpans(void*) 0.3% 0.3% WebCore WebCore::CanvasRenderingContext2D::fillRect(float, float, float, float, int&) 0.3% 0.3% JavaScriptCore KJS::SubNode::evaluate(KJS::ExecState*) 0.3% 0.3% JavaScriptCore KJS::ActivationImp::getOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.3% 0.3% WebCore WebCore::RenderView::repaintViewRectangle(WebCore::IntRect const&, bool) 0.3% 0.3% JavaScriptCore d2b 0.3% 0.3% JavaScriptCore KJS::JSValue::toFloat(KJS::ExecState*) const 0.3% 0.3% JavaScriptCore KJS::ElementNode::evaluate(KJS::ExecState*) 0.3% 0.3% JavaScriptCore KJS::compareWithCompareFunctionForQSort(void const*, void const*) 0.2% 0.2% WebCore WebCore::String::String[not-in-charge](KJS::UString const&) 0.2% 0.2% JavaScriptCore KJS::ForNode::execute(KJS::ExecState*) 0.2% 0.2% WebCore void WTF::deleteAllValues<WebCore::ValueList*, WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const>(WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const&) 0.2% 0.2% JavaScriptCore KJS::PreIncLocalVarNode::evaluate(KJS::ExecState*) 0.2% 0.2% JavaScriptCore KJS::ArrayInstance::sort(KJS::ExecState*, KJS::JSObject*) 0.2% 0.2% JavaScriptCore KJS::UString::UString[not-in-charge](char const*) 0.2% 0.2% JavaScriptCore KJS::UString::from(double) 0.2% 0.2% JavaScriptCore KJS::NumberImp::toString(KJS::ExecState*) const 0.2% 0.2% JavaScriptCore KJS::ArrayInstance::~ArrayInstance [not-in-charge]() 0.2% 0.2% WebCore void WTF::deleteAllValues<WebCore::Function*, WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const>(WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const&) 0.2% 0.2% JavaScriptCore KJS::Collector::markStackObjectsConservatively(void*, void*) 0.1% 0.1% WebCore WebCore::RenderBox::absoluteClippedOverflowRect() 0.1% 0.1% JavaScriptCore KJS::PropertyMap::get(KJS::Identifier const&) const 0.1% 0.1% JavaScriptCore KJS::NumberNode::evaluateToNumber(KJS::ExecState*) 0.1% 0.1% WebCore WebCore::RenderBox::computeAbsoluteRepaintRect(WebCore::IntRect&, bool) 0.1% 0.1% JavaScriptCore KJS::NumberImp::type() const 0.1% 0.1% JavaScriptCore KJS::JSCell::mark() 0.1% 0.1% WebCore WebCore::JSDOMWindow::customGetOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.1% 0.1% WebCore WebCore::CSSParser::parseColorFromValue(WebCore::Value*, unsigned&, bool) 0.1% 0.1% WebCore std::pair<WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, bool> WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::add<WebCore::ValueList*, WebCore::ValueList*, WTF::HashSetTranslator<(bool)1, WebCore::ValueList*, WTF::HashTraits<WebCore::ValueList*>, WTF::HashTraits<int>, WTF::PtrHash<WebCore::ValueList*> > >(WebCore::ValueList* const&, WebCore::ValueList* const&) 0.1% 0.1% JavaScriptCore WTF::HashMap<WTF::RefPtr<KJS::UString::Rep>, unsigned long, KJS::IdentifierRepHash, KJS::IdentifierRepHashTraits, KJS::SymbolTableIndexHashTraits>::get(KJS::UString::Rep*) const 0.1% 0.1% WebCore WebCore::JSDOMWindow::getOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.1% 0.1% WebCore WebCore::JSCanvasRenderingContext2DPrototypeFunctionFill::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.1% 0.1% WebCore findProp(char const*, unsigned) 0.1% 0.1% WebCore WebCore::ValueList::~ValueList [not-in-charge]() 0.1% 0.1% JavaScriptCore KJS::ReturnNode::execute(KJS::ExecState*) 0.1% 0.1% JavaScriptCore KJS::PropertyMap::~PropertyMap [not-in-charge]() 0.1% 0.1% WebCore WebCore::RenderObject::repaint(bool) 0.1% 0.1% WebCore WebCore::DeprecatedString::isAllASCII() const 0.1% 0.1% JavaScriptCore KJS::StringImp::toString(KJS::ExecState*) const 0.1% 0.1% WebCore WebCore::CSSParser::parseColorParameters(WebCore::Value*, int*, bool) 0.1% 0.1% JavaScriptCore KJS::LessEqNode::evaluateToBoolean(KJS::ExecState*) 0.1% 0.1% WebCore WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::find<int, WTF::IdentityHashTranslator<int, int, WTF::IntHash<int> > >(int const&) 0.1% 0.1% WebCore WebCore::toHTMLCanvasStyle(KJS::ExecState*, KJS::JSValue*) 0.1% 0.1% WebCore WebCore::DeprecatedString::~DeprecatedString [not-in-charge]() 0.1% 0.1% WebCore WebCore::CSSParser::~CSSParser [not-in-charge]() 0.1% 0.1% JavaScriptCore KJS::JSObject::type() const 0.1% 0.1% JavaScriptCore WTF::fastZeroedMalloc(unsigned long) 0.1% 0.1% WebCore WebCore::JSCanvasRenderingContext2D::putValueProperty(KJS::ExecState*, int, KJS::JSValue*, int) 0.1% 0.1% WebCore WebCore::Frame::settings() const 0.1% 0.1% WebCore WebCore::CanvasRenderingContext2D::setFillStyle(WTF::PassRefPtr<WebCore::CanvasStyle>) 0.1% 0.1% JavaScriptCore KJS::AssignDotNode::evaluate(KJS::ExecState*) 0.1% 0.1% WebCore WebCore::Widget::getView() const 0.1% 0.1% WebCore WebCore::CSSParser::parseValue(int, bool) 0.1% 0.1% WebCore KJS::Window::allowsAccessFrom(KJS::JSGlobalObject const*) const 0.1% 0.1% JavaScriptCore KJS::ExecState::lexicalGlobalObject() const 0.1% 0.1% WebCore WebCore::RenderView::printing() const 0.1% 0.1% WebCore WebCore::RenderObject::container() const 0.1% 0.1% JavaScriptCore KJS::PropertyMap::getLocation(KJS::Identifier const&) 0.1% 0.1% JavaScriptCore KJS::jsString(KJS::UString const&) 0.1% 0.1% JavaScriptCore KJS::JSObject::mark() 0.1% 0.1% JavaScriptCore KJS::ArrayInstance::getOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.1% 0.1% WebCore WebCore::JSCanvasRenderingContext2D::getOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.1% 0.1% JavaScriptCore KJS::Collector::markProtectedObjects() 0.1% 0.1% WebCore WebCore::Document::settings() const 0.1% 0.1% WebCore WebCore::CSSParser::parseColor(WebCore::String const&, unsigned&, bool) 0.1% 0.1% JavaScriptCore KJS::MathProtoFuncSqrt::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.1% 0.1% JavaScriptCore KJS::LessNode::evaluateToBoolean(KJS::ExecState*) 0.1% 0.1% JavaScriptCore KJS::JSGlobalObject::getOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.1% 0.1% JavaScriptCore KJS::FunctionCallDotNode::evaluateToNumber(KJS::ExecState*) 0.1% 0.1% WebCore WebCore::JSCanvasRenderingContext2DPrototypeFunctionBeginPath::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.1% 0.1% WebCore WebCore::JSCanvasRenderingContext2DPrototype::getOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.1% 0.1% WebCore WebCore::DeprecatedString::toDouble(bool*) const 0.1% 0.1% WebCore WebCore::DeprecatedString::DeprecatedString[not-in-charge](WebCore::DeprecatedChar const*, unsigned) 0.1% 0.1% WebCore WebCore::Color::setNamedColor(WebCore::String const&) 0.1% 0.1% JavaScriptCore KJS::DotAccessorNode::evaluate(KJS::ExecState*) 0.1% 0.1% JavaScriptCore WTF::TCMalloc_Central_FreeList::InsertRange(void*, void*, int) 0.1% 0.1% WebCore WebCore::String::length() const 0.1% 0.1% WebCore WebCore::Path::addLineTo(WebCore::FloatPoint const&) 0.1% 0.1% WebCore WebCore::CSSParser::CSSParser[not-in-charge](bool) 0.1% 0.1% WebCore std::pair<WTF::HashTableIterator<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >, bool> WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::add<WebCore::Function*, WebCore::Function*, WTF::HashSetTranslator<(bool)1, WebCore::Function*, WTF::HashTraits<WebCore::Function*>, WTF::HashTraits<int>, WTF::PtrHash<WebCore::Function*> > >(WebCore::Function* const&, WebCore::Function* const&) 0.1% 0.1% WebCore KJS::staticFunctionGetter(KJS::ExecState*, KJS::JSObject*, KJS::Identifier const&, KJS::PropertySlot const&) 0.1% 0.1% JavaScriptCore KJS::jsOwnedString(KJS::UString const&) 0.1% 0.1% JavaScriptCore KJS::FunctionImp::mark() 0.1% 0.1% JavaScriptCore KJS::ArrayNode::evaluate(KJS::ExecState*) 0.1% 0.1% JavaScriptCore KJS::ArrayInstance::ArrayInstance[not-in-charge](KJS::JSObject*, KJS::List const&) 0.1% 0.1% WebCore WebCore::String::deprecatedString() const 0.1% 0.1% WebCore WebCore::ParseString::lower() 0.1% 0.1% WebCore WebCore::CSSPrimitiveValue::~CSSPrimitiveValue [in-charge deleting]() 0.1% 0.1% JavaScriptCore KJS::AddStringsNode::evaluate(KJS::ExecState*) 0.1% 0.1% WebKit -[WebHTMLView drawSingleRect:] 0.1% 0.1% WebCore WebCore::String::lower() const 0.1% 0.1% WebCore WebCore::setSharedTimerFireTime(double) 0.1% 0.1% WebCore WebCore::RenderReplaced::overflowRect(bool) const 0.1% 0.1% WebCore WebCore::DeprecatedStringData::DeprecatedStringData[not-in-charge]() 0.1% 0.1% WebCore WebCore::CSSParser::validUnit(WebCore::Value*, WebCore::CSSParser::Units, bool) 0.1% 0.1% WebCore WebCore::CSSParser::parseColor(unsigned&, WebCore::String const&, bool) 0.1% 0.1% WebCore WebCore::CanvasRenderingContext2D::clearPathForDashboardBackwardCompatibilityMode() 0.1% 0.1% JavaScriptCore KJS::ArrayObjectImp::construct(KJS::ExecState*, KJS::List const&) 0.1% 0.1% JavaScriptCore KJS::ActivationImp::mark() 0.1% 0.1% JavaScriptCore Balloc 0.0% 0.0% WebCore WTF::Vector<WebCore::Value, (unsigned long)16>::shrink(unsigned long) 0.0% 0.0% WebCore WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::expand() 0.0% 0.0% WebCore WebCore::JSCanvasRenderingContext2D::put(KJS::ExecState*, KJS::Identifier const&, KJS::JSValue*, int) 0.0% 0.0% WebCore WebCore::JSCanvasRenderingContext2D::classInfo() const 0.0% 0.0% WebCore WebCore::HTMLCanvasElement::drawingContext() const 0.0% 0.0% WebCore WebCore::DeprecatedStringData::initialize(WebCore::DeprecatedChar const*, unsigned) 0.0% 0.0% WebCore WebCore::DeprecatedString::makeSharedNullHandle() 0.0% 0.0% JavaScriptCore KJS::InternalFunctionImp::implementsCall() const 0.0% 0.0% WebCore WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::rehash(int) 0.0% 0.0% WebCore WebCore::StyleBase::stylesheet() 0.0% 0.0% WebCore WebCore::String::String[in-charge](unsigned short const*, unsigned) 0.0% 0.0% WebCore WebCore::RenderObject::view() const 0.0% 0.0% WebCore WebCore::RenderBlock::isBlockFlow() const 0.0% 0.0% WebCore WebCore::JSCanvasRenderingContext2DPrototypeFunctionMoveTo::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.0% 0.0% WebCore WebCore::JSCanvasRenderingContext2DPrototypeFunctionLineTo::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.0% 0.0% WebCore WebCore::DeprecatedStringData::DeprecatedStringData[in-charge]() 0.0% 0.0% WebCore WebCore::CSSParser::sinkFloatingValueList(WebCore::ValueList*) 0.0% 0.0% WebCore WebCore::CSSParser::parseColor(WebCore::CSSMutableStyleDeclaration*, WebCore::String const&) 0.0% 0.0% JavaScriptCore KJS::StringImp::type() const 0.0% 0.0% JavaScriptCore KJS::ReadModifyLocalVarNode::evaluate(KJS::ExecState*) 0.0% 0.0% JavaScriptCore KJS::AddStringLeftNode::evaluate(KJS::ExecState*) 0.0% 0.0% WebKit -[WebHTMLView(WebPrivate) _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] 0.0% 0.0% WebCore WebCore::StringImpl::~StringImpl [not-in-charge]() 0.0% 0.0% WebCore WebCore::StringImpl::StringImpl[in-charge](unsigned short const*, unsigned) 0.0% 0.0% WebCore WebCore::RenderBox::borderBox() const 0.0% 0.0% WebCore WebCore::Frame::ownerElement() const 0.0% 0.0% WebCore WebCore::DeprecatedString::toFloat(bool*) const 0.0% 0.0% WebCore WebCore::CSSStyleDeclaration::CSSStyleDeclaration[not-in-charge](WebCore::CSSRule*) 0.0% 0.0% WebCore WebCore::CSSParser::parseColor(WebCore::Value*) 0.0% 0.0% WebCore WebCore::CanvasRenderingContext2D::lineTo(float, float) 0.0% 0.0% JavaScriptCore KJS::ArrayInstance::~ArrayInstance [in-charge]() 0.0% 0.0% WebCore WebCore::Document::ownerElement() const 0.0% 0.0% WebCore WebCore::CSSPrimitiveValue::cleanup() 0.0% 0.0% WebCore WebCore::CSSParser::createFloatingValueList() 0.0% 0.0% WebCore KJS::JSGlobalObject::isGlobalObject() const 0.0% 0.0% JavaScriptCore KJS::ArrayInstance::lengthGetter(KJS::ExecState*, KJS::JSObject*, KJS::Identifier const&, KJS::PropertySlot const&) 0.0% 0.0% WebCore WebCore::String::String[not-in-charge](unsigned short const*, unsigned) 0.0% 0.0% WebCore WebCore::JSDOMWindow::impl() const 0.0% 0.0% WebCore WebCore::equal(WebCore::StringImpl const*, char const*) 0.0% 0.0% WebCore WebCore::DeprecatedString::DeprecatedString[not-in-charge]() 0.0% 0.0% WebCore WebCore::CSSPrimitiveValue::cssValueType() const 0.0% 0.0% WebCore WebCore::CSSMutableStyleDeclaration::~CSSMutableStyleDeclaration [in-charge deleting]() 0.0% 0.0% WebCore WebCore::CanvasRenderingContext2D::drawingContext() const 0.0% 0.0% WebCore KJS::ScriptInterpreter::markDOMNodesForDocument(WebCore::Document*) 0.0% 0.0% JavaScriptCore KJS::NumberNode::evaluate(KJS::ExecState*) 0.0% 0.0% WebCore WTF::HashSet<WebCore::ValueList*, WTF::PtrHash<WebCore::ValueList*>, WTF::HashTraits<WebCore::ValueList*> >::add(WebCore::ValueList* const&) 0.0% 0.0% WebCore WebCore::HTMLCanvasElement::willDraw(WebCore::FloatRect const&) 0.0% 0.0% WebCore WebCore::GraphicsContext::platformContext() const 0.0% 0.0% WebCore WebCore::FrameView::repaintRectangle(WebCore::IntRect const&, bool) 0.0% 0.0% WebCore WebCore::deprecatedString(WebCore::ParseString const&) 0.0% 0.0% WebCore WebCore::CSSParser::clearProperties() 0.0% 0.0% WebCore WebCore::CSSParser::addProperty(int, WTF::PassRefPtr<WebCore::CSSValue>, bool) 0.0% 0.0% JavaScriptCore KJS::JSVariableObject::mark() 0.0% 0.0% WebCore WTF::Vector<WTF::RefPtr<WebCore::CSSRuleList>, (unsigned long)0>::shrink(unsigned long) 0.0% 0.0% WebCore WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::deallocateTable(int*, int) 0.0% 0.0% WebCore WebCore::StringImpl::init(unsigned short const*, unsigned) 0.0% 0.0% WebCore WebCore::RenderBox::height() const 0.0% 0.0% WebCore WebCore::FloatSize::FloatSize[not-in-charge](_NSSize const&) 0.0% 0.0% WebCore WebCore::DeprecatedValueListImpl::DeprecatedValueListImpl[not-in-charge](void (*)(WebCore::DeprecatedValueListImplNode*), WebCore::DeprecatedValueListImplNode* (*)(WebCore::DeprecatedValueListImplNode*)) 0.0% 0.0% WebCore WebCore::DeprecatedValueListImpl::DeprecatedValueListImpl[in-charge](void (*)(WebCore::DeprecatedValueListImplNode*), WebCore::DeprecatedValueListImplNode* (*)(WebCore::DeprecatedValueListImplNode*)) 0.0% 0.0% WebCore WebCore::DeprecatedString::DeprecatedString[in-charge](WebCore::DeprecatedChar const*, unsigned) 0.0% 0.0% WebCore WebCore::CSSParser::~CSSParser [in-charge]() 0.0% 0.0% WebCore WebCore::CSSMutableStyleDeclaration::CSSMutableStyleDeclaration[not-in-charge]() 0.0% 0.0% WebCore WebCore::CSSMutableStyleDeclaration::CSSMutableStyleDeclaration[in-charge]() 0.0% 0.0% WebCore WebCore::CanvasStyle::CanvasStyle[not-in-charge](WebCore::String const&) 0.0% 0.0% JavaScriptCore KJS::ResolveNode::evaluate(KJS::ExecState*) 0.0% 0.0% JavaScriptCore KJS::JSGlobalObject::mark() 0.0% 0.0% WebKit -[WebHTMLView isFlipped] 0.0% 0.0% WebCore WTF::Vector<WTF::RefPtr<WebCore::StyleBase>, (unsigned long)0>::shrink(unsigned long) 0.0% 0.0% JavaScriptCore WTF::TCMalloc_Central_FreeList::ShrinkCache(int, bool) 0.0% 0.0% WebCore WTF::HashSet<WebCore::Function*, WTF::PtrHash<WebCore::Function*>, WTF::HashTraits<WebCore::Function*> >::add(WebCore::Function* const&) 0.0% 0.0% WebCore WebCore::StyleBase::isStyleSheet() const 0.0% 0.0% WebCore WebCore::stopSharedTimer() 0.0% 0.0% WebCore WebCore::RenderObject::borderTopExtra() const 0.0% 0.0% WebCore WebCore::RenderFlow::hasColumns() const 0.0% 0.0% WebCore WebCore::IntRect::operator _NSRect() const 0.0% 0.0% WebCore WebCore::freeHandle(WebCore::DeprecatedStringData**) 0.0% 0.0% WebCore WebCore::FloatRect::FloatRect[not-in-charge](_NSRect const&) 0.0% 0.0% WebCore WebCore::FloatRect::FloatRect[in-charge](_NSRect const&) 0.0% 0.0% WebCore WebCore::DeprecatedStringData::~DeprecatedStringData [not-in-charge]() 0.0% 0.0% WebCore WebCore::DeprecatedStringData::~DeprecatedStringData [in-charge]() 0.0% 0.0% WebCore WebCore::CSSParser::sinkFloatingValue(WebCore::Value&) 0.0% 0.0% WebCore WebCore::CSSParser::createFloatingFunction() 0.0% 0.0% WebCore WebCore::CSSParser::checkForOrphanedUnits() 0.0% 0.0% WebCore void WTF::deleteAllValues<WebCore::CSSSelector*, WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const>(WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> > const&) 0.0% 0.0% WebCore KJS::JSObject::isActivationObject() 0.0% 0.0% WebKit -[WebClipView hasAdditionalClip] 0.0% 0.0% WebCore WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::remove(int*) 0.0% 0.0% WebCore WebCore::String::String[in-charge](KJS::UString const&) 0.0% 0.0% WebCore WebCore::RenderView::computeAbsoluteRepaintRect(WebCore::IntRect&, bool) 0.0% 0.0% WebCore WebCore::RenderObject::isTable() const 0.0% 0.0% WebCore WebCore::Path::moveTo(WebCore::FloatPoint const&) 0.0% 0.0% WebCore WebCore::makeRGB(int, int, int) 0.0% 0.0% WebCore WebCore::JSCanvasRenderingContext2D::setFillStyle(KJS::ExecState*, KJS::JSValue*) 0.0% 0.0% WebCore WebCore::FloatRect::FloatRect[not-in-charge](CGRect const&) 0.0% 0.0% WebCore WebCore::DeprecatedValueListImpl::~DeprecatedValueListImpl [not-in-charge]() 0.0% 0.0% WebCore WebCore::DeprecatedValueListImpl::~DeprecatedValueListImpl [in-charge]() 0.0% 0.0% WebCore WebCore::DeprecatedString::~DeprecatedString [in-charge]() 0.0% 0.0% WebCore WebCore::CSSPrimitiveValue::CSSPrimitiveValue[in-charge](unsigned) 0.0% 0.0% WebCore WebCore::CanvasRenderingContext2D::willDraw(WebCore::FloatRect const&) 0.0% 0.0% JavaScriptCore KJS::StringNode::evaluate(KJS::ExecState*) 0.0% 0.0% WebKit -[WebHTMLView drawRect:] 0.0% 0.0% JavaScriptCore WTF::TCMalloc_PageHeap::IncrementalScavenge(unsigned long) 0.0% 0.0% WebCore WTF::HashTable<int, int, WTF::IdentityExtractor<int>, WTF::IntHash<int>, WTF::HashTraits<int>, WTF::HashTraits<int> >::allocateTable(int) 0.0% 0.0% WebCore WebCore::ScheduledAction::execute(KJS::Window*) 0.0% 0.0% WebCore WebCore::RenderView::isRenderView() const 0.0% 0.0% WebCore WebCore::RenderObject::borderBottomExtra() const 0.0% 0.0% WebCore WebCore::RenderBox::width() const 0.0% 0.0% WebCore WebCore::JSCanvasRenderingContext2DPrototypeFunctionSave::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.0% 0.0% WebCore WebCore::getPropertyID(char const*, int) 0.0% 0.0% WebCore WebCore::FloatRect::operator _NSRect() const 0.0% 0.0% WebCore WebCore::FloatRect::FloatRect[in-charge](CGRect const&) 0.0% 0.0% WebCore WebCore::FloatPoint::FloatPoint[not-in-charge](_NSPoint const&) 0.0% 0.0% WebCore WebCore::DeprecatedValueListImpl::Private::deleteList(WebCore::DeprecatedValueListImplNode*) 0.0% 0.0% WebCore WebCore::CSSPrimitiveValue::CSSPrimitiveValue[not-in-charge](unsigned) 0.0% 0.0% WebCore WebCore::CSSParser::CSSParser[in-charge](bool) 0.0% 0.0% WebCore WebCore::ContainerNode::virtualFirstChild() const 0.0% 0.0% WebCore WebCore::Color::parseHexColor(WebCore::String const&, unsigned&) 0.0% 0.0% WebCore WebCore::CanvasRenderingContext2D::moveTo(float, float) 0.0% 0.0% WebCore WebCore::CanvasRenderingContext2D::beginPath() 0.0% 0.0% JavaScriptCore KJS::NumberImp::toPrimitive(KJS::ExecState*, KJS::JSType) const 0.0% 0.0% JavaScriptCore KJS::MathObjectImp::getOwnPropertySlot(KJS::ExecState*, KJS::Identifier const&, KJS::PropertySlot&) 0.0% 0.0% JavaScriptCore KJS::ArrayInstance::compactForSorting() 0.0% 0.0% WebCore WebCore::ValueList::~ValueList [in-charge]() 0.0% 0.0% WebCore WebCore::timerFired(__CFRunLoopTimer*, void*) 0.0% 0.0% WebCore WebCore::StringImpl::~StringImpl [in-charge]() 0.0% 0.0% WebCore WebCore::StringImpl::StringImpl[not-in-charge](unsigned short const*, unsigned) 0.0% 0.0% WebCore WebCore::String::characters() const 0.0% 0.0% WebCore WebCore::setCGFillColor(CGContext*, WebCore::Color const&) 0.0% 0.0% WebCore WebCore::ScrollView::contentsHeight() const 0.0% 0.0% WebCore WebCore::RenderObject::hasControlClip() const 0.0% 0.0% WebCore WebCore::RenderFlow::paintLines(WebCore::RenderObject::PaintInfo&, int, int) 0.0% 0.0% WebCore WebCore::RenderBox::paintRootBoxDecorations(WebCore::RenderObject::PaintInfo&, int, int) 0.0% 0.0% WebCore WebCore::RenderBlock::overflowLeft(bool) const 0.0% 0.0% WebCore WebCore::RenderBlock::MarginInfo::MarginInfo[not-in-charge](WebCore::RenderBlock*, int, int) 0.0% 0.0% WebCore WebCore::Path::Path[not-in-charge](WebCore::Path const&) 0.0% 0.0% WebCore WebCore::Path::isEmpty() const 0.0% 0.0% WebCore WebCore::Node::virtualFirstChild() const 0.0% 0.0% WebCore WebCore::Node::traverseNextNode(WebCore::Node const*) const 0.0% 0.0% WebCore WebCore::JSNode::mark() 0.0% 0.0% WebCore WebCore::JSDocument::mark() 0.0% 0.0% WebCore WebCore::InlineBox::paint(WebCore::RenderObject::PaintInfo&, int, int) 0.0% 0.0% WebCore WebCore::HTMLTokenizer::write(WebCore::SegmentedString const&, bool) 0.0% 0.0% WebCore WebCore::FrameLoader::client() const 0.0% 0.0% WebCore WebCore::Frame::dragCaretController() const 0.0% 0.0% WebCore WebCore::Frame::document() const 0.0% 0.0% WebCore WebCore::Font::drawGlyphs(WebCore::GraphicsContext*, WebCore::FontData const*, WebCore::GlyphBuffer const&, int, int, WebCore::FloatPoint const&) const 0.0% 0.0% WebCore WebCore::FloatSize::FloatSize[in-charge](CGSize const&) 0.0% 0.0% WebCore WebCore::FloatSize::FloatSize[in-charge](_NSSize const&) 0.0% 0.0% WebCore WebCore::FloatPoint::FloatPoint[in-charge](CGPoint const&) 0.0% 0.0% WebCore WebCore::DeprecatedString::DeprecatedString[in-charge]() 0.0% 0.0% WebCore WebCore::CanvasStyle::CanvasStyle[in-charge](WebCore::String const&) 0.0% 0.0% JavaScriptCore KJS::NativeErrorImp::mark() 0.0% 0.0% JavaScriptCore KJS::MathProtoFuncSin::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) 0.0% 0.0% JavaScriptCore KJS::JSWrapperObject::mark() 0.0% 0.0% JavaScriptCore KJS::JSLock::registerThread() 0.0% 0.0% JavaScriptCore KJS::FunctionImp::~FunctionImp [in-charge]() 0.0% 0.0% JavaScriptCore KJS::ExecState::mark() 0.0% 0.0% JavaScriptCore KJS::Collector::collect() 0.0% 0.0% WebKit core(WebFrame*) 0.0% 0.0% WebKit -[WebHTMLView(WebPrivate) viewWillDraw] 0.0% 0.0% WebKit -[WebHTMLView(WebHTMLViewFileInternal) _isTopHTMLView] 0.0% 0.0% WebKit -[WebHTMLView respondsToSelector:] 0.0% 0.0% WebKit -[WebFrameView documentView] 0.0% 0.0% WebKit -[_WebSafeForwarder forwardInvocation:]
Oliver Hunt
Comment 3
2008-03-06 02:47:34 PST
Indeed we are slow, but, well, by my measurements ToT is faster than firefox nightlies as of a couple of days ago, and Opera weeklies...
Matthew Delaney
Comment 4
2010-06-15 15:59:11 PDT
After 2 years of inattention, we're up to 27fps on shipping Safari 5. Is that fast enough to close this bug out?
Oliver Hunt
Comment 5
2010-06-15 16:03:25 PDT
I suspect we should sample this and get an up to date idea of where perf is being consumed, on my mac book pro this only gets 15-20fps. I am sure we can do better.
Andreas Kling
Comment 6
2010-08-18 23:32:54 PDT
The bottleneck here is CSSParser::parseColor()'s handling of rgb() colors with percent values. There's no fast-path for those so it goes through cssyyparse(). Circa 30% of runtime is spent doing this.
Andras Becsi
Comment 7
2011-03-11 06:20:28 PST
Created
attachment 85468
[details]
proposed patch With QtWebKit trunk on my Core i5 Linux machine without the patch I get 23-24 fps, and 35-36 fps with it on
http://canvex.lazyilluminati.com/misc/3d.html
. In comparison on Chromium 9.0.597.107 I get 34-35 fps on Opera 11.01 I get 44-45 fps on this machine.
Andreas Kling
Comment 8
2011-03-11 08:14:38 PST
Comment on
attachment 85468
[details]
proposed patch Quoth CSS (
http://www.w3.org/TR/css3-color/#rgb-def
): "The format of an RGB value in the functional notation is ‘rgb(’ followed by a comma-separated list of three numerical values (either three integer values or three percentage values) followed by ‘)’." (And the same is basically repeated for rgba().) Thus we should not allow mixing integer and percentage component values, i.e it should be either all integers, or all percentages. The in-tree test css2.1/t040306-syntax-01-f.html covers mixing of RGB component types, and should fail with this change.
Eric Seidel (no email)
Comment 9
2011-03-11 15:49:08 PST
Attachment 85468
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8135486
WebKit Review Bot
Comment 10
2011-03-11 20:35:36 PST
Attachment 85468
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/8131530
Andras Becsi
Comment 11
2011-03-12 09:38:02 PST
Created
attachment 85596
[details]
proposed patch Do not allow mixing of percentage and integer values. Thanks Andreas for pointing this out.
Eric Seidel (no email)
Comment 12
2011-03-12 09:50:33 PST
Attachment 85596
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8132729
Andras Becsi
Comment 13
2011-03-12 09:55:33 PST
Created
attachment 85597
[details]
proposed patch Fix the Mac build.
WebKit Review Bot
Comment 14
2011-03-12 10:30:55 PST
Attachment 85596
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/8141735
Adam Barth
Comment 15
2011-03-15 02:34:41 PDT
Comment on
attachment 85597
[details]
proposed patch View in context:
https://bugs.webkit.org/attachment.cgi?id=85597&action=review
I got excited about this patch, but I'm too tired to finish reviewing it.
> Source/WebCore/css/CSSParser.cpp:3922 > + bytes[i] = string[i];
I would put a static_cast here to emphasize the change in type.
Andras Becsi
Comment 16
2011-03-16 06:44:09 PDT
Created
attachment 85925
[details]
updated patch Update patch to ToT and use static_cast for explicit type conversion in parseDoubleIfValid. Thanks Adam for taking a look at it.
Adam Barth
Comment 17
2011-03-17 11:24:35 PDT
Comment on
attachment 85925
[details]
updated patch This patch looks correct to me, but I don't really know this code that well. I'd feel better if someone who knows the CSS parser better did the final review.
Andras Becsi
Comment 18
2011-03-18 09:47:19 PDT
Created
attachment 86172
[details]
updated patch
Darin Adler
Comment 19
2011-03-18 11:04:05 PDT
Comment on
attachment 86172
[details]
updated patch View in context:
https://bugs.webkit.org/attachment.cgi?id=86172&action=review
I think we need some more test cases beyond the ones added here. I don’t think the test cases cover all the branches in the code. For example, I don’t see any test cases with multiple periods and a percentage sign or with any other illegal characters.
> Source/WebCore/css/CSSParser.cpp:3901 > +static inline int parseDoubleIfValid(const UChar* string, const UChar* end, UChar terminator, double& value)
I think it’s unclear that this function result is the number of characters consumed. There should be a comment saying so. I don’t think it makes sense to mark this function inline. It’s called in three different places and it’s pretty long too.
> Source/WebCore/css/CSSParser.cpp:3933 > + if (processedLength) { > + value = 0.0; > + bytes[processedLength] = static_cast<char>(terminator); > + bytes[processedLength + 1] = '\0'; > + char* foundTerminator; > + value = WTF::strtod(bytes.data(), &foundTerminator); > + return (*foundTerminator == static_cast<char>(terminator)) ? processedLength : 0; > + } > + return 0;
This should use early return instead of putting all the code inside an if. It’s not a good idea to cast a UChar into a char. If the terminator is guaranteed to be an ASCII character, then it should be passed in to this function as a char, not a UChar. If it’s not, then this code won’t work. Does this code really need to use WTF::strtod?
> Source/WebCore/css/CSSParser.cpp:3936 > +static inline bool parseColorIntOrPercentage(const UChar*& string, const UChar* end, UChar terminator, CSSPrimitiveValue::UnitTypes& expect, int& value)
I don’t think it makes sense to mark a function inline that’s this long and called in so many places.
> Source/WebCore/css/CSSParser.cpp:3939 > + double localValue = 0.0;
We normally just say 0, not 0.0.
> Source/WebCore/css/CSSParser.cpp:3982 > + localValue = localValue / 100.0 * 256.0;
There’s an extra space here. Multiplying by 256 is wrong. You should multiply by nextafter(256, 0).
> Source/WebCore/css/CSSParser.cpp:3984 > + if (localValue > 255) > + localValue = 255;
This probably wouldn’t be necessary if we multiplied by the right value. Unless we allow percentage values over 100.
> Source/WebCore/css/CSSParser.cpp:4034 > + double d;
Can you use a word name for this local variable rather than just "d"?
> Source/WebCore/css/CSSParser.cpp:4035 > + if (parseDoubleIfValid(string, end, terminator, d)) {
This code does significant extra work that it didn’t before, including calling strtod. That doesn’t seem good for performance.
> Source/WebCore/css/CSSParser.cpp:4062 > + double d = 0.0; > + if (parseDoubleIfValid(string, end, terminator, d)) { > + value = negative ? 0 : static_cast<int>(d * nextafter(256.0, 0.0)); > + string = end; > + return true; > + } > + return false;
Can you use a word name for this local variable rather than just "d"? Not sure why you are initializing it. This should be early return style, not “nest the success case inside an if” style.
Andras Becsi
Comment 20
2011-03-21 09:15:31 PDT
Created
attachment 86322
[details]
proposed patch v2
Andras Becsi
Comment 21
2011-03-21 09:28:35 PDT
(In reply to
comment #19
) Thanks for the comprehensive review.
> I think we need some more test cases beyond the ones added here. I don’t think the test cases cover all the branches in the code. For example, I don’t see any test cases with multiple periods and a percentage sign or with any other illegal characters.
Added some cases to test consistency in case of illegal combinations.
> > > Source/WebCore/css/CSSParser.cpp:3901 > > +static inline int parseDoubleIfValid(const UChar* string, const UChar* end, UChar terminator, double& value) > > I think it’s unclear that this function result is the number of characters consumed. There should be a comment saying so.
Added comments.
> > I don’t think it makes sense to mark this function inline. It’s called in three different places and it’s pretty long too.
Removed inline.
> It’s not a good idea to cast a UChar into a char. If the terminator is guaranteed to be an ASCII character, then it should be passed in to this function as a char, not a UChar. If it’s not, then this code won’t work. > > Does this code really need to use WTF::strtod?
I think it is much more safe to use strtod than trying to implement it here.
> > > Source/WebCore/css/CSSParser.cpp:3936 > > +static inline bool parseColorIntOrPercentage(const UChar*& string, const UChar* end, UChar terminator, CSSPrimitiveValue::UnitTypes& expect, int& value) > > I don’t think it makes sense to mark a function inline that’s this long and called in so many places. >
Removed inline.
> > Source/WebCore/css/CSSParser.cpp:3939 > > + double localValue = 0.0; > > We normally just say 0, not 0.0. > > > Source/WebCore/css/CSSParser.cpp:3982 > > + localValue = localValue / 100.0 * 256.0; > > There’s an extra space here. Multiplying by 256 is wrong. You should multiply by nextafter(256, 0).
If I use nextafter here I get wrong values because the double is casted to int at the end. (For example for 50% using nextafter gives a value of 127 instead of 128)
> > > Source/WebCore/css/CSSParser.cpp:3984 > > + if (localValue > 255) > > + localValue = 255; > > This probably wouldn’t be necessary if we multiplied by the right value. Unless we allow percentage values over 100.
We need to check the value here because the percentageg need to be clamped to the 0-100 intervall to retain consistency.
> > > Source/WebCore/css/CSSParser.cpp:4034 > > + double d; > > Can you use a word name for this local variable rather than just "d"? > > > Source/WebCore/css/CSSParser.cpp:4035 > > + if (parseDoubleIfValid(string, end, terminator, d)) { > > This code does significant extra work that it didn’t before, including calling strtod. That doesn’t seem good for performance.
I changed the code to have a separate isValidDouble function.
> > > Source/WebCore/css/CSSParser.cpp:4062 > > + double d = 0.0; > > + if (parseDoubleIfValid(string, end, terminator, d)) { > > + value = negative ? 0 : static_cast<int>(d * nextafter(256.0, 0.0)); > > + string = end; > > + return true; > > + } > > + return false; > > Can you use a word name for this local variable rather than just "d"? Not sure why you are initializing it.
I need to initialize it because on chromium it fails to build without initializing the value, because of a compiler warning.
Andras Becsi
Comment 22
2011-03-21 09:32:23 PDT
Created
attachment 86325
[details]
proposed patch v2 Using early return, in parseAlphaValue.
Andras Becsi
Comment 23
2011-03-21 09:44:48 PDT
Created
attachment 86326
[details]
proposed patch v2 with layout test Mixing of percentage and numeric values is already covered by css2.1/t040306-syntax-01-f.html, as Andreas pointed out earlier, so the added cases should explicitly test invalid combinations.
Andras Becsi
Comment 24
2011-03-22 11:38:14 PDT
Created
attachment 86486
[details]
proposed patch v3 - Avoid using strtod, and add result of new test cases to the expected file.
Andras Becsi
Comment 25
2011-03-25 07:41:14 PDT
(In reply to
comment #24
)
> Created an attachment (id=86486) [details] > proposed patch v3 > > - Avoid using strtod, and add result of new test cases to the expected file.
This patch still applies. Darin, could you take a look at this approach?
Darin Adler
Comment 26
2011-03-29 10:41:20 PDT
Comment on
attachment 86486
[details]
proposed patch v3 View in context:
https://bugs.webkit.org/attachment.cgi?id=86486&action=review
> Source/WebCore/css/CSSParser.cpp:3904 > +// Returns the number of characters which form a valid double > +// and are terminated by the given terminator character > +static int isValidDouble(const UChar* string, const UChar* end, const char terminator)
Given that this returns the number of characters, the function name should probably be something that doesn’t make it sound like it returns a boolean.
> Source/WebCore/css/CSSParser.cpp:3964 > + unsigned scale = 1; > + > + while (position < length && scale < MAX_SCALE) { > + fraction = fraction * 10 + string[position++] - '0'; > + scale *= 10; > + } > + > + value = localValue + (fraction / static_cast<double>(scale));
It seems that "scale" should be a double here instead of unsigned.
Andras Becsi
Comment 27
2011-03-29 10:44:40 PDT
Comment on
attachment 86486
[details]
proposed patch v3 Setting cq- to address Darins suggestions, landing by hand.
Jessie Berlin
Comment 28
2011-03-29 12:01:53 PDT
This change appears to have broken the Windows 7 Release Tests:
http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/10932
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r82284%20(10933)/results.html
Jessie Berlin
Comment 29
2011-03-29 12:11:58 PDT
And Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r82289%20(27421)/results.html
Darin Adler
Comment 30
2011-03-29 13:46:07 PDT
Andras, are you going to fix the failing tests?
Csaba Osztrogonác
Comment 31
2011-03-29 13:57:27 PDT
I rolled out the original patch, the unreviewed fix and the chromium expected results:
http://trac.webkit.org/changeset/82315
Andras can check this fail tomorrow.
Andras Becsi
Comment 32
2011-03-30 04:42:33 PDT
I'm investigating the roundig issue and relanding it with the fix later. Since we now calculate everything in double and cast to int at the very end, it seems that the value is rounded incorrectly. Thanks Ossy for taking care of the rollout.
Andras Becsi
Comment 33
2011-03-31 09:28:12 PDT
Re-landed in
http://trac.webkit.org/changeset/82464
. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug