Bug 82927 - add css parsing of -webkit-flex
Summary: add css parsing of -webkit-flex
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 82128
  Show dependency treegraph
 
Reported: 2012-04-02 12:36 PDT by Tony Chang
Modified: 2012-04-02 17:24 PDT (History)
4 users (show)

See Also:


Attachments
Patch (55.08 KB, patch)
2012-04-02 12:37 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (54.86 KB, patch)
2012-04-02 13:30 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (54.68 KB, patch)
2012-04-02 13:48 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-04-02 12:36:15 PDT
add css parsing of -webkit-flex
Comment 1 Tony Chang 2012-04-02 12:37:45 PDT
Created attachment 135157 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-02 12:42:42 PDT
Attachment 135157 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.h:1259:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexis Menard (darktears) 2012-04-02 12:46:51 PDT
Comment on attachment 135157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135157&action=review

> Source/WebCore/css/CSSParser.cpp:1794
> +            if (RefPtr<CSSValue> flex = parseFlex(value->function->args.get())) {

If you fallback to the end of the switch with validPrimitive and parsedValue it is not good?
Comment 4 Tony Chang 2012-04-02 13:30:09 PDT
Created attachment 135168 [details]
Patch
Comment 5 Tony Chang 2012-04-02 13:30:53 PDT
(In reply to comment #3)
> (From update of attachment 135157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135157&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:1794
> > +            if (RefPtr<CSSValue> flex = parseFlex(value->function->args.get())) {
> 
> If you fallback to the end of the switch with validPrimitive and parsedValue it is not good?

Good idea; done.
Comment 6 Ojan Vafai 2012-04-02 13:32:03 PDT
Comment on attachment 135157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135157&action=review

Please address at least the test comments before committing.

> Source/WebCore/css/CSSParser.cpp:1799
> +        else if (value->unit == CSSParserValue::Function) {
> +            if (!equalIgnoringCase(value->function->name, "-webkit-flex(") || m_valueList->next())
> +                return false;
> +
> +            if (RefPtr<CSSValue> flex = parseFlex(value->function->args.get())) {
> +                addProperty(propId, flex.release(), important);
> +                return true;
> +            }
> +            return false;
> +        }

Maybe add a FIXME to remove this once we get rid of the flex function?

> Source/WebCore/rendering/style/StyleFlexibleBoxData.cpp:37
> +    : m_widthPositiveFlex(RenderStyle::initialPositiveFlex())
> +    , m_widthNegativeFlex(RenderStyle::initialNegativeFlex())
> +    , m_heightPositiveFlex(RenderStyle::initialPositiveFlex())
> +    , m_heightNegativeFlex(RenderStyle::initialNegativeFlex())

Add FIXME to kill these member variables?

> LayoutTests/css3/flexbox/flex-property-parsing.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

Whatever template file generated this is outdated.

> LayoutTests/css3/flexbox/flex-property-parsing.html:7
> +<p id="description"></p>

Don't need this line anymore.

> LayoutTests/css3/flexbox/flex-property-parsing.html:11
> +<div id="console"></div>

Don't need this line either.

> LayoutTests/css3/flexbox/flex-property-parsing.html:12
> +<script src="script-tests/flex-property-parsing.js"></script>

Can you inline this? There's no need to have a separate flex-parsing.js file. We should only use that pattern for pure JS test suites we want to be able to run outside of a browser context.
Comment 7 WebKit Review Bot 2012-04-02 13:33:41 PDT
Attachment 135168 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.h:1259:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Tony Chang 2012-04-02 13:48:59 PDT
Created attachment 135174 [details]
Patch for landing
Comment 9 Tony Chang 2012-04-02 13:52:18 PDT
I think the cq will fail to land this because of the style warning.
Comment 10 WebKit Review Bot 2012-04-02 17:24:46 PDT
Comment on attachment 135174 [details]
Patch for landing

Clearing flags on attachment: 135174

Committed r112968: <http://trac.webkit.org/changeset/112968>
Comment 11 WebKit Review Bot 2012-04-02 17:24:59 PDT
All reviewed patches have been landed.  Closing bug.