Adds filter support for offscreen canvas

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 2004-2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2006, 2007 Nicholas Shanks (webkit@nickshanks.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc.
  * All rights reserved.
  * Copyright (C) 2007 Alexey Proskuryakov <ap@webkit.org>
  * Copyright (C) 2007, 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
@@ skipping 101 matching lines @@
       feature = UseCounter::CSSFilterBlur;
       break;
     case FilterOperation::DROP_SHADOW:
       feature = UseCounter::CSSFilterDropShadow;
       break;
   };
   UseCounter::count(document, feature);
 }
 
 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;
 
   if (inValue.isIdentifierValue()) {
     DCHECK_EQ(toCSSIdentifierValue(inValue).getValueID(), CSSValueNone);
     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;
     }
 
     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 =
         filterValue->length() && filterValue->item(0).isPrimitiveValue()
             ? &toCSSPrimitiveValue(filterValue->item(0))
             : nullptr;
     switch (filterValue->functionType()) {
       case CSSValueGrayscale:
       case CSSValueSepia:
       case CSSValueSaturate: {
@@ skipping 29 matching lines @@
             amount /= 100;
         }
 
         operations.operations().append(
             BasicComponentTransferFilterOperation::create(amount,
                                                           operationType));
         break;
       }
       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));
         break;
       }
       default:
         ASSERT_NOT_REACHED();
         break;
     }
   }
 
   return operations;
 }
 
 }  // namespace blink