Implementation of brand-color

Source/core/dom/Document.cpp

Index: Source/core/dom/Document.cpp
diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
index f72f2564cef33af2412c9031b9e88fcd0d4e21af..a03b7fdb617c93bd3de6f01d331e18b88cb657b0 100644
--- a/Source/core/dom/Document.cpp
+++ b/Source/core/dom/Document.cpp
@@ -51,6 +51,7 @@
 #include "core/css/StyleSheetContents.h"
 #include "core/css/StyleSheetList.h"
 #include "core/css/invalidation/StyleInvalidator.h"
+#include "core/css/parser/CSSPropertyParser.h"
 #include "core/css/resolver/FontBuilder.h"
 #include "core/css/resolver/StyleResolver.h"
 #include "core/css/resolver/StyleResolverStats.h"
@@ -4737,6 +4738,19 @@ Vector<IconURL> Document::iconURLs(int iconTypesMask)
     return iconURLs;
 }
 
+Color Document::brandColor() const
+{
+    if (!RuntimeEnabledFeatures::brandColorEnabled())
+        return Color();
+
+    for (HTMLMetaElement* metaElement = head() ? Traversal<HTMLMetaElement>::firstChild(*head()) : 0; metaElement; metaElement = Traversal<HTMLMetaElement>::nextSibling(*metaElement)) {

[esprehn, 2014/06/19 10:37:04]

You call this method from inside HTMLMetaElement::process() which already knows
which element you're looking for. Crawling the head again is just wasted work.

I don't think you want to expose a property on document like this. Instead you
should plumb the color down through the dispatchDidChangeBrandColor(color)
notification and HTMLMetaElement should contain this processing. Document should
have no knowledge of this feature.

[michaelbai, 2014/06/19 16:42:51]

In the case that there are multiple brand-color meta tags, I just want the last
one, even others are changed later.

On 2014/06/19 10:37:04, esprehn wrote:

[esprehn, 2014/06/19 18:02:20]

That's not what your code does. Every time a <meta> is inserted you notify the
embedder of it, then the embedder calls this function and it'll keep finding the
first one that was inserted, not the last one.

Note also that the parser can yield arbitrarily, so you could get one <meta>,
the embedder tells the browser process which does something with the color, then
the parser could resume and another <meta> would be inserted. Script can do that
too by just appendChild'ing meta elements inside setTimeouts.

Finally crawling the head() looking for <meta> is not correct. They can show up
anywhere in the page, and you will even notify the embedder when they do.

+        RGBA32 rgb;
+        if (equalIgnoringCase(metaElement->name(), "brand-color") && CSSPropertyParser::fastParseColor(rgb, metaElement->content().string().stripWhiteSpace(), true))

[esprehn, 2014/06/19 10:37:04]

CSSPropertyParser::fastParseColor is a private API of the parser, you shouldn't
be using it here. You end up allowing rgb() and rgba() syntax in addition to hex
and keywords, but you disallow hsl() and hsla() colors and other things we
didn't fast path. 

You want to be using BisonCSSParser::parseColor.

[michaelbai, 2014/06/19 16:42:51]

We only support color name, rgba(), rgb() or "#" syntax in brand-color.


On 2014/06/19 10:37:04, esprehn wrote:

[esprehn, 2014/06/19 18:02:21]

Why only support an arbitrary subset of the css color spec? That doesn't make
sense. It's a one line change to follow the CSS color spec here, and the
embedded doesn't have to know anything about it.

+            return Color(rgb);
+    }
+    return Color();
+}
+
 HTMLLinkElement* Document::linkManifest() const
 {
     HTMLHeadElement* head = this->head();