Closed Bug 1371199 Opened 7 years ago Closed 7 years ago

stylo: No zero value returned for SVG paint servers

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1371199 - Update Stylo test expectations for reftests that use by-animation of 'fill'.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Servo PR, #17467
41 bytes, text/x-github-pull-request
Details | Review 
layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg fails on Stylo.

This test code is as follows:

 var animAttrHash = { "attributeName" : "fill",
                      "by"            : "#AAF573" };
 testAnimatedRectGrid("animate", [animAttrHash]);

Adding some loggingto the code I see:

  Parsed fill property value '#AAF573' => rgb(170, 245, 115)
  Failed to get zero_value

That is, the call to get_zero_value in Servo_AnimationValues_GetZeroValue is producing none.

I guess we need to implement get_zero_value on IntermediateSVGPaint.
Priority: -- → P2
(In reply to Brian Birtles (:birtles) from comment #0)
> layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg fails on
> Stylo.
> 
> This test code is as follows:
> 
>  var animAttrHash = { "attributeName" : "fill",
>                       "by"            : "#AAF573" };
>  testAnimatedRectGrid("animate", [animAttrHash]);
> 
> Adding some loggingto the code I see:
> 
>   Parsed fill property value '#AAF573' => rgb(170, 245, 115)
>   Failed to get zero_value
> 
> That is, the call to get_zero_value in Servo_AnimationValues_GetZeroValue is
> producing none.
> 
> I guess we need to implement get_zero_value on IntermediateSVGPaint.

I tried to add the get_zero_value of IntermediateSVGPaint, however it didn't pass this test case.
If we return zero value of IntermediateSVGPaint, stylo will call 'accumulate' with zero value and specified value with 'by'.

Brian,
I think that we will animation as accumulate from current color(i.e. in this test, Indigo) to added specified color(i.e. Indigo + #AAF573) in this case.
Is my understand correct?
Flags: needinfo?(bbirtles)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> (In reply to Brian Birtles (:birtles) from comment #0)
> > layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg fails on
> > Stylo.
> > 
> > This test code is as follows:
> > 
> >  var animAttrHash = { "attributeName" : "fill",
> >                       "by"            : "#AAF573" };
> >  testAnimatedRectGrid("animate", [animAttrHash]);
> > 
> > Adding some loggingto the code I see:
> > 
> >   Parsed fill property value '#AAF573' => rgb(170, 245, 115)
> >   Failed to get zero_value
> > 
> > That is, the call to get_zero_value in Servo_AnimationValues_GetZeroValue is
> > producing none.
> > 
> > I guess we need to implement get_zero_value on IntermediateSVGPaint.
> 
> I tried to add the get_zero_value of IntermediateSVGPaint, however it didn't
> pass this test case.
> If we return zero value of IntermediateSVGPaint, stylo will call
> 'accumulate' with zero value and specified value with 'by'.
> 
> Brian,
> I think that we will animation as accumulate from current color(i.e. in this
> test, Indigo) to added specified color(i.e. Indigo + #AAF573) in this case.
> Is my understand correct?

I discussed with Brian, I added the log into geckolib/glue.rs, maybe my modification is wrong.
I added following get_zero_value function into IntermediateSVGPaint:

-------------------------------------------------------------
    fn get_zero_value(&self) -> Option<Self> {
        Some(IntermediateSVGPaint {
            kind: self.kind.get_zero_value().unwrap(),
            fallback: Some(IntermediateRGBA::transparent()),
        })
    }
-------------------------------------------------------------

However, specified value with 'by' don't contain the fallback(i.e. fallback is None), So we can't add() operation with zero value and specified value. We will need to following get_zero_value function:

-------------------------------------------------------------
    fn get_zero_value(&self) -> Option<Self> {
        Some(IntermediateSVGPaint {
            kind: self.kind.get_zero_value().unwrap(),
            fallback: None,
        })
    }
-------------------------------------------------------------
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Flags: needinfo?(bbirtles)
Better still, we should match |self|. If |self| contains a fallback, we should fill in fallback with a zero value, otherwise we make it None.
Thank you for your feedback.

(In reply to Brian Birtles (:birtles) from comment #4)
> Better still, we should match |self|. If |self| contains a fallback, we
> should fill in fallback with a zero value, otherwise we make it None.

OK. 
I didn't care the containing the fallback value.
I've added these fallback matching:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=edf64f95ac6a727f717ec9099eddf073400e7eb0

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e012a4f63f4cfcfb9bb3d594435a7ad5636f774c

Two test failed.
 1) layout/reftests/svg/smil/style/anim-css-fill-overflow-1-by.svg 
      -> It will be test success, we can remove test fail annotation as well.

 2) layout/reftests/svg/smil/anim-paintserver-1.svg
      -> It willbe test fail.

If we run following test-case, gecko render lime rectange, however stylo didn't render anything. I added log into geckolib/glue.rs(Servo_AnimationValues_Interpolate/Servo_AnimationValues_Add/Servo_AnimationValues_Accumulate), but any functions didn't called.
----------------------------------------
<svg xmlns="http://www.w3.org/2000/svg">
  <defs>
    <linearGradient id="lime">
      <stop offset="0.0" stop-color="lime"/>
    </linearGradient>
  </defs>
  <rect x="0" y="0" width="100" height="100" fill="url(#lime)">
    <animate attributeName="fill" to="pikapikaglittergold"/>
  </rect>
</svg>
----------------------------------------
Blocks: 1374161
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review155384

Looks good but I'd like to check again with the get_zero_value for Option defined, assuming that works and you agree it makes sense.

::: commit-message-72346:1
(Diff revision 1)
> +Bug 1371199 - Add get_zero_value of IntermediateSVGPaint. r?birtles

Nit: s/of/for/

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2991
(Diff revision 1)
> +            fallback: match self.fallback {
> +                Some(_) => Some(IntermediateRGBA::transparent()),
> +                None => None,
> +            },

Could we just define get_zero_value for Option where we have:

  impl <T> Animatable for Option<T>
    where T: Animatable,

and then use that here?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2997
(Diff revision 1)
> +                Some(_) => Some(IntermediateRGBA::transparent()),
> +                None => None,
> +            },
> +        })
> +    }
> +

