1296477 - stylo: Implement content: {counter(..), counters(..), url(..), attr(..)}

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Emilio Cobos Álvarez [:emilio]]

These are missing values that we're missing for this properties in stylo:

 * counter(..), counters(..): Supported in Servo, missing glue.
 * url(..): Not supported in Servo, will presumably take advantage from Cameron's background-image support patches.
 * attr(..): Not supported in Servo, but should be easy to parse and support.

Comment 1

[Manish Goregaokar [:manishearth]]

I'm having some trouble with the URL value. Firstly, I'm not sure if I should be setting it as uncacheable -- nsRuleNode.cpp does not, but going by the comments image requests in reset structs should be set as uncacheable.

Secondly, given the way it's being done in the patch, I'm getting a panic that `Resolve()` hasn't been called yet. I suspect I'll need to do something with image tracking on the pres context, but I'm not sure.

heycam, any idea what I should be doing here?

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review commit: https://reviewboard.mozilla.org/r/119442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119442/

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119442/diff/1-2/

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119442/diff/2-3/

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Manish Goregaokar [:manishearth] from comment #1)
> I'm having some trouble with the URL value. Firstly, I'm not sure if I
> should be setting it as uncacheable -- nsRuleNode.cpp does not, but going by
> the comments image requests in reset structs should be set as uncacheable.

I think it should be fine to not call SetUncacheable.  Which comments are you referring to?

> Secondly, given the way it's being done in the patch, I'm getting a panic
> that `Resolve()` hasn't been called yet. I suspect I'll need to do something
> with image tracking on the pres context, but I'm not sure.

You'll need to modify nsStyleContent::FinishStyle to call Resolve() on the nsStyleImageRequest objects that are stored in the nsStyleContent.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #5)
> I think it should be fine to not call SetUncacheable.  Which comments are
> you referring to?

Oh, if you are referring to |cacheable| on the Servo side, then from the comments Emilio added there it sounds like it's not needed any more.  Try without it.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

https://reviewboard.mozilla.org/r/119442/#review121330

This generally looks OK.  A few things need fixing up.

::: layout/style/ServoBindings.h:226
(Diff revision 3)
> -                            const uint8_t* url_bytes,
> +                            ServoBundledURI uri);
> -                            uint32_t url_length,
> -                            ThreadSafeURIHolder* base_uri,
> -                            ThreadSafeURIHolder* referrer,
> -                            ThreadSafePrincipalHolder* principal);

The changes to other functions to use ServoBundledURI really should be in a separate commit.

::: layout/style/ServoBindings.h:248
(Diff revision 3)
> +void Gecko_SetContentDataImage(nsStyleContentData* contentData, ServoBundledURI uri);
> +void Gecko_SetContentDataArray(nsStyleContentData* contentData, nsStyleContentType type, uint32_t len);

Nit: Should "contentData" be "content_data", since we're trying to cause bindgen to generate Rust-friendly identifiers here?

::: layout/style/ServoBindings.h:281
(Diff revision 3)
>  // Clear the mContents field in nsStyleContent. This is needed to run the
>  // destructors, otherwise we'd leak the images (though we still don't support
>  // those), strings, and whatnot.

This comment needs updating.

::: layout/style/ServoBindings.h:360
(Diff revision 3)
>  void Gecko_CSSValue_SetKeyword(nsCSSValueBorrowedMut css_value, nsCSSKeyword keyword);
>  void Gecko_CSSValue_SetPercentage(nsCSSValueBorrowedMut css_value, float percent);
>  void Gecko_CSSValue_SetAngle(nsCSSValueBorrowedMut css_value, float radians);
>  void Gecko_CSSValue_SetCalc(nsCSSValueBorrowedMut css_value, nsStyleCoord::CalcValue calc);
>  void Gecko_CSSValue_SetFunction(nsCSSValueBorrowedMut css_value, int32_t len);
> -void Gecko_CSSValue_SetString(nsCSSValueBorrowedMut css_value, const nsString string);
> +void Gecko_CSSValue_SetString(nsCSSValueBorrowedMut css_value, const uint8_t* aString, uint32_t aLength);

Nit: "string" and "length".

