Closed
Bug 1341761
Opened 7 years ago
Closed 7 years ago
stylo: need -moz-element support
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: canova)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
Probably needs servo-side parser changes.
Flags: needinfo?(simon.sapin)
Updated•7 years ago
|
Priority: -- → P1
Comment 1•7 years ago
|
||
Nazim, given that you finished all your other P1s so fast, I'm giving you the last two unowned P1s that I've been saving for whoever finished first. :-) Definitely let me know if you don't have time - you're already doing a lot and we're really grateful!
Assignee: nobody → canaltinova
Assignee | ||
Comment 2•7 years ago
|
||
Sure, I'll work on these.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Sorry about the delay on this. I submit a patch but still I need to test this. Xidorn, also I just saw your comment in https://github.com/servo/servo/issues/15443. I would like to try calling method directly but I don't know the exact direction here. I would be glad if you can point out :)
Flags: needinfo?(xidorn+moz)
Comment 5•7 years ago
|
||
I think I asked Manish or Emilio about whether we can enable method generating in bindgen, so that we can call C++ methods from Rust side directly... IIRC they said we probably don't want to do that (for now)? I don't recall clearly, but probably you should just add a new FFI function here.
Flags: needinfo?(xidorn+moz)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8854487 [details] Bug 1341761 - stylo: Add -moz-element support https://reviewboard.mozilla.org/r/126432/#review129236 ::: layout/style/ServoBindings.cpp:971 (Diff revision 1) > +Gecko_SetImageElement(nsStyleImage* aImage, const uint8_t* aString) { > + MOZ_ASSERT(aImage); > + aImage->SetElementId(aString); > +} It seems to me `SetElementId` accepts a `const uint16_t*`, how can you pass a `const uint8_t*` to it directly? You would need to either convert it here, or probably convert `nsStyleImage::mElementId` to use `nsIAtom`. ::: servo/components/style/values/computed/image.rs:90 (Diff revision 1) > GradientKind::Linear(_) => write!(f, "linear-gradient({:?})", grad), > GradientKind::Radial(_, _) => write!(f, "radial-gradient({:?})", grad), > } > }, > Image::ImageRect(ref image_rect) => write!(f, "{:?}", image_rect), > + Image::Element(ref selector) => write!(f, "{:?}", selector), Should be `element({:?})` I think. ::: servo/components/style/values/specified/image.rs:41 (Diff revision 1) > fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { > match *self { > Image::Url(ref url_value) => url_value.to_css(dest), > Image::Gradient(ref gradient) => gradient.to_css(dest), > Image::ImageRect(ref image_rect) => image_rect.to_css(dest), > + Image::Element(ref selector) => dest.write_str(selector), The selector is not all of this branch. It should be `"-moz-element({})"` with selector inside. ::: servo/components/style/values/specified/image.rs:71 (Diff revision 1) > Image::Url(SpecifiedUrl::for_cascade(url)) > } > + > + /// Parses an `-moz-element(# <element-id>)`. > + fn parse_element(input: &mut Parser) -> Result<String, ()> { > + if input.try(|i| i.expect_function_matching("element")).is_ok() { Should this be `-moz-element` rather than `element`?
Attachment #8854487 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8854487 [details] Bug 1341761 - stylo: Add -moz-element support https://reviewboard.mozilla.org/r/126432/#review129236 > It seems to me `SetElementId` accepts a `const uint16_t*`, how can you pass a `const uint8_t*` to it directly? > > You would need to either convert it here, or probably convert `nsStyleImage::mElementId` to use `nsIAtom`. Yeah, actually I left it that way thinking I'll change it eventually when I saw your comment in servo issue. I'll convert mElementId to use nsIAtom atom then. > Should be `element({:?})` I think. Yeah, sorry about these. I was trying something and forgot to revert afterwards.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
Converted mElementId to use nsIAtom and addressed the comments
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8855409 [details] Bug 1341761 - Convert nsStyleImage::mElementId to use nsIAtom https://reviewboard.mozilla.org/r/127248/#review130092 Please address the following issues. ::: layout/style/nsRuleNode.cpp:1379 (Diff revision 1) > break; > } > case eCSSUnit_Element: > - aResult.SetElementId(aValue.GetStringBufferValue()); > + { > + nsCOMPtr<nsIAtom> atom = NS_Atomize(aValue.GetStringBufferValue()); > + aResult.SetElementId(atom); This adds an unnecessary refcounting. You should change `SetElementId` to take `already_AddRefed<nsIAtom>` and use `atom.forget()` here. (It is also fine to keep using `nsCOMPtr<nsIAtom>` as the parameter and use `Move(atom)` here, but I believe it is considered a better approach to just use `already_AddRefed` to avoid potential unnecessary refcounting like this.) ::: layout/style/nsStyleStruct.h:432 (Diff revision 1) > nsStyleImage& operator=(const nsStyleImage& aOther); > > void SetNull(); > void SetImageRequest(already_AddRefed<nsStyleImageRequest> aImage); > void SetGradientData(nsStyleGradient* aGradient); > - void SetElementId(const char16_t* aElementId); > + void SetElementId(nsCOMPtr<nsIAtom> aElementId); As suggested above, this should probably take `already_AddRefed<nsIAtom>` instead. ::: layout/style/nsStyleStruct.h:552 (Diff revision 1) > > nsStyleImageType mType; > union { > nsStyleImageRequest* mImage; > nsStyleGradient* mGradient; > - char16_t* mElementId; > + nsCOMPtr<nsIAtom> mElementId; This could be a bit troublesome. Compilers we use do have supported unrestricted unions, but I would suggest it is better to keep using raw pointer for this right now. You would need to write special constructor and destructor for the union otherwise. See https://msdn.microsoft.com/en-us/library/5dxy4b7b.aspx#Anchor_4 ::: layout/style/nsStyleStruct.cpp:2217 (Diff revision 1) > if (mType == eStyleImageType_Gradient) { > mGradient->Release(); > } else if (mType == eStyleImageType_Image) { > NS_RELEASE(mImage); > } else if (mType == eStyleImageType_Element) { > - free(mElementId); > + mElementId->Release(); Please just use `NS_RELEASE` which would also reset the pointer itself to `nullptr`. ::: layout/style/nsStyleStruct.cpp:2267 (Diff revision 1) > if (mType != eStyleImageType_Null) { > SetNull(); > } > > if (aElementId) { > - mElementId = NS_strdup(aElementId); > + mElementId = aElementId; Once you do the suggested edit above (`already_AddRefed` and raw pointer in union), you should change this to `mElementId = aElementId.take();`.
Attachment #8855409 -
Flags: review?(xidorn+moz)
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8854487 [details] Bug 1341761 - stylo: Add -moz-element support https://reviewboard.mozilla.org/r/126432/#review130120 r=me with the refcounting issue fixed. ::: layout/style/ServoBindings.cpp:957 (Diff revision 2) > + already_AddRefed<nsIAtom> atom = already_AddRefed<nsIAtom>(aAtom); > + aImage->SetElementId(atom); It seems to me you are passing in a borrowed `nsIAtom` as ptr, no? Wrapping a unowned in `already_AddRefed` likely leads to double-free. This should be `SetElementId(do_AddRef(aAtom))`. ::: servo/components/style/values/computed/image.rs:94 (Diff revision 2) > }, > Image::ImageRect(ref image_rect) => write!(f, "{:?}", image_rect), > + Image::Element(ref selector) => { > + f.write_str("-moz-element(#")?; > + // FIXME: We should get rid of these intermediate strings. > + serialize_identifier(&*selector.to_string(), f)?; I guess we don't need to be that careful for `Debug`. ::: servo/components/style/values/specified/image.rs:80 (Diff revision 2) > + Token::IDHash(id) => { > + Ok(Atom::from(id)) > + }, This could be merged into one line, I guess.
Attachment #8854487 -
Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8855409 [details] Bug 1341761 - Convert nsStyleImage::mElementId to use nsIAtom https://reviewboard.mozilla.org/r/127248/#review130610 ::: layout/style/nsStyleStruct.cpp:2264 (Diff revision 3) > + RefPtr<nsIAtom> atom = aElementId; > if (mType != eStyleImageType_Null) { > SetNull(); > } > > - if (aElementId) { > + if (atom) { You can actually do ```c++ if (RefPtr<nsIAtom> atom = aElementId) { ``` directly to make it more compact if you like.
Attachment #8855409 -
Flags: review?(xidorn+moz) → review+
Reporter | ||
Comment 19•7 years ago
|
||
> if (RefPtr<nsIAtom> atom = aElementId) {
Except please use nsCOMPtr, not RefPtr. Smaller codesize.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
Thanks changed it to nsCOMPtr and removed the Servo part. Servo part: https://github.com/servo/servo/pull/16319
Reporter | ||
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8856104 [details] Bug 1341761 - stylo: Update test expectations for -moz-element https://reviewboard.mozilla.org/r/128052/#review130988
Attachment #8856104 -
Flags: review+
Comment 25•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 72ea69f2ff31 -d a21a26ea6947: rebasing 388373:72ea69f2ff31 "Bug 1341761 - Convert nsStyleImage::mElementId to use nsIAtom r=xidorn" merging layout/style/nsComputedDOMStyle.cpp merging layout/style/nsRuleNode.cpp merging layout/style/nsStyleStruct.cpp merging layout/style/nsStyleStruct.h warning: conflicts while merging layout/style/nsStyleStruct.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging layout/style/nsStyleStruct.h! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment 26•7 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2275f5298ad7 Convert nsStyleImage::mElementId to use nsIAtom. r=xidorn https://hg.mozilla.org/integration/autoland/rev/d547f588f461 stylo: Add -moz-element support. r=xidorn https://hg.mozilla.org/integration/autoland/rev/0e7cd0312ecf stylo: Update test expectations for -moz-element. r=bzbarsky
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2275f5298ad7 https://hg.mozilla.org/mozilla-central/rev/d547f588f461 https://hg.mozilla.org/mozilla-central/rev/0e7cd0312ecf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
status-firefox54:
affected → ---
Updated•7 years ago
|
Flags: needinfo?(simon.sapin)
You need to log in
before you can comment on or make changes to this bug.
Description
•