Implementation of brand-color

Implementation of brand-color

Add APIs
- WebFrame::brandColor() to get the brand color.
- WebFrameClinet::didChangeBrandColor to notify the brand color change.

BUG=383941
Intent to implement and ship link
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nzRY-h_-_ig

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176142

[michaelbai, 2014-06-12 23:47:58]

This is new patch solely for brand-color, in the meanwhile, I am working on
whatwg stuff.

https://codereview.chromium.org/329943004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/329943004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4743: if (equalIgnoringCase(metaElement->name(),
"brand-color") && CSSPropertyParser::fastParseColor(rgb,
metaElement->content().string(), true))
Knew I shouldn't use CSSPropertyParser::fastParseColor(), could you suggest what
function shall I use?

[abarth-chromium, 2014-06-13 05:40:42]

Thanks!  This CL is definitely on the right track.  A couple minor comments
below.  Also, please add a WebFrameTest for this feature.

https://codereview.chromium.org/329943004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/329943004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4743: if (equalIgnoringCase(metaElement->name(),
"brand-color") && CSSPropertyParser::fastParseColor(rgb,
metaElement->content().string(), true))
fastParseColor is fine.  Do you want to strip leading an trailing HTML spaces? 
Please add a test for leading and trailing spaces regardless of whether you
decide to strip them.

https://codereview.chromium.org/329943004/diff/1/Source/core/html/HTMLMetaEle...
File Source/core/html/HTMLMetaElement-in.cpp (right):

https://codereview.chromium.org/329943004/diff/1/Source/core/html/HTMLMetaEle...
Source/core/html/HTMLMetaElement-in.cpp:476: else if
(equalIgnoringCase(nameValue, "brand-color") && document().frame() &&
document().frame()->loader().client())
document().frame()->loader().client() is never zero.  Please omit the check.

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrame.h#newcod...
public/web/WebFrame.h:160: virtual WebColor brandColor() const = 0;
This is a property of the document (as you say in the comment).  It should be on
WebDocument.  (Also, please leave two blank lines between sections.)

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrameClient.h#...
public/web/WebFrameClient.h:270: 
Two blank lines between sections please.

[michaelbai, 2014-06-13 15:55:06]

Thanks for quick response, I have a qq for moving brandColor() to WebDocument,
see my inline comment.

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrame.h#newcod...
public/web/WebFrame.h:160: virtual WebColor brandColor() const = 0;
I added this method here because
- I saw the similar property like iconURL() is in WebFrame.
- Also I added didChangeBrandColor() in WebFrameClient, it is suitable to have
brandColor() in WebDocument in this case? 


On 2014/06/13 05:40:41, abarth wrote:
> This is a property of the document (as you say in the comment).  It should be
on
> WebDocument.  (Also, please leave two blank lines between sections.)

[abarth-chromium, 2014-06-13 17:12:16]

On 2014/06/13 at 15:55:06, michaelbai wrote:
> I added this method here because
> - I saw the similar property like iconURL() is in WebFrame.

Originally, we weren't all that careful about what goes on WebFrame and what
goes on WebDocument.  iconURL is old and likely should be moved to WebDocument. 
If you're interested in follow-on cleanup CLs, that might be a good candidate.

> - Also I added didChangeBrandColor() in WebFrameClient, it is suitable to have
brandColor() in WebDocument in this case? 

Yes, the notifications go out via WebFrameClient because the embedder's objects
are all associated with WebFrame (and WebView).  We can deliver notifications
about objects deeper inside Blink (like WebDocument) via WebFrameClient.

[michaelbai, 2014-06-13 19:56:51]

PTAL, here a major change

Use RuntimeEnabledFeature to guard the code, as didChangeBrandColor will pop up
to the upper layer and go through RPC finally, I think this should only enable
for required implementation.

https://codereview.chromium.org/329943004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/329943004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4743: if (equalIgnoringCase(metaElement->name(),
"brand-color") && CSSPropertyParser::fastParseColor(rgb,
metaElement->content().string(), true))
On 2014/06/13 05:40:41, abarth wrote:
> fastParseColor is fine.  Do you want to strip leading an trailing HTML spaces?

> Please add a test for leading and trailing spaces regardless of whether you
> decide to strip them.

Striped the spaces, and added test

https://codereview.chromium.org/329943004/diff/1/Source/core/html/HTMLMetaEle...
File Source/core/html/HTMLMetaElement-in.cpp (right):

https://codereview.chromium.org/329943004/diff/1/Source/core/html/HTMLMetaEle...
Source/core/html/HTMLMetaElement-in.cpp:476: else if
(equalIgnoringCase(nameValue, "brand-color") && document().frame() &&
document().frame()->loader().client())
On 2014/06/13 05:40:41, abarth wrote:
> document().frame()->loader().client() is never zero.  Please omit the check.

Done.

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/329943004/diff/1/public/web/WebFrameClient.h#...
public/web/WebFrameClient.h:270: 
On 2014/06/13 05:40:41, abarth wrote:
> Two blank lines between sections please.

Done.

[abarth-chromium, 2014-06-13 22:06:58]

https://codereview.chromium.org/329943004/diff/20001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/329943004/diff/20001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:32: // This is enabled by Android
only.
We can enable the feature on every platform.  The other platforms will just
ignore the notification.

https://codereview.chromium.org/329943004/diff/20001/public/web/WebDocument.h
File public/web/WebDocument.h (right):

https://codereview.chromium.org/329943004/diff/20001/public/web/WebDocument.h...
public/web/WebDocument.h:85: // The brand color (if any) specified by the
document loaded in this frame.
This comment is now incorrect ("the document loaded in this frame" doesn't make
sense because this API hangs off WebDocument itself).  Let's just remove it.

[abarth-chromium, 2014-06-13 22:07:55]

https://codereview.chromium.org/329943004/diff/20001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/329943004/diff/20001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:33: BrandColor
LGTM if you set this to status=experimental and remove the comment and extra
blank lines.

Once we get approval from the API owners to ship, we can flip this to
status=stable.

[michaelbai, 2014-06-13 23:11:38]

https://codereview.chromium.org/329943004/diff/20001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/329943004/diff/20001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:33: BrandColor
On 2014/06/13 22:07:55, abarth wrote:
> LGTM if you set this to status=experimental and remove the comment and extra
> blank lines.
> 
> Once we get approval from the API owners to ship, we can flip this to
> status=stable.

Done.

[michaelbai, 2014-06-13 23:43:44]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-06-13 23:44:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/329943004/100001

[commit-bot: I haz the power, 2014-06-14 01:09:02]

Change committed as 176142

[pdr., 2014-06-14 01:17:03]

On 2014/06/14 01:09:02, I haz the power (commit-bot) wrote:
> Change committed as 176142

I think this should land but shouldn't we wait for 3 LGTMs from the API owners
on blink-dev?

[michaelbai, 2014-06-14 01:28:49]

On 2014/06/14 01:17:03, pdr wrote:
> On 2014/06/14 01:09:02, I haz the power (commit-bot) wrote:
> > Change committed as 176142
> 
> I think this should land but shouldn't we wait for 3 LGTMs from the API owners
> on blink-dev?

It behinds RuntimeEnabledFeature flag.

[pdr., 2014-06-14 01:41:05]

On 2014/06/14 01:28:49, michaelbai wrote:
> On 2014/06/14 01:17:03, pdr wrote:
> > On 2014/06/14 01:09:02, I haz the power (commit-bot) wrote:
> > > Change committed as 176142
> > 
> > I think this should land but shouldn't we wait for 3 LGTMs from the API
owners
> > on blink-dev?
> 
> It behinds RuntimeEnabledFeature flag.

Sorry, 3 LGTMs are not required but some level of consensus is asked for. I
would have liked to see Tab and Elliott happy.

I'm not asking for this to be reverted but I feel like we should be on our best
behavior with this. It doesn't seem onerous to shoot a simple spec off to
whatwg.

[michaelbai, 2014-06-14 01:49:05]

I asked account to update wiki, and hasn't gotten any response.


On Fri, Jun 13, 2014 at 6:41 PM, <pdr@chromium.org> wrote:

> On 2014/06/14 01:28:49, michaelbai wrote:
>
>> On 2014/06/14 01:17:03, pdr wrote:
>> > On 2014/06/14 01:09:02, I haz the power (commit-bot) wrote:
>> > > Change committed as 176142
>> >
>> > I think this should land but shouldn't we wait for 3 LGTMs from the API
>>
> owners
>
>> > on blink-dev?
>>
>
>  It behinds RuntimeEnabledFeature flag.
>>
>
> Sorry, 3 LGTMs are not required but some level of consensus is asked for. I
> would have liked to see Tab and Elliott happy.
>
> I'm not asking for this to be reverted but I feel like we should be on our
> best
> behavior with this. It doesn't seem onerous to shoot a simple spec off to
> whatwg.
>
> https://codereview.chromium.org/329943004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2014-06-14 02:44:10]

On 2014/06/14 at 01:41:05, pdr wrote:
> I'm not asking for this to be reverted but I feel like we should be on our
best behavior with this.

