Closed Bug 1455576 Opened 6 years ago Closed 6 years ago

Have InspectorUtils::CssPropertySupportsType not rely on data in nsCSSPropList

Categories

(DevTools :: Inspector: Rules, defect)

59 Branch
defect
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(4 files)

By either porting the needed information to Servo side, or putting data into devtools directly.
Assignee: nobody → xidorn+moz
Comment on attachment 8970137 [details]
Bug 1455576 part 2 - Add a ValueInfo trait for exposing types needed by devtools.

https://reviewboard.mozilla.org/r/238938/#review244580

Looks good, thank you! Maybe nox wants to give more feedback, but this looks good otherwise.

I have one request, if it's not very annoying. Could the trait be named `SpecifiedValueInfo`? ValueInfo is very generic I'd say.

::: servo/components/style/properties/helpers.mako.rs:750
(Diff revision 2)
>              })
>          }
>  
> +        <%
> +            sub_properties_for_value_info = shorthand.sub_properties
> +            if shorthand.name == "border":

Should we special-case `all` too?

::: servo/components/style/properties/properties.mako.rs:1332
(Diff revision 2)
> +            % for prop in data.shorthands_except_all():
> +            ShorthandId::${prop.camel_case} =>
> +                <shorthands::${prop.ident}::Longhands as ValueInfo>::SUPPORTED_TYPES,
> +            % endfor
> +            // 'all' accepts no value other than CSS-wide keywords
> +            ShorthandId::All => 0,

Oh, I guess you special-case it here.

::: servo/components/style/values/specified/align.rs:235
(Diff revision 2)
>  }
>  
>  /// Value for the `align-content` property.
>  ///
>  /// <https://drafts.csswg.org/css-align/#propdef-align-content>
> -#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue, ToCss)]
> +#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue, ToCss, ValueInfo)]

This is going to be tricky because some properties support some AlignFlags keywords but not others...

But for now this is fine.

::: servo/components/style_derive/value_info.rs:37
(Diff revision 2)
> +                }
> +            }
> +            Data::Struct(ref s) => {
> +                derive_struct_fields(&s.fields, &mut types_value)
> +            }
> +            _ => unreachable!("unsupported type"),

Probably worth printing the data type here, or enumerating them.
Attachment #8970137 - Flags: review?(emilio) → review+
Comment on attachment 8970137 [details]
Bug 1455576 part 2 - Add a ValueInfo trait for exposing types needed by devtools.

https://reviewboard.mozilla.org/r/238938/#review244600

::: servo/components/style/properties/properties.mako.rs:1125
(Diff revision 2)
>      }
> +
> +    /// The supported types of this property. The return value should be
> +    /// style_traits::CssType when it can become a bitflags type.
> +    fn supported_types(&self) -> u8 {
> +        match *self {

Maybe this could be better on `NonCustomPropertyId` (to handle both longhands and shorthands) and using indexing, though I _hope_ rustc manages to generate a static table out of this match expression.
Comment on attachment 8970138 [details]
Bug 1455576 part 3 - Use Servo side data to back InspectorUtils::CssPropertySupportsType.

https://reviewboard.mozilla.org/r/238940/#review244598

::: servo/ports/geckolib/glue.rs:988
(Diff revision 2)
> +    *found = false;
> +    let prop_name = prop_name.as_ref().unwrap().as_str_unchecked();
> +    let id = match PropertyId::parse(prop_name) {
> +        Ok(id) => id,
> +        Err(_) => return false,
> +    };

This should check enabled_for_all_content and return false here in order to preserve the behavior.

Maybe it could share the codepath with IsShorthand? No big deal though.

::: servo/ports/geckolib/glue.rs:999
(Diff revision 2)
> +    // caught by the property-db test anyway.)
> +    let ty = match ty {
> +        1 => CssType::COLOR,
> +        2 => CssType::GRADIENT,
> +        3 => CssType::TIMING_FUNCTION,
> +        _ => unreachable!("unknown CSS type"),

print the number maybe?
Attachment #8970138 - Flags: review?(emilio) → review+
Comment on attachment 8970139 [details]
Bug 1455576 part 4 - Remove CSS_PROPERTY_PARSE_FUNCTION and CSS_PROPERTY_VALUE_PARSER_FUNCTION flags.

https://reviewboard.mozilla.org/r/238942/#review244604

Nice :)
Attachment #8970139 - Flags: review?(emilio) → review+
Comment on attachment 8970137 [details]
Bug 1455576 part 2 - Add a ValueInfo trait for exposing types needed by devtools.

https://reviewboard.mozilla.org/r/238938/#review244580

> Should we special-case `all` too?

`all` is not handled here. It has no `Longhands` type at all. So it doesn't apply.
Blocks: 1434130
Blocks: 1456364
Attachment #8970136 - Flags: review?(pbrosset) → review?(ttromey)
Comment on attachment 8970136 [details]
Bug 1455576 part 1 - Shrink the list of CSS_TYPES in devtools to only those being used.

https://reviewboard.mozilla.org/r/238936/#review245114

Thanks for the patch.  I love that it removes dead code.

I think there is one issue, which is why I am blanking the review.

::: dom/chrome-webidl/InspectorUtils.webidl:44
(Diff revision 2)
> -  const unsigned long TYPE_LENGTH = 0;
> -  const unsigned long TYPE_PERCENTAGE = 1;
> -  const unsigned long TYPE_COLOR = 2;
> +  const unsigned long TYPE_COLOR = 1;
> +  const unsigned long TYPE_GRADIENT = 2;
> +  const unsigned long TYPE_TIMING_FUNCTION = 3;
> -  const unsigned long TYPE_URL = 3;

I think this renumbering probably introduces a bug.

The issue is that the property database can be sent over from the server -- but if you use a new (post-patch) firefox to debug an older firefox, the numbering will change.

It would have been better to also send over the type constants from the server, but I think this wasn't done (and indeed I think we just didn't consider it at all).

I think it would be fine to either not renumber the constants (and add a comment explaining why), or arrange for the values to be sent via the CSS properties actor.
Attachment #8970136 - Flags: review?(ttromey)
I guess I'll just avoid renumbering it for now.

Also I don't think the problem is on renumbering constants in InspectorUtils, because property database doesn't record that. It records values in CSS_TYPES mapping. We should just avoid renumbering that.
Comment on attachment 8970136 [details]
Bug 1455576 part 1 - Shrink the list of CSS_TYPES in devtools to only those being used.

https://reviewboard.mozilla.org/r/238936/#review245340

Thanks for the update.  I agree with your reasoning and I think this patch is correct.
Attachment #8970136 - Flags: review?(ttromey) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8520ed7853e6
part 1 - Shrink the list of CSS_TYPES in devtools to only those being used. r=tromey
https://hg.mozilla.org/integration/autoland/rev/bf94e4ebba66
part 2 - Add a ValueInfo trait for exposing types needed by devtools. r=emilio
https://hg.mozilla.org/integration/autoland/rev/00797e05e96d
part 3 - Use Servo side data to back InspectorUtils::CssPropertySupportsType. r=emilio
https://hg.mozilla.org/integration/autoland/rev/d9667fd69b46
part 4 - Remove CSS_PROPERTY_PARSE_FUNCTION and CSS_PROPERTY_VALUE_PARSER_FUNCTION flags. r=emilio
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.