::: layout/style/ServoBindings.cpp:926
(Diff revision 3)
>  }
>  
> +void
> +Gecko_SetContentDataImage(nsStyleContentData* aContent, ServoBundledURI aURI)
> +{
> +  aContent->SetImageRequest(CreateStyleImageRequest(nsStyleImageRequest::Mode::Track, aURI));

Nit: please keep to 80 columns.

::: layout/style/ServoBindings.cpp:930
(Diff revision 3)
> +{
> +  aContent->SetImageRequest(CreateStyleImageRequest(nsStyleImageRequest::Mode::Track, aURI));
> +}
> +
> +void
> +Gecko_SetContentDataArray(nsStyleContentData* aContent, nsStyleContentType aType, uint32_t aLen)

Nit: and here.

::: layout/style/ServoBindings.cpp:933
(Diff revision 3)
> +
> +void
> +Gecko_SetContentDataArray(nsStyleContentData* aContent, nsStyleContentType aType, uint32_t aLen)
> +{
> +  nsCSSValue::Array* arr = nsCSSValue::Array::Create(aLen);
> +  aContent->SetCounters(aType, arr);

Could you add an assertion in nsStyleContentData::SetCounters that the array has length 2 or 3?

::: servo/components/style/gecko/conversions.rs:121
(Diff revision 3)
> +                } else {
> +                    error!("Skipping url without extra data")
> +                }

Doesn't for_ffi() only return None when as_slice_components fails, not when there is no UrlExtraData available?  (Every SpecifiedUrl value has a valid UrlExtraData.)

The old code used the value returned from as_slice_components even when it was wrapped in Err, but for_ffi doesn't.  What's the implication of that?  Should we be setting some value if we can't set the url() value here?

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:107
(Diff revision 3)
> +    /// Set to a string value
> +    pub fn set_string(&mut self, s: &str) {
> +        unsafe { bindings::Gecko_CSSValue_SetString(self, s.as_ptr(), s.len() as u32) }
> +    }
> +
> +    /// Set to a string value

s/string/url/

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:111
(Diff revision 3)
> +        } else {
> +            error!("Skipping url without extra data")
> +        }

Same here.  The error message seems wrong, and should we be setting some value if we can't set the url() value?

::: servo/components/style/properties/gecko.mako.rs:3151
(Diff revision 3)
> -                                     extra_data.base.get(),
> -                                     extra_data.referrer.get(),
> -                                     extra_data.principal.get());
> +                } else {
> +                    error!("Skipping url without extra data")
> +                }

Same here.

::: servo/components/style/properties/gecko.mako.rs:3192
(Diff revision 3)
>      <% impl_app_units("column_rule_width", "mColumnRuleWidth", need_clone=True,
>                        round_to_pixels=True) %>
>  </%self:impl_trait>
>  
>  <%self:impl_trait style_struct_name="Counters"
> -                  skip_longhands="content">
> +                  skip_longhands="content counter-increment counter-reset">

Support for counter-increment and counter-reset should really be in a separate commit.  (Keeping logically separate changes in separate commits helps reviewers and code archaeologists alike!)

::: servo/components/style/properties/gecko.mako.rs:3241
(Diff revision 3)
> +                        ContentItem::Attr(value) => {
> +                            self.gecko.mContents[i].mType = eStyleContentType_Attr;
> +                            unsafe {
> +                                // NB: we share allocators, so doing this is fine.
> +                                *self.gecko.mContents[i].mContent.mString.as_mut() =
> +                                    as_utf16_and_forget(&value);
> +                            }
> +                        }

Maybe share this code with the previous arm?  (The amount of actual code shared is small, but visually, with six lines, it might be good for the reader.)

::: servo/components/style/properties/gecko.mako.rs:3265
(Diff revision 3)
> -                        ContentItem::Counters(..)
> -                            => self.gecko.mContents[i].mType = eStyleContentType_Uninitialized,
> +                            unsafe {
> +                                bindings::Gecko_SetContentDataArray(&mut self.gecko.mContents[i], eStyleContentType_Counter, 2)
> +                            }
> +                            let mut array = unsafe { &mut **self.gecko.mContents[i].mContent.mCounters.as_mut() };
> +                            array[0].set_string(&name);
> +                            array[1].set_string(&style.to_css_string());

Can you add a comment in here (or somewhere) that once Servo supports <custom-ident> values for list-style-type (and types in counter() / counters()) that we might need to add some extra handling here.

::: servo/components/style/properties/gecko.mako.rs:3279
(Diff revision 3)
> +                            } else {
> +                                error!("Skipping url without extra data")
> +                            }

Same here.

::: servo/components/style/properties/longhand/counters.mako.rs:89
(Diff revision 3)
>                      ContentItem::NoOpenQuote => dest.write_str("no-open-quote"),
>                      ContentItem::NoCloseQuote => dest.write_str("no-close-quote"),
>  
>                      % if product == "gecko":
>                          ContentItem::MozAltContent => dest.write_str("-moz-alt-content"),
> +                        ContentItem::Attr(ref attr) => write!(dest, "attr({})", attr),

Is writing attr unescaped correct?  I'm having trouble finding an up-to-date spec that defines precisely what is inside attr(), but if we assume that it's something like [ ident? '|' ident ] (which is what nsCSSParser::ParseAttr accepts), then I think the idents need to be CSS-escaped.