Nit: no need for the whitespace here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:3035
(Diff revision 1)
> +            &SVGPaintKind::None => {
> +                Some(SVGPaintKind::None)
> +            },
> +            &SVGPaintKind::ContextFill => {
> +                Some(SVGPaintKind::ContextFill)
> +            },
> +            &SVGPaintKind::ContextStroke => {
> +                Some(SVGPaintKind::ContextStroke)
> +            },

I don't know much about rust but it seems like there's probably a way to right this more simply.

e.g.

  &SVGPaintKind::None |
  &SVGPaintKind::ContextFill |
  &SVGPaintKind::ContextStroke => Some(self.clone()),

?

(There might be an even better way still)
Attachment #8879357 - Flags: review?(bbirtles)
Comment on attachment 8879358 [details]
Bug 1371199 - Update Stylo test expectations for reftests that use by-animation of 'fill'.

https://reviewboard.mozilla.org/r/150654/#review155396

::: commit-message-b8ac7:1
(Diff revision 1)
> +Bug 1371199 - Enable reftests of SVG related with 'by' on stylo. r?birtles

Update Stylo test expectations for reftests that use by-animation of 'fill'
Attachment #8879358 - Flags: review?(bbirtles) → review+
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review155952

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2998
(Diff revision 2)
>      }
> +
> +    #[inline]
> +    fn get_zero_value(&self) -> Option<Self> {
> +        Some(IntermediateSVGPaint {
> +            kind: self.kind.get_zero_value().unwrap(),

This doesn't seem right. get_zero_value for IntermediateSVGPaintKind can return None in which case unwrap() will panic.

I think you can use `kind: option_try!(self.kind.get_zero_value())` here.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2999
(Diff revision 2)
> +            // get_zero_value trait return Option<Self>, So if we call
> +            // Option's get_zero_value, return value will be
> +            // Option<Option<T>> since Self is Option<T>.
> +            fallback: self.fallback.get_zero_value().unwrap_or(None),

Will rust actually complain if we change the type of get_zero_value() for Option to Self? (i.e. not Option<Self>)?

If it does complain, perhaps we should just call get_zero_value on Option something else, like get_unwrapped_zero_value (get_inner_zero_value?), make it return Self, and call that here -- rather than doing the extra wrap/unwrap dance.
Attachment #8879357 - Flags: review?(bbirtles)
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review155970

::: servo/components/style/properties/helpers/animated_properties.mako.rs:891
(Diff revision 2)
> +        match self {
> +            &Some(ref value) => Some(value.get_zero_value()),
> +            &None => None,
> +        }
> +    }

We can just use map().
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review155972

::: servo/components/style/properties/helpers/animated_properties.mako.rs:891
(Diff revision 2)
> +        match self {
> +            &Some(ref value) => Some(value.get_zero_value()),
> +            &None => None,
> +        }
> +    }

Oh, nvm. this returns Option<Option<Self>>?
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review155974

::: servo/components/style/properties/helpers/animated_properties.mako.rs:891
(Diff revision 2)
> +        match self {
> +            &Some(ref value) => Some(value.get_zero_value()),
> +            &None => None,
> +        }
> +    }

well, Option<Option<*inner*Self>>...
Yeah, this is all a bit odd because get_zero_value should probably return Result instead of Option, but I'm suggesting for Option we create a separate method that just returns Option<*inner*Self> and call that.
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review155952

Thanks a lot.

I updated the patch, Could you please review it again?
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review155970

> We can just use map().

I created the get_inner_zero_value instead of it.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #21)
> Comment on attachment 8879357 [details]
> Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/150652/diff/3-4/

I discussed with Brian, it is better to unwrap Option<Option<Self>> at get_zero_value of IntermediateSVGPaint than creating a new trait. So I updated the patch.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #24)
> Comment on attachment 8879357 [details]
> Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/150652/diff/4-5/

Sorry, I addressed wrong matching.
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review156568

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2991
(Diff revision 5)
> +
> +    #[inline]
> +    fn get_zero_value(&self) -> Option<Self> {
> +        Some(IntermediateSVGPaint {
> +            kind: option_try!(self.kind.get_zero_value()),
> +            fallback: option_try!(self.fallback.map(|v| v.get_zero_value())),

This isn't right. If fallback is None, this will make the whole function return None which is not what we want.

I suspect you want to just do:

  fallback: self.fallback.map(|v| v.get_zero_value().unwrap())
Attachment #8879357 - Flags: review?(bbirtles)
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review156568

> This isn't right. If fallback is None, this will make the whole function return None which is not what we want.
> 
> I suspect you want to just do:
> 
>   fallback: self.fallback.map(|v| v.get_zero_value().unwrap())

I'm sorry to bother you.. 
I updated the patch.

Thanks.
Comment on attachment 8879357 [details]
Bug 1371199 - Add get_zero_value for IntermediateSVGPaint.

https://reviewboard.mozilla.org/r/150652/#review156576
Attachment #8879357 - Flags: review?(bbirtles) → review+
Attached file Servo PR, #17467 — 

    
    

    
      
  

    
  

        Attachment #8879357 -
        Attachment is obsolete: true

    
    

    
  
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/61a32c9dfaf1
Update Stylo test expectations for reftests that use by-animation of 'fill'. r=birtles

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/61a32c9dfaf1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox56:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: