Closed
Bug 1296477
Opened 8 years ago
Closed 7 years ago
stylo: Implement content: {counter(..), counters(..), url(..), attr(..)}
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: emilio, Assigned: manishearth)
References
(Depends on 1 open bug)
Details
Attachments
(4 files)
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.
Updated•7 years ago
|
Blocks: stylo-style-mochitest
Updated•7 years ago
|
Assignee: nobody → manishearth
Priority: -- → P1
Assignee | ||
Comment 1•7 years ago
|
||
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?
Flags: needinfo?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
(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.
Flags: needinfo?(cam)
Comment 6•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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.
Attachment #8846402 -
Flags: review?(cam) → review-
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8846402 [details] Bug 1296477 - Part 2: stylo: Implement remaining `content` values; https://reviewboard.mozilla.org/r/119442/#review121380
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
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
Attachment #8846484 -
Flags: review?(cam) → review+
Comment 14•7 years ago
|
||
mozreview-review |
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?
Attachment #8846402 -
Flags: review?(cam) → review+
Comment 15•7 years ago
|
||
mozreview-review |
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.
Attachment #8846485 -
Flags: review?(cam) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8846486 [details] Bug 1296477 - Part 4: stylo: Update test expectations; https://reviewboard.mozilla.org/r/119530/#review121410
Attachment #8846486 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
https://github.com/servo/servo/pull/15939
Comment 31•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
Pushed by manishearth@gmail.com: 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•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•