::: servo/components/style/properties/longhand/counters.mako.rs:176
(Diff revision 3)
> +                            // Syntax is `[namespace? `|`]? ident`
> +                            // TODO we should be checking that this is a valid namespace

Not only this, but nsCSSParser::ParseAttr encodes the namespace prefix using its mNameSpaceMap, so that the resulting string that is stored must look something like "4|href".

::: servo/components/style/properties/longhand/counters.mako.rs:183
(Diff revision 3)
> +                            let _ = input.try(|i| i.expect_ident()).is_ok();
> +                            let has_bar = input.try(|i| i.expect_delim('|')).is_ok();
> +                            if has_bar {
> +                                input.expect_ident()?;
> +                            }
> +                            

Nit: delete this line.

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

https://reviewboard.mozilla.org/r/119442/#review121380

Comment 9

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1296477 - Part 1: stylo: Use ServoBundledURI everywhere else, fix from_ffi to handle the error case;

Review commit: https://reviewboard.mozilla.org/r/119526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119526/

Comment 10

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1296477 - Part 3: stylo: Support counter-increment and counter-reset;

Review commit: https://reviewboard.mozilla.org/r/119528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119528/

Comment 11

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1296477 - Part 4: stylo: Update test expectations;

Review commit: https://reviewboard.mozilla.org/r/119530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119530/

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119442/diff/3-4/

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8846484 [details]
Bug 1296477 - Part 1: stylo: Use ServoBundledURI everywhere else, fix from_ffi to handle the error case;

https://reviewboard.mozilla.org/r/119526/#review121398

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

https://reviewboard.mozilla.org/r/119442/#review121402

::: layout/style/ServoBindings.cpp:926
(Diff revision 4)
> +  auto req = CreateStyleImageRequest(nsStyleImageRequest::Mode::Track, aURI);
> +  aContent->SetImageRequest(Move(req));

I think static analysis checks will prevent you from creating a local variable of type already_AddRefed (see the MOZ_NON_AUTOABLE annotation on it).  Instead I would write this as:

  RefPtr<...> req = ...;
  ...SetImageRequest(req.forget());

::: layout/style/ServoBindings.cpp:934
(Diff revision 4)
> +
> +void
> +Gecko_SetContentDataArray(nsStyleContentData* aContent,
> +                          nsStyleContentType aType, uint32_t aLen)
> +{
> +  MOZ_ASSERT(aLen == 2 || aLen == 3);

I think the assertion would be better down in nsStyleContentData::SetCounters.

::: layout/style/nsStyleStruct.cpp:3767
(Diff revision 4)
> +  for (auto iter = mContents.begin(); iter != mContents.end(); iter++) {
> +    iter->Resolve(aPresContext);
> +  }

Feel free to use ranged-for syntax here:

  for (nsStyleContentData& data : mContents) {
    data.Resolve(aPresContext);
  }

::: servo/components/style/properties/gecko.mako.rs:3244
(Diff revision 4)
> -                        ContentItem::Counters(..)
> -                            => self.gecko.mContents[i].mType = eStyleContentType_Uninitialized,
> +                            unsafe {
> +                                bindings::Gecko_SetContentDataArray(&mut self.gecko.mContents[i], eStyleContentType_Counter, 2)
> +                            }
> +                            let mut array = unsafe { &mut **self.gecko.mContents[i].mContent.mCounters.as_mut() };
> +                            array[0].set_string(&name);
> +                            // When we support <custom-ident> values for list-style-image this will need to be updated

list-style-type

::: servo/components/style/properties/gecko.mako.rs:3254
(Diff revision 4)
> +                                bindings::Gecko_SetContentDataArray(&mut self.gecko.mContents[i], eStyleContentType_Counters, 3)
> +                            }
> +                            let mut array = unsafe { &mut **self.gecko.mContents[i].mContent.mCounters.as_mut() };
> +                            array[0].set_string(&name);
> +                            array[1].set_string(&sep);
> +                            // When we support <custom-ident> values for list-style-image this will need to be updated

list-style-type

::: servo/components/style/properties/longhand/counters.mako.rs:175
(Diff revision 4)
> +                            let pos = input.position();
> +                            // Syntax is `[namespace? `|`]? ident`
> +                            // TODO we should be checking that this is a valid namespace
> +                            // and encoding it as a namespace number from the map
> +                            let _ = input.try(|i| i.expect_ident()).is_ok();
> +                            let has_bar = input.try(|i| i.expect_delim('|')).is_ok();
> +                            if has_bar {
> +                                input.expect_ident()?;
> +                            }
> +                            Ok(ContentItem::Attr(input.slice_from(pos).to_owned()))

Please do file the followup bug to handle the namespace map stuff.

