Adds filter support for offscreen canvas

third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp

Index: third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
diff --git a/third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp b/third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
index 041f1914cd3145754a62c46196089ce2942d2042..8d7d637e0fe3aaeabf7f962cd0cb5a2ac2db27d5 100644
--- a/third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
+++ b/third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
@@ -119,7 +119,7 @@ static void countFilterUse(FilterOperation::OperationType operationType,
 }
 
 FilterOperations FilterOperationResolver::createFilterOperations(
-    StyleResolverState& state,
+    StyleResolverState* state,

[Justin Novosad, 2016/10/24 18:51:22]

This approach seems reasonable to me, but I'll defer to esprehn@ on this.

[esprehn, 2016/11/22 01:38:37]

Lets refactor this into two methods, one that takes a state and one that doesn't
and move the state to be the second argument.

     const CSSValue& inValue) {
   FilterOperations operations;
 
@@ -128,24 +128,29 @@ FilterOperations FilterOperationResolver::createFilterOperations(
     return operations;
   }
 
-  const CSSToLengthConversionData& conversionData =
-      state.cssToLengthConversionData();
+  const CSSToLengthConversionData* conversionData;
+  if (state)
+    conversionData = &(state->cssToLengthConversionData());
+
   for (auto& currValue : toCSSValueList(inValue)) {
     if (currValue->isURIValue()) {
-      countFilterUse(FilterOperation::REFERENCE, state.document());
+      if (!state)
+        continue;

[meade_UTC10, 2016/10/26 06:57:34]

This for loop is big and kind of hard to reason about, so I think adding more
complexity to it is not so great. Is there some way you can modularise what
you're trying to do here to make it easier to read?

For example, this continue statement causes everything from here to line 250 to
be skipped, not just to line 157 as I first thought. Instead there could be a
method for handling a URIValue (136-157), and another method for handling a
FunctionValue (159-25). This could make it easier to unit test the behaviour,
which is also good :)

[fserb, 2016/10/26 19:08:26]

True. Although there's nothing to do if there's a URIValue and no State/document
(and the specs are fine with that, it seems). We should really skip everything
and not return a proper operation if we get a url. But yes, you are right that
this makes the code a bit confusing. I'll try to address that.

+      countFilterUse(FilterOperation::REFERENCE, state->document());
 
       const CSSURIValue& urlValue = toCSSURIValue(*currValue);
-      SVGURLReferenceResolver resolver(urlValue.value(), state.document());
+      SVGURLReferenceResolver resolver(urlValue.value(), state->document());
       ReferenceFilterOperation* operation = ReferenceFilterOperation::create(
           urlValue.value(), resolver.fragmentIdentifier());
       if (!resolver.isLocal()) {
-        if (!urlValue.loadRequested())
-          state.elementStyleResources().addPendingSVGDocument(operation,
-                                                              &urlValue);
-        else if (urlValue.cachedDocument())
+        if (!urlValue.loadRequested()) {
+          state->elementStyleResources().addPendingSVGDocument(operation,
+                                                               &urlValue);
+        } else if (urlValue.cachedDocument()) {
           ReferenceFilterBuilder::setDocumentResourceReference(
               operation,
               new DocumentResourceReference(urlValue.cachedDocument()));
+        }
       }
       operations.operations().append(operation);
       continue;
@@ -154,7 +159,8 @@ FilterOperations FilterOperationResolver::createFilterOperations(
     const CSSFunctionValue* filterValue = toCSSFunctionValue(currValue.get());
     FilterOperation::OperationType operationType =
         filterOperationForType(filterValue->functionType());
-    countFilterUse(operationType, state.document());
+    if (state)
+      countFilterUse(operationType, state->document());
     DCHECK_LE(filterValue->length(), 1u);
 
     const CSSPrimitiveValue* firstValue =
@@ -204,22 +210,34 @@ FilterOperations FilterOperationResolver::createFilterOperations(
       }
       case CSSValueBlur: {
         Length stdDeviation = Length(0, Fixed);
-        if (filterValue->length() >= 1)
-          stdDeviation = firstValue->convertToLength(conversionData);
+        if (filterValue->length() >= 1) {
+          if (state)
+            stdDeviation = firstValue->convertToLength(*conversionData);
+          else
+            stdDeviation = Length(firstValue->getDoubleValue(), Fixed);

[Justin Novosad, 2016/10/24 18:51:22]

This part is doubtful. We probably want some kind of default converisionData to
kick in in order to get the right unit conversions. Physical units like cm/in
don't really make a lot of sense for an object that is off screen.  'em' doesn't
make a lot of sense either.  We need to think about this. Do we want to allow
only 'px' in the case of OffscreenCanvas?  I am okay with solving this in a
follow-up CL, but we can't ship  until this is resolved.

[fserb, 2016/10/26 19:08:26]

I did try to create a deafult ConversionData. I'm up for that. But I couldn't do
anything that worked, I can try again.

+        }
         operations.operations().append(
             BlurFilterOperation::create(stdDeviation));
         break;
       }
       case CSSValueDropShadow: {
         const CSSShadowValue& item = toCSSShadowValue(filterValue->item(0));
-        IntPoint location(item.x->computeLength<int>(conversionData),
-                          item.y->computeLength<int>(conversionData));
-        int blur =
-            item.blur ? item.blur->computeLength<int>(conversionData) : 0;
+        IntPoint location;
+        int blur;
+
+        if (state) {
+          location = IntPoint(item.x->computeLength<int>(*conversionData),
+                              item.y->computeLength<int>(*conversionData));
+          blur = item.blur ? item.blur->computeLength<int>(*conversionData) : 0;
+        } else {
+          location = IntPoint(item.x->getIntValue(), item.y->getIntValue());
+          blur = item.blur ? item.blur->getIntValue() : 0;

[Justin Novosad, 2016/10/24 18:51:22]

More doubtful conversions.

[fserb, 2016/10/26 19:08:26]

I'll try that.

+        }
         Color shadowColor = Color::black;
-        if (item.color)
-          shadowColor = state.document().textLinkColors().colorFromCSSValue(
-              *item.color, state.style()->color());
+        if (item.color && state) {
+          shadowColor = state->document().textLinkColors().colorFromCSSValue(
+              *item.color, state->style()->color());
+        }
 
         operations.operations().append(
             DropShadowFilterOperation::create(location, blur, shadowColor));