Closed Bug 1286299 Opened 6 years ago Closed 5 years ago

test_value_computation.html fail when turning on MASK_AS_SHORTHAND

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

As title, two mochitest failure after enable MASK_AS_SHORTHAND.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d1bc67f7660&selectedJob=23729841

1084 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should not get initial value for 'mask:url(#mymask)' on elementn. - didn't expect "border-box ; none ; match-source ; border-box ; 0% ; 0% ; repeat ; auto auto ; add", but got it
42912 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | parse+serialize should be idempotent for '-webkit-mask: url(404.png) alpha padding-box border-box' - got "url(\"404.png\") repeat 0% 0% border-box add alpha", expected "url(\"404.png\") repeat 0% 0% add alpha"
Attachment #8770227 - Flags: review?(dholbert)
hi dholbert,
Sorry, this is a side effect of Bug 1245499. Please help to check this patch again.
The test case failure in [1] will be fix in bug 1286337
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2634576887b6
Attachment #8770227 - Flags: review?(dholbert) → review-
Comment on attachment 8770227 [details]
Bug 1286299 - Fix getComputedStyle value of a local URI mask-image.

https://reviewboard.mozilla.org/r/63740/#review60794

::: layout/style/nsComputedDOMStyle.cpp:2140
(Diff revision 1)
>      RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
>  
>      const nsStyleImage& image = aLayers.mLayers[i].mImage;
> +    if (aLayers.mLayers[i].mSourceURI &&
> +        image.GetType() == eStyleImageType_Null) {
> +      // This mask reference to a local URI, such as "mask:url(#mymask)".

This needs a bit of rewording. In particular, here are my concerns about the current comment text:
- It's a bit confusing to have the comment start out with "This mask [...]", as if it's already clear to the reader that we're dealing with a mask. (it's not clear -- this function isn't mask-specific, and the new checks you're adding aren't superficially mask-specific).
- It's not immediately clear to me what this means: "Layer::mImage is left behind".
- The last sentence ("We only use...mSourceURI to keep URI information") is ambiguous, around what "only" is referring to.  It can easily be read in two ways: "the information is *only* stored in mSourceURI" vs. "*only* this information [and no other information] is stored in mSourceURI". I think you mean the former, but that's not obvious.

So, my suggested changes:
 (1) Please start this comment off with something that introduces the fact that we now know we're dealing with a mask, e.g.:
 "This is how we represent a 'mask-image' reference for a local URI, such as [...etc]".
 
 (2) Please clarify what you mean about Layer::mImage being "left behind".
 
 (3) Also, please make it clearer why you're bothering to mention Layer::mImage in the first place.  (There's no code here that obviously uses Layer::mImage.  I suspect SetValueToStyleImage() might, and you're meaning to say that call won't work in this case, right? Worth mentioning that function by name, explicitly.)

 (3) Please reword the last sentence to clarify the ambiguity I mentioned above. Possible replacement: "For local references, we only store the URI in one place -- on Layer::mSourceURI."  Or alternately, you could just say "Fortunately, we don't need Layer::mImage -- we can just grab the local reference URI from Layer::mSourceURI."
Summary: test_value_computation.html/test_value_storage.html fail with MASK_AS_SHORTHAND turning on → test_value_computation.html fail when turning on MASK_AS_SHORTHAND
Comment on attachment 8770227 [details]
Bug 1286299 - Fix getComputedStyle value of a local URI mask-image.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63740/diff/1-2/
Attachment #8770227 - Flags: review- → review?(dholbert)
Comment on attachment 8770227 [details]
Bug 1286299 - Fix getComputedStyle value of a local URI mask-image.

https://reviewboard.mozilla.org/r/63740/#review61052

Thanks for clarifying the code-comment here! This is much clearer now.

r=me, with a few last nits:

First, please tweak commit message:
  s/computed property value/getComputedStyle value/
  
("computed property value" isn't as clear, since we also consider the value in nsStyleStruct.h to be the "computed value". getComputedStyle is its own special thing, somewhat distinct from the "computed value" that we use elsewhere.)

::: layout/style/nsComputedDOMStyle.cpp:2138
(Diff revision 2)
>  
>    for (uint32_t i = 0, i_end = aLayers.mImageCount; i < i_end; ++i) {
>      RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
>  
>      const nsStyleImage& image = aLayers.mLayers[i].mImage;
> +    // Layer::mImage::GetType() returns eStyleImageType_Null in two conditions

Please add a ":" at the end of this first line (after "conditions").

::: layout/style/nsComputedDOMStyle.cpp:2140
(Diff revision 2)
>      RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
>  
>      const nsStyleImage& image = aLayers.mLayers[i].mImage;
> +    // Layer::mImage::GetType() returns eStyleImageType_Null in two conditions
> +    // 1. The value of mask-image/bg-image is 'none'.
> +    //    Since this layer does not refer to any source, Layer::mSourceURL must

mSourceURL is a typo here (should be "URI", not "URL")

::: layout/style/nsComputedDOMStyle.cpp:2146
(Diff revision 2)
> +    //    be nullptr too.
> +    // 2. This layer refers to a local resource, e.g. mask-image:url(#mymask).
> +    //    For local references, there is no need to download any external
> +    //    resource, so Layer::mImage is not used.
> +    //    Instead, we store the local URI in one place -- on Layer::mSourceURI.
> +    //    That says, you can only access local URI of a layer through

The end of this comment (the part begining with "That says") is still a bit confusing about what it's trying to say / what its purpose is.

Consider replacing with:
  Hence, we must serialize using mSourceURI (instead of SetValueToStyleImage()/mImage) in this case.
(That's a bit more directly connected to the code that follows, and I think it's really what you're trying to say?)
Attachment #8770227 - Flags: review?(dholbert) → review+
Comment on attachment 8770227 [details]
Bug 1286299 - Fix getComputedStyle value of a local URI mask-image.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63740/diff/2-3/
Attachment #8770227 - Attachment description: Bug 1286299 - Fix computed property value of a local URI mask-image. → Bug 1286299 - Fix getComputedStyle value of a local URI mask-image.
https://reviewboard.mozilla.org/r/63740/#review61052

> The end of this comment (the part begining with "That says") is still a bit confusing about what it's trying to say / what its purpose is.
> 
> Consider replacing with:
>   Hence, we must serialize using mSourceURI (instead of SetValueToStyleImage()/mImage) in this case.
> (That's a bit more directly connected to the code that follows, and I think it's really what you're trying to say?)

Yes, it is.
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c9ea2fa9a20
Fix getComputedStyle value of a local URI mask-image. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/7c9ea2fa9a20
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.