Split out CSSParser public API

Split out CSSParser public API

The goal was to identify and separate out the "public" API
for the CSS parsing subsystem from the larger set
of public methods on CSSParser (now BisonCSSParser)
which are marked public just for CSSGrammer.y to consume.

BisonCSSParser (previously CSSParser) exposes over *300*
public methods and members! All but the 15 copied to
CSSParser were just for CSSGrammar.y integration.

This is all in preparation for attempting to swap out
the CSSParser wholesale with a from-scratch implementation
of the CSS3 Syntax specification. CSSParser.cpp now provides
a place for me to do that run-time switching between
the Bison-based parser and my new hand-rolled parser (not yet landed).

Much of what is now BisonCSSParser will need to be split out
into a separate "CSSBuilder" object or something akin to the
HTMLConstructionSite and re-used by my proposed re-written CSS parser
as the CSS3 syntax spec doesn't specify all the details encapsulated
in the old parser grammar file.

Follow-up patches will work to further reduce the exposed CSSParser
API and remove more of this redundant indirection.

This was originally tracked as:
https://codereview.chromium.org/121673002
but as moved here due to rietveld 500 errors.

BUG=330389

[esprehn, 2013-12-31 01:52:38]

I'll review this as soon as I can.

[eseidel, 2014-01-01 17:06:37]

I think we need to consider this patch on its own merrits and not on the promise
of a future parser.  After several readthroughs, it's not clear to me that we
want to implement the CSS Parsing Spec anymore.  But I do think we want to bring
some sanity to the current craziness that is BisonCSSParser.  Perhaps by
breaking it up into pieces instead of adding a layer of abstraction in front of
it.

[darktears, 2014-01-02 21:10:07]

On 2014/01/01 17:06:37, eseidel wrote:
> I think we need to consider this patch on its own merrits and not on the
promise
> of a future parser.  After several readthroughs, it's not clear to me that we
> want to implement the CSS Parsing Spec anymore.  But I do think we want to
bring
> some sanity to the current craziness that is BisonCSSParser.  Perhaps by
> breaking it up into pieces instead of adding a layer of abstraction in front
of
> it.

Right, you may want to update the description slightly.

I don't think it's going to hurt to separate what Bison uses internally through
CSSGrammar.y and the API used in the Blink engine to parse chunk of CSS code.

[darktears, 2014-01-02 21:10:18]

On 2014/01/02 21:10:07, darktears wrote:
> On 2014/01/01 17:06:37, eseidel wrote:
> > I think we need to consider this patch on its own merrits and not on the
> promise
> > of a future parser.  After several readthroughs, it's not clear to me that
we
> > want to implement the CSS Parsing Spec anymore.  But I do think we want to
> bring
> > some sanity to the current craziness that is BisonCSSParser.  Perhaps by
> > breaking it up into pieces instead of adding a layer of abstraction in front
> of
> > it.
> 
> Right, you may want to update the description slightly.
> 
> I don't think it's going to hurt to separate what Bison uses internally
through
> CSSGrammar.y and the API used in the Blink engine to parse chunk of CSS code.

LGTM

[ojan, 2014-02-19 19:52:30]

lgtm

https://codereview.chromium.org/112933010/diff/1/Source/core/css/parser/CSSPa...
File Source/core/css/parser/CSSParser.h (right):

https://codereview.chromium.org/112933010/diff/1/Source/core/css/parser/CSSPa...
Source/core/css/parser/CSSParser.h:74: // Only used by MediaList.cpp, always in
HTMLStandardMode.
I'm not sure these "Only used for" comments are terribly helpful. You can easily
discover that with a quick grep of the code and I worry that they'd become
stale.

Not a big deal though. Up to you.

https://codereview.chromium.org/112933010/diff/1/Source/core/css/resolver/Fil...
File Source/core/css/resolver/FilterOperationResolver.cpp (right):

https://codereview.chromium.org/112933010/diff/1/Source/core/css/resolver/Fil...
Source/core/css/resolver/FilterOperationResolver.cpp:34: #include
"core/css/parser/BisonCSSParser.h" // FIXME: For isBlendMode, etc.
It's not clear what the FIXME task here is. Remove the include? Change it to
CSSParser.h?

https://codereview.chromium.org/112933010/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/112933010/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.cpp:390: // FIXME: Why ignore the return value?
Maybe it always returns true. Even if so, I think the FIXME is still valid. It
should ASSERT.

https://codereview.chromium.org/112933010/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasStyle.cpp (right):

https://codereview.chromium.org/112933010/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasStyle.cpp:61: CSSParser::parseColor(rgba,
canvas->inlineStyle()->getPropertyValue(CSSPropertyColor));
Should this also have a FIXME about ignoring the return value?

[haraken, 2014-02-19 22:57:43]

On 2014/02/19 19:52:30, ojan wrote:
> lgtm
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/css/parser/CSSPa...
> File Source/core/css/parser/CSSParser.h (right):
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/css/parser/CSSPa...
> Source/core/css/parser/CSSParser.h:74: // Only used by MediaList.cpp, always
in
> HTMLStandardMode.
> I'm not sure these "Only used for" comments are terribly helpful. You can
easily
> discover that with a quick grep of the code and I worry that they'd become
> stale.
> 
> Not a big deal though. Up to you.
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/css/resolver/Fil...
> File Source/core/css/resolver/FilterOperationResolver.cpp (right):
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/css/resolver/Fil...
> Source/core/css/resolver/FilterOperationResolver.cpp:34: #include
> "core/css/parser/BisonCSSParser.h" // FIXME: For isBlendMode, etc.
> It's not clear what the FIXME task here is. Remove the include? Change it to
> CSSParser.h?
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/editing/EditingS...
> File Source/core/editing/EditingStyle.cpp (right):
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/editing/EditingS...
> Source/core/editing/EditingStyle.cpp:390: // FIXME: Why ignore the return
value?
> Maybe it always returns true. Even if so, I think the FIXME is still valid. It
> should ASSERT.
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/html/canvas/Canv...
> File Source/core/html/canvas/CanvasStyle.cpp (right):
> 
>
https://codereview.chromium.org/112933010/diff/1/Source/core/html/canvas/Canv...
> Source/core/html/canvas/CanvasStyle.cpp:61: CSSParser::parseColor(rgba,
> canvas->inlineStyle()->getPropertyValue(CSSPropertyColor));
> Should this also have a FIXME about ignoring the return value?

[haraken, 2014-02-19 22:58:06]

rubberstamp LGTM for bindings/

[tfarina, 2014-02-22 00:16:50]

https://codereview.chromium.org/112933010/diff/1/Source/core/css/parser/CSSPa...
File Source/core/css/parser/CSSParser.h (right):

https://codereview.chromium.org/112933010/diff/1/Source/core/css/parser/CSSPa...
Source/core/css/parser/CSSParser.h:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
2014 and use the shorter license template?

[esprehn, 2014-04-29 22:15:16]

Any more work on this eseidel?