1470145 - Better debugging for a couple things.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1470145: Better debugging for stylesheets and URLs.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1470145: Better debugging for media-query related code and ua-cache.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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 4

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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 7

[Emilio Cobos Álvarez (:emilio)]

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

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

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 10

[Pulsebot]

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Nice catches for the rest though, thanks for your reviews :)

Comment 12

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a1eb2582c7d2
https://hg.mozilla.org/mozilla-central/rev/1440edb27dc4