Bug 16708 – CSS: Slow parsing of rgb() with percent values

• Component: CSS
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I only see 7.6fps on TOT.

[Eric Seidel (no email)]

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]

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]

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]

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]

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]

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 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)]

Attachment 85468 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8135486

[WebKit Review Bot]

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

[Andras Becsi]

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)]

Attachment 85596 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8132729

[Andras Becsi]

Created attachment 85597 [details]
proposed patch

Fix the Mac build.

[WebKit Review Bot]

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

[Adam Barth]

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]

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

Created attachment 86172 [details]
updated patch

[Darin Adler]

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]

Created attachment 86322 [details]
proposed patch v2

[Andras Becsi]

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

Created attachment 86325 [details]
proposed patch v2

Using early return, in parseAlphaValue.

[Andras Becsi]

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]

Created attachment 86486 [details]
proposed patch v3

- Avoid using strtod, and add result of new test cases to the expected file.

[Andras Becsi]

(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 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 on attachment 86486 [details]
proposed patch v3

Setting cq- to address Darins suggestions, landing by hand.

[Jessie Berlin]

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]

And Snow Leopard:

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r82289%20(27421)/results.html

[Darin Adler]

Andras, are you going to fix the failing tests?

[Csaba Osztrogonác]

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]

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]

Re-landed in http://trac.webkit.org/changeset/82464.
Closing bug.