The gate for implementing an experimental feature is based on its impact on the
codebase rather than any progress along the standards track.  Given the
simplicity of this feature's implementation, it seemed easier to land it behind
a runtime flag.  We can either enable it (if we decide to ship it) or rip it out
(if we decide not to ship it).

> It doesn't seem onerous to shoot a simple spec off to whatwg.

I certainly agree with you that (at a bare minimum) we should register the
MetaExtension before shipping the feature.

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

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4746: for (HTMLMetaElement* metaElement = head() ?
Traversal<HTMLMetaElement>::firstChild(*head()) : 0; metaElement; metaElement =
Traversal<HTMLMetaElement>::nextSibling(*metaElement)) {
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.

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4748: if (equalIgnoringCase(metaElement->name(),
"brand-color") && CSSPropertyParser::fastParseColor(rgb,
metaElement->content().string().stripWhiteSpace(), true))
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.

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
File Source/web/tests/data/brand_color_test.html (right):

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
Source/web/tests/data/brand_color_test.html:1: <html>
missing doctype

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
Source/web/tests/data/brand_color_test.html:3: <title>Original Title</title>
What is "Original title"? We usually leave html, head, title and body out of our
tests since they're not needed.

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
Source/web/tests/data/brand_color_test.html:4: <meta name="brand-color"
content="  blue   ">
You really need more tests, what if I change this dynamically?

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

Thanks for the comments, please see my response inline.

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4746: for (HTMLMetaElement* metaElement = head() ?
Traversal<HTMLMetaElement>::firstChild(*head()) : 0; metaElement; metaElement =
Traversal<HTMLMetaElement>::nextSibling(*metaElement)) {
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:
> 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.

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4748: if (equalIgnoringCase(metaElement->name(),
"brand-color") && CSSPropertyParser::fastParseColor(rgb,
metaElement->content().string().stripWhiteSpace(), true))
We only support color name, rgba(), rgb() or "#" syntax in brand-color.


On 2014/06/19 10:37:04, esprehn wrote:
> 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.

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
File Source/web/tests/data/brand_color_test.html (right):

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
Source/web/tests/data/brand_color_test.html:1: <html>
I am not aware the test html doc has strict rule, I check the html file here
before I wrote this test and did see most of them don't have doctype, some of
them even have <html> tag 

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

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
Source/web/tests/data/brand_color_test.html:3: <title>Original Title</title>
I will have another patch to remove this.


On 2014/06/19 10:37:04, esprehn wrote:
> What is "Original title"? We usually leave html, head, title and body out of
our
> tests since they're not needed.

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
Source/web/tests/data/brand_color_test.html:4: <meta name="brand-color"
content="  blue   ">
I just test the main feature, as to notification of attribute change, it should
be the fundamental feature and have other tests to cover it.

On 2014/06/19 10:37:04, esprehn wrote:
> You really need more tests, what if I change this dynamically?

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

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4746: for (HTMLMetaElement* metaElement = head() ?
Traversal<HTMLMetaElement>::firstChild(*head()) : 0; metaElement; metaElement =
Traversal<HTMLMetaElement>::nextSibling(*metaElement)) {
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.

https://codereview.chromium.org/329943004/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:4748: if (equalIgnoringCase(metaElement->name(),
"brand-color") && CSSPropertyParser::fastParseColor(rgb,
metaElement->content().string().stripWhiteSpace(), true))
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.

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
File Source/web/tests/data/brand_color_test.html (right):

https://codereview.chromium.org/329943004/diff/100001/Source/web/tests/data/b...
Source/web/tests/data/brand_color_test.html:4: <meta name="brand-color"
content="  blue   ">
You don't test dynamic insertions, or having multiple meta elements. Indeed your
code doesn't work as you described (you take the first one, not the last). I
think you need more tests here.

[michaelbai, 2014-06-20 19:45:34]

esprehn@

Thanks for your comments, here is the update

- Last brand-color is my typo, we do want the first brand-color.
- We enforce the brand-color must be in head element.
- We supports all CSS color value.

I have sent you another CL for review.

Thanks all your help.

[eseidel, 2014-06-30 20:59:50]

It's really unfortunate that this CL is needed at all.  Ideally the web platform
should expose a better way to parse colors.

http://stackoverflow.com/questions/11068240/what-is-the-most-efficient-way-to...

[michaelbai, 2014-06-30 21:30:33]

On 2014/06/30 20:59:50, eseidel wrote:
> It's really unfortunate that this CL is needed at all.  Ideally the web
platform
> should expose a better way to parse colors.
> 
>
http://stackoverflow.com/questions/11068240/what-is-the-most-efficient-way-to...

Sorry, I don't understand your point, the given link is about parsing color by
javascript, what I did here is to notify the color to browser.