Another thing is that nsCSSParser does not allow spaces between any of the tokens in the `[ ident? '|' ]? ident`.  Can you do the same here?

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8846485 [details]
Bug 1296477 - Part 3: stylo: Support counter-increment and counter-reset;

https://reviewboard.mozilla.org/r/119528/#review121406

::: layout/style/ServoBindings.h:281
(Diff revision 1)
> -// Clear the mContents field in nsStyleContent. This is needed to run the
> -// destructors, otherwise we'd leak the images (though we still don't support
> +// Clear the mContents, mCounterIncrements, or mCounterResets field in nsStyleContent. This is
> +// needed to run the destructors, otherwise we'd leak the images (though we still don't support
>  // those), strings, and whatnot.

The part about not supporting images can be removed, too.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8846486 [details]
Bug 1296477 - Part 4: stylo: Update test expectations;

https://reviewboard.mozilla.org/r/119530/#review121410

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119442/diff/4-5/

Comment 18

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846485 [details]
Bug 1296477 - Part 3: stylo: Support counter-increment and counter-reset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119528/diff/1-2/

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846486 [details]
Bug 1296477 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119530/diff/1-2/

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846484 [details]
Bug 1296477 - Part 1: stylo: Use ServoBundledURI everywhere else, fix from_ffi to handle the error case;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119526/diff/1-2/

Comment 22

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119442/diff/5-6/

Comment 23

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846485 [details]
Bug 1296477 - Part 3: stylo: Support counter-increment and counter-reset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119528/diff/2-3/

Comment 24

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846486 [details]
Bug 1296477 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119530/diff/2-3/

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8846486 [details]
Bug 1296477 - Part 4: stylo: Update test expectations;

https://reviewboard.mozilla.org/r/119530/#review121876

::: layout/tables/crashtests/crashtests.list:139
(Diff revision 3)
>  load 573354-1.xhtml
>  load 576890-1.html
>  load 576890-2.html
>  load 576890-3.html
>  load 580481-1.xhtml
> -asserts(1) asserts-if(stylo,0) load 595758-1.xhtml # Bug 714667
> +asserts(1) asserts-if(stylo,1) load 595758-1.xhtml # Bug 714667

I guess you can remove the `asserts-if` now, given its identical to the general case.

Comment 26

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846484 [details]
Bug 1296477 - Part 1: stylo: Use ServoBundledURI everywhere else, fix from_ffi to handle the error case;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119526/diff/2-3/

Comment 27

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119442/diff/6-7/

Comment 28

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846485 [details]
Bug 1296477 - Part 3: stylo: Support counter-increment and counter-reset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119528/diff/3-4/

Comment 29

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846486 [details]
Bug 1296477 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119530/diff/3-4/

Comment 30

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/15939

Comment 31

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3652c33b4f90 -d fe16f2642746: rebasing 382002:3652c33b4f90 "Bug 1296477 - Part 1: stylo: Use ServoBundledURI everywhere else, fix from_ffi to handle the error case; r=heycam"
rebasing 382003:95b0b6e184fb "Bug 1296477 - Part 2: stylo: Implement remaining `content` values; r=heycam"
merging layout/style/nsCSSValue.cpp
merging layout/style/nsStyleStruct.cpp
merging layout/style/nsStyleStruct.h
rebasing 382004:020105e21503 "Bug 1296477 - Part 3: stylo: Support counter-increment and counter-reset; r=heycam"
merging layout/style/test/stylo-failures.md
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 32

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846484 [details]
Bug 1296477 - Part 1: stylo: Use ServoBundledURI everywhere else, fix from_ffi to handle the error case;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119526/diff/3-4/

Comment 33

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846402 [details]
Bug 1296477 - Part 2: stylo: Implement remaining `content` values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119442/diff/7-8/

Comment 34

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846485 [details]
Bug 1296477 - Part 3: stylo: Support counter-increment and counter-reset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119528/diff/4-5/

Comment 35

[Manish Goregaokar [:manishearth]]

Comment on attachment 8846486 [details]
Bug 1296477 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119530/diff/4-5/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9028aa08ef9b
Part 1: stylo: Use ServoBundledURI everywhere else, fix from_ffi to handle the error case; r=heycam
https://hg.mozilla.org/integration/autoland/rev/66dd12f55a93
Part 2: stylo: Implement remaining `content` values; r=heycam
https://hg.mozilla.org/integration/autoland/rev/77b479d7b218
Part 3: stylo: Support counter-increment and counter-reset; r=heycam
https://hg.mozilla.org/integration/autoland/rev/7130125870e0
Part 4: stylo: Update test expectations; r=heycam

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9028aa08ef9b
https://hg.mozilla.org/mozilla-central/rev/66dd12f55a93
https://hg.mozilla.org/mozilla-central/rev/77b479d7b218
https://hg.mozilla.org/mozilla-central/rev/7130125870e0