Bug 72478 - [CSSShaders] Implement the computed style for mesh parameters of the custom() filter
: [CSSShaders] Implement the computed style for mesh parameters of the custom()...
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: https://dvcs.w3.org/hg/FXTF/raw-file/...
:
:
: 71446
  Show dependency treegraph
 
Reported: 2011-11-15 23:54 PST by
Modified: 2011-11-18 04:13 PST (History)


Attachments
Patch V1 (29.09 KB, patch)
2011-11-16 02:39 PST, Alexandru Chiculita
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (35.49 KB, patch)
2011-11-18 03:40 PST, Alexandru Chiculita
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (35.49 KB, patch)
2011-11-18 03:41 PST, Alexandru Chiculita
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-15 23:54:34 PST
This patch should add the mesh parameters to the CustomFilterOperation class and also implement the CSSStyleSelector code that fills that data. Also, to be able to test the patch, the Computed Style will also be added.
------- Comment #1 From 2011-11-16 02:39:25 PST -------
Created an attachment (id=115354) [details]
Patch V1
------- Comment #2 From 2011-11-16 02:41:46 PST -------
(From update of attachment 115354 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=115354&action=review

> Source/WebCore/css/CSSParser.cpp:6649
> +            return 0;

This was needed to avoid parsing the invalid syntax "custom(none, 10, 10)" as "custom(none, 10)".
------- Comment #3 From 2011-11-17 20:22:05 PST -------
(From update of attachment 115354 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=115354&action=review

Looks good. I'd like to see the verbose computed style, but if you see other similar properties not doing it then I'm happy.

> LayoutTests/css3/filters/custom-filter-property-computed-style-expected.txt:41
> +PASS removeBaseURL(subRule.cssText) is 'custom(url(vertex.shader), 10)'

See below. I think we should return 10 10 for the computed value here.

> LayoutTests/css3/filters/custom-filter-property-computed-style-expected.txt:59
> +PASS removeBaseURL(subRule.cssText) is 'custom(none url(fragment.shader), detached)'

And similarly here should return "1 1 detached". In general I like having the computed style being a verbose form of the input.

> LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:44
>  testInvalidFilterRule("To many parameter values", "custom(url(shader), p1 1 2 3 4 5)");

Another typo here in the unchanged code.

> Source/WebCore/ChangeLog:12
> +        Also fixed a case where "custom(none, 10, 10 filter-box)" was actually
> +        treated as "custom(none, 10)".

was incorrectly treated

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:760
> +            if (customOperation->meshRows() > 0 || customOperation->meshColumns() > 0) {
> +                meshParameters->append(primitiveValueCache->createValue(customOperation->meshRows(), CSSPrimitiveValue::CSS_NUMBER));
> +                if (customOperation->meshRows() != customOperation->meshColumns())
> +                    meshParameters->append(primitiveValueCache->createValue(customOperation->meshColumns(), CSSPrimitiveValue::CSS_NUMBER));
> +            }

I think the computed style for a single mesh input should still give both x and y values. This is what we do for things like translate(10px) which is computed as translate(10px, 0).

> Source/WebCore/css/CSSStyleSelector.cpp:5507
> +        // The second value might be the mesh box or the list of paramters:

typo parameters

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:57
> +    static PassRefPtr<CustomFilterOperation> create(PassRefPtr<StyleShader> vertexShader, PassRefPtr<StyleShader> fragmentShader,
> +        unsigned meshRows, unsigned meshColumns, MeshBoxType meshBoxType, MeshType meshType)

I think here we tend to indent to align with the first parameter. (I'm not sure)

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:88
>          return m_vertexShader.get() == other->m_vertexShader.get()
> -            && m_fragmentShader.get() == other->m_fragmentShader.get();
> +            && m_fragmentShader.get() == other->m_fragmentShader.get()
> +            && m_meshRows == other->m_meshRows
> +            && m_meshColumns == other->m_meshColumns
> +            && m_meshBoxType == other->m_meshBoxType
> +            && m_meshType == other->m_meshType;
>      }
>      

Same here (maybe :)

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:91
> +    CustomFilterOperation(PassRefPtr<StyleShader> vertexShader, PassRefPtr<StyleShader> fragmentShader,
> +        unsigned meshRows, unsigned meshColumns, MeshBoxType meshBoxType, MeshType meshType)
>          : FilterOperation(CUSTOM)

.. and here (again maybe :)
------- Comment #4 From 2011-11-18 03:40:16 PST -------
Created an attachment (id=115781) [details]
Patch for landing
------- Comment #5 From 2011-11-18 03:41:52 PST -------
Created an attachment (id=115783) [details]
Patch for landing
------- Comment #6 From 2011-11-18 03:48:59 PST -------
(In reply to comment #3)
> (From update of attachment 115354 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115354&action=review
> 
> Looks good. I'd like to see the verbose computed style, but if you see other similar properties not doing it then I'm happy.

Thanks for the review. I converted the patch to output all the parameters, even if they are only the default values and could be omitted. It's easier to parse the computed style in JS.
------- Comment #7 From 2011-11-18 04:13:04 PST -------
(From update of attachment 115783 [details])
Clearing flags on attachment: 115783

Committed r100749: <http://trac.webkit.org/changeset/100749>
------- Comment #8 From 2011-11-18 04:13:09 PST -------
All reviewed patches have been landed.  Closing bug.