1341761 - stylo: need -moz-element support

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Probably needs servo-side parser changes.

Comment 1

[Bobby Holley (:bholley)]

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!

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Sure, I'll work on these.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1341761 - stylo: Add -moz-element support

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

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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 :)

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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`?

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1341761 - Convert nsStyleImage::mElementId to use nsIAtom

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

Comment 9

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8854487 [details]
Bug 1341761 - stylo: Add -moz-element support

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

Comment 10

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Converted mElementId to use nsIAtom and addressed the comments

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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();`.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 13

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8855409 [details]
Bug 1341761 - Convert nsStyleImage::mElementId to use nsIAtom

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

Comment 14

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8854487 [details]
Bug 1341761 - stylo: Add -moz-element support

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

Comment 15

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8855409 [details]
Bug 1341761 - Convert nsStyleImage::mElementId to use nsIAtom

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

Comment 16

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8854487 [details]
Bug 1341761 - stylo: Add -moz-element support

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

Comment 17

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1341761 - stylo: Update test expectations for -moz-element

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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> if (RefPtr<nsIAtom> atom = aElementId) {

Except please use nsCOMPtr, not RefPtr.  Smaller codesize.

Comment 20

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8855409 [details]
Bug 1341761 - Convert nsStyleImage::mElementId to use nsIAtom

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

Comment 21

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8854487 [details]
Bug 1341761 - stylo: Add -moz-element support

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

Comment 22

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8856104 [details]
Bug 1341761 - stylo: Update test expectations for -moz-element

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

Comment 23

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks changed it to nsCOMPtr and removed the Servo part.
Servo part: https://github.com/servo/servo/pull/16319

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8856104 [details]
Bug 1341761 - stylo: Update test expectations for -moz-element

https://reviewboard.mozilla.org/r/128052/#review130988

Comment 25

[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 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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2275f5298ad7
https://hg.mozilla.org/mozilla-central/rev/d547f588f461
https://hg.mozilla.org/mozilla-central/rev/0e7cd0312ecf