Move viewport descriptor handling into CSSPropertyParser

Move viewport descriptor handling into CSSPropertyParser

Move viewport descriptor handling from LegacyCSSPropertyParser
into CSSPropertyParser and rename parseViewportProperty
to parseViewportDescriptor.

BUG=499780
Committed: https://crrev.com/d7413bda9f69b7ab5ad3e6e69660e9132df808dd
Cr-Commit-Position: refs/heads/master@{#353116}

[rwlbuis, 2015-10-06 21:11:40]

rob.buis@samsung.com changed reviewers:
+ alancutter@chromium.org, timloh@chromium.org

[rwlbuis, 2015-10-06 21:11:40]

PTAL.

[Timothy Loh, 2015-10-07 00:18:33]

Not sure if (min-,max-,)zoom should be accepting calc here, alancutter WDYT?

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:296: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumePercent(CSSParserTokenRange&
range, ValueRange valueRange)
Probably makes more sense if this is before consumeLengthOrPercent

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:803: static
PassRefPtrWillBeRawPtr<CSSValue> consumeViewportLonghand(CSSParserTokenRange&
range, CSSPropertyID propId, CSSParserMode cssParserMode)
consumeSingleViewportDescriptor (or something better)? I don't like the name
longhand here because longhands are properties/descriptors that can be expanded
from shorthands.

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:813: parsedValue
= consumeIdent(range);
return consumeIdent(range); and so on for the rest of the function?

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:850: case
CSSPropertyWidth: {
I think there's an assertion in addProperty that can be updated or removed

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:854:
addProperty(CSSPropertyMinWidth, minWidth, important);
I think this would be slightly nicer as:

RefPtrWillBeRawPtr<CSSValue> minWidth = consume..;
if (!minWidth)
    return false;

RefPtrWillBeRawPtr<CSSValue> maxWidth = minWidth;
if (!m_range.atEnd())
    maxWidth = consume...
if (!maxWidth || !m_range.atEnd())
    return false;

addProperty(.., minWidth, ..);
addProperty(.., maxWidth, ..);

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:889: default:
How about return false in this default and ASSERT_NOT_REACHED for default in the
other function?

[rwlbuis, 2015-10-07 23:08:53]

PTAL.

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:296: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumePercent(CSSParserTokenRange&
range, ValueRange valueRange)
On 2015/10/07 00:18:33, Timothy Loh wrote:
> Probably makes more sense if this is before consumeLengthOrPercent 

Done.

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:803: static
PassRefPtrWillBeRawPtr<CSSValue> consumeViewportLonghand(CSSParserTokenRange&
range, CSSPropertyID propId, CSSParserMode cssParserMode)
On 2015/10/07 00:18:33, Timothy Loh wrote:
> consumeSingleViewportDescriptor (or something better)? I don't like the name
> longhand here because longhands are properties/descriptors that can be
expanded
> from shorthands.

Done.

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:813: parsedValue
= consumeIdent(range);
On 2015/10/07 00:18:33, Timothy Loh wrote:
> return consumeIdent(range); and so on for the rest of the function?

Yeah much better :) Done.

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:850: case
CSSPropertyWidth: {
On 2015/10/07 00:18:33, Timothy Loh wrote:
> I think there's an assertion in addProperty that can be updated or removed

Done.

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:854:
addProperty(CSSPropertyMinWidth, minWidth, important);
On 2015/10/07 00:18:33, Timothy Loh wrote:
> I think this would be slightly nicer as:
> 
> RefPtrWillBeRawPtr<CSSValue> minWidth = consume..;
> if (!minWidth)
>     return false;
> 
> RefPtrWillBeRawPtr<CSSValue> maxWidth = minWidth;
> if (!m_range.atEnd())
>     maxWidth = consume...
> if (!maxWidth || !m_range.atEnd())
>     return false;
> 
> addProperty(.., minWidth, ..);
> addProperty(.., maxWidth, ..);

Done.

https://codereview.chromium.org/1380083004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:889: default:
On 2015/10/07 00:18:33, Timothy Loh wrote:
> How about return false in this default and ASSERT_NOT_REACHED for default in
the
> other function?

Done.

[Timothy Loh, 2015-10-08 01:05:21]

lgtm

https://codereview.chromium.org/1380083004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1380083004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:277: static
inline bool isCSSWideKeyword(const CSSValueID& id)
not sure if this was supposed to be moved?

https://codereview.chromium.org/1380083004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:940:
RefPtrWillBeRawPtr<CSSValue> parsedValue = consumeNumber(range,
ValueRangeNonNegative);
I prefer:

if (RefPtrWillBeRawPtr<CSSValue> parsedValue = consumeNumber(range,
ValueRangeNonNegative))
    return parsedValue.release();
return consumePercent(Range, ValueRangeNonNegative);

https://codereview.chromium.org/1380083004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:965:
m_range.consumeWhitespace();
We should probably do this up front somewhere so it doesn't need to be in
parseViewportDescriptor/parseFontFaceDescriptor/parseShorthand/parseSingleValue,
but there's no reason for that to be in this patch.

https://codereview.chromium.org/1380083004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1004:
parsedValue = consumeSingleViewportDescriptor(m_range, propId,
m_context.mode());
if we only set parsedValue here, we should just move the addProperty logic and
variable declaration into here

[rwlbuis, 2015-10-08 02:06:25]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-10-08 02:06:26]

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/1380083004/#ps120001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-10-08 02:07:22]

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

[commit-bot: I haz the power, 2015-10-08 02:10:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 02:10:01]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rwlbuis, 2015-10-08 14:15:24]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-10-08 14:15:25]

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/1380083004/#ps140001
(title: "Another attempt")

[commit-bot: I haz the power, 2015-10-08 14:15:51]

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

[commit-bot: I haz the power, 2015-10-08 17:05:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 17:05:43]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[rwlbuis, 2015-10-08 18:08:48]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-10-08 18:09:13]

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

[commit-bot: I haz the power, 2015-10-08 19:09:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-08 19:09:42]

Patchset 8 (id:??) landed as
https://crrev.com/d7413bda9f69b7ab5ad3e6e69660e9132df808dd
Cr-Commit-Position: refs/heads/master@{#353116}