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)
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 | ||
Updated•6 years ago
|
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52658d4d5faf647479c33d2cf7c0abe10f802635
Comment 9•6 years ago
|
||
mozreview-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 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 10•6 years ago
|
||
mozreview-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 11•6 years ago
|
||
mozreview-review |
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 12•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8970136 -
Flags: review?(pbrosset) → review?(ttromey)
Comment 18•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 19•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•6 years ago
|
||
mozreview-review |
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+
Comment 25•6 years ago
|
||
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
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8520ed7853e6 https://hg.mozilla.org/mozilla-central/rev/bf94e4ebba66 https://hg.mozilla.org/mozilla-central/rev/00797e05e96d https://hg.mozilla.org/mozilla-central/rev/d9667fd69b46
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•