Better debugging for a couple things.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

No description provided.
Comment on attachment 8986758 [details]
Bug 1470145: Better debugging for media-query related code and ua-cache.

https://reviewboard.mozilla.org/r/252062/#review258518

::: servo/components/style/stylist.rs:113
(Diff revision 1)
>          let mut new_data = UserAgentCascadeData {
>              cascade_data: CascadeData::new(),
>              precomputed_pseudo_element_decls: PrecomputedPseudoElementDeclarations::default(),
>          };
>  
> +        debug!("> Picking the slow path", device);

This argument is gone locally.
Comment on attachment 8986757 [details]
Bug 1470145: Better debugging for stylesheets and URLs.

https://reviewboard.mozilla.org/r/252060/#review258632

::: servo/components/style/stylesheets/mod.rs:77
(Diff revision 1)
> +    #[inline]
>      pub fn is_chrome(&self) -> bool {
> -        self.mIsChrome
> +        self.0.mIsChrome
> +    }
> +
> +    /// Create a reference to this `UrlExtraData` from a reference to pointer.

You should copy the requirement of the pointer from `RefPtr::from_ptr_ref` to here as well, otherwise it might not be clear what the unsafe is for.

::: servo/components/style/stylesheets/mod.rs:79
(Diff revision 1)
> -        self.mIsChrome
> +        self.0.mIsChrome
> +    }
> +
> +    /// Create a reference to this `UrlExtraData` from a reference to pointer.
> +    #[inline]
> +    pub unsafe fn from_ptr_ref(ptr: &*mut ::gecko_bindings::structs::URLExtraData) -> &Self {

Also it seems you can just remove `RefPtr::from_ptr_ref` since every use of that seems to be for `URLExtraData`.

::: servo/components/style/stylesheets/mod.rs:105
(Diff revision 1)
> +
> +        formatter.debug_struct("URLExtraData")
> +            .field("is_chrome", &self.is_chrome())
> +            .field("base", &DebugURI(self.0.mBaseURI.raw::<structs::nsIURI>()))
> +            .field("referrer", &DebugURI(self.0.mReferrer.raw::<structs::nsIURI>()))
> +            .finish()

We may at some point want to dump principal as well, but I guess it's fine to leave it alone for now. It would be a bit complicated itself.

::: servo/ports/geckolib/glue.rs:2514
(Diff revision 1)
>      data: *mut URLExtraData,
>  ) -> bool {
>      let value = value.as_ref().unwrap().as_str_unchecked();
>      let mut input = ParserInput::new(&value);
>      let mut parser = Parser::new(&mut input);
> -    let url_data = RefPtr::from_ptr_ref(&data);
> +    let url_data = UrlExtraData::from_ptr_ref(&data);

Maybe have a separate commit to make this and all the functions you touch in this file in this commit unsafe themselves.
Attachment #8986757 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8986758 [details]
Bug 1470145: Better debugging for media-query related code and ua-cache.

https://reviewboard.mozilla.org/r/252062/#review258634

::: servo/components/style/gecko/media_queries.rs:83
(Diff revision 1)
> +                &mut doc_uri,
> +            )
> +        };
> +
> +        f.debug_struct("Device")
> +            .field("url", &doc_uri)

`document_url` is probably better. It feels weird why `Device` would have a `url`.

::: servo/components/style_derive/to_css.rs:235
(Diff revision 1)
>  #[darling(attributes(css), default)]
>  #[derive(Default, FromVariant)]
>  pub struct CssVariantAttrs {
>      pub function: Option<Override<String>>,
> +    // Here because structs variants are also their whole type definition.
> +    pub derive_debug: bool,

??? Where is this used?
Attachment #8986758 - Flags: review?(xidorn+moz)
Comment on attachment 8986758 [details]
Bug 1470145: Better debugging for media-query related code and ua-cache.

https://reviewboard.mozilla.org/r/252062/#review258640

::: servo/components/style/gecko/media_queries.rs:83
(Diff revision 1)
> +                &mut doc_uri,
> +            )
> +        };
> +
> +        f.debug_struct("Device")
> +            .field("url", &doc_uri)

Sure, I'll rename.

::: servo/components/style_derive/to_css.rs:235
(Diff revision 1)
>  #[darling(attributes(css), default)]
>  #[derive(Default, FromVariant)]
>  pub struct CssVariantAttrs {
>      pub function: Option<Override<String>>,
> +    // Here because structs variants are also their whole type definition.
> +    pub derive_debug: bool,

This is used in here:

  https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/servo/components/style_derive/to_css.rs#55
  
We need to have it because for `struct`s we parse the `#[css]` block both as `CssInputAttrs` and `CssVariantAttrs`, so otherwise parsing it as `CssVariantAttrs` would panic. It's the same reason we need `comma` in `CssInputAttrs`, for example.
Comment on attachment 8986758 [details]
Bug 1470145: Better debugging for media-query related code and ua-cache.

re-requesting per the last comment (assuming that and the `url` name were the only concern).
Attachment #8986758 - Flags: review?(xidorn+moz)
Comment on attachment 8986758 [details]
Bug 1470145: Better debugging for media-query related code and ua-cache.

https://reviewboard.mozilla.org/r/252062/#review258646
Attachment #8986758 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> ::: servo/ports/geckolib/glue.rs:2514
> (Diff revision 1)
> >      data: *mut URLExtraData,
> >  ) -> bool {
> >      let value = value.as_ref().unwrap().as_str_unchecked();
> >      let mut input = ParserInput::new(&value);
> >      let mut parser = Parser::new(&mut input);
> > -    let url_data = RefPtr::from_ptr_ref(&data);
> > +    let url_data = UrlExtraData::from_ptr_ref(&data);
> 
> Maybe have a separate commit to make this and all the functions you touch in
> this file in this commit unsafe themselves.

I didn't do this because I don't think it's particularly worth the time splitting the patch for these three functions.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1eb2582c7d2
Better debugging for stylesheets and URLs. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/1440edb27dc4
Better debugging for media-query related code and ua-cache. r=xidorn
Nice catches for the rest though, thanks for your reviews :)
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/a1eb2582c7d2
https://hg.mozilla.org/mozilla-central/rev/1440edb27dc4
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.