test_value_computation.html fail when turning on MASK_AS_SHORTHAND

RESOLVED FIXED in Firefox 50

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: cjku, Assigned: cjku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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"
(Assignee)

Comment 1

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2634576887b6
(Assignee)

Comment 2

a year ago
Created attachment 8770227 [details]
Bug 1286299 - Fix getComputedStyle value of a local URI mask-image.

Review commit: https://reviewboard.mozilla.org/r/63740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63740/
(Assignee)

Updated

a year ago
Attachment #8770227 - Flags: review?(dholbert)
(Assignee)

Comment 3

a year ago
hi dholbert,
Sorry, this is a side effect of Bug 1245499. Please help to check this patch again.
(Assignee)

Comment 4

a year ago
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
(Assignee)

Comment 6

a year ago
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+
(Assignee)

Comment 8

a year ago
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.
(Assignee)

Comment 9

a year ago
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.

Comment 10

a year ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c9ea2fa9a20
Fix getComputedStyle value of a local URI mask-image. r=dholbert

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c9ea2fa9a20
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.