Made CSSPrimitiveValue::getString() only work for String types

Made CSSPrimitiveValue::getString() only work for String types

Cleaned up CSSPrimitiveValue::getString() to only return the string
value if the unit type is a string (attribute, URL, String or custom
ident). Also cleaned up callsites and added asserts to better document
the expected behavior.

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

[sashab, 2015-08-26 04:14:07]

sashab@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2015-08-27 01:30:09]

Patch title/description should say getStringValue instead of getString

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
File Source/core/css/CSSCounterValue.h (right):

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
Source/core/css/CSSCounterValue.h:38: String listStyle() const { return
getValueName(m_listStyle->getValueID()); }
We should probably just remove this function and update callers appropriately...

https://codereview.chromium.org/1307673003/diff/40001/Source/web/WebAXObject.cpp
File Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/1307673003/diff/40001/Source/web/WebAXObject....
Source/web/WebAXObject.cpp:1243: return
WebString(String(getValueName(CSSPrimitiveValue::create(computedStyle->display())->getValueID())));
->cssText()?

[sashab, 2015-08-27 06:30:16]

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
File Source/core/css/CSSCounterValue.h (right):

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
Source/core/css/CSSCounterValue.h:38: String listStyle() const { return
getValueName(m_listStyle->getValueID()); }
On 2015/08/27 01:30:09, Timothy Loh wrote:
> We should probably just remove this function and update callers
appropriately...

Which function are you talking about? I updated this to use cssText() but we
could rename listStyleIdent() to just listStyle() and update callers that way,
if that's what you meant.

https://codereview.chromium.org/1307673003/diff/40001/Source/web/WebAXObject.cpp
File Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/1307673003/diff/40001/Source/web/WebAXObject....
Source/web/WebAXObject.cpp:1243: return
WebString(String(getValueName(CSSPrimitiveValue::create(computedStyle->display())->getValueID())));
On 2015/08/27 01:30:09, Timothy Loh wrote:
> ->cssText()?

This whole creation of CSSPrimitiveValue isn't actually needed... Maybe what we
need is a toString(platform enum) function.

[Timothy Loh, 2015-08-27 06:38:49]

lgtm, but you'll need a Source/web OWNER too

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
File Source/core/css/CSSCounterValue.h (right):

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
Source/core/css/CSSCounterValue.h:38: String listStyle() const { return
getValueName(m_listStyle->getValueID()); }
On 2015/08/27 06:30:16, sashab wrote:
> On 2015/08/27 01:30:09, Timothy Loh wrote:
> > We should probably just remove this function and update callers
> appropriately...
> 
> Which function are you talking about? I updated this to use cssText() but we
> could rename listStyleIdent() to just listStyle() and update callers that way,
> if that's what you meant.

listStyle(); I think it should just be inlined into customCSSText (separate
patch?)

https://codereview.chromium.org/1307673003/diff/60001/Source/web/WebAXObject.cpp
File Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/1307673003/diff/60001/Source/web/WebAXObject....
Source/web/WebAXObject.cpp:34: #include "core/CSSValueKeywords.h"
so this include isn't need anymore?

[sashab, 2015-08-27 06:53:38]

sashab@chromium.org changed reviewers:
+ esprehn@chromium.org

[sashab, 2015-08-27 06:53:39]

esprehn ptal :)

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
File Source/core/css/CSSCounterValue.h (right):

https://codereview.chromium.org/1307673003/diff/40001/Source/core/css/CSSCoun...
Source/core/css/CSSCounterValue.h:38: String listStyle() const { return
getValueName(m_listStyle->getValueID()); }
On 2015/08/27 06:38:49, Timothy Loh wrote:
> On 2015/08/27 06:30:16, sashab wrote:
> > On 2015/08/27 01:30:09, Timothy Loh wrote:
> > > We should probably just remove this function and update callers
> > appropriately...
> > 
> > Which function are you talking about? I updated this to use cssText() but we
> > could rename listStyleIdent() to just listStyle() and update callers that
way,
> > if that's what you meant.
> 
> listStyle(); I think it should just be inlined into customCSSText (separate
> patch?)

Oh I see. Yeah OK, I'll do this in a separate patch.

https://codereview.chromium.org/1307673003/diff/60001/Source/web/WebAXObject.cpp
File Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/1307673003/diff/60001/Source/web/WebAXObject....
Source/web/WebAXObject.cpp:34: #include "core/CSSValueKeywords.h"
On 2015/08/27 06:38:49, Timothy Loh wrote:
> so this include isn't need anymore?

Done.

[sashab, 2015-08-31 00:27:21]

ping esprehn :)

[esprehn, 2015-08-31 01:18:31]

lgtm, this is just to avoid duplicating what cssText() already does?

[sashab, 2015-08-31 01:20:35]

Yup pretty much.

[sashab, 2015-08-31 01:20:38]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-08-31 01:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1307673003/#ps80001
(title: "Rebase and review feedback")

[commit-bot: I haz the power, 2015-08-31 01:20:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307673003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307673003/80001

[commit-bot: I haz the power, 2015-08-31 02:46:31]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201459