Closed
Bug 1294299
Opened 8 years ago
Closed 8 years ago
Stylo: Implement CSSStyleDeclaration for Element.style
Categories
(Core :: DOM: CSS Object Model, defect, P3)
Core
DOM: CSS Object Model
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(14 files)
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
SimonSapin
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
emilio
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
SimonSapin
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
SimonSapin
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Summary: Stylo: Implement CSSStyleDeclaration interface → Stylo: Implement CSSStyleDeclaration for Element.style
Assignee | ||
Comment 1•8 years ago
|
||
I now have patches for both sides for this bug, with majority of the new glue functions in Servo side being unimplemented!(). The implementation of the glue functions is blocked by the string manipulation crate and the corresponding implementation in Servo side (which are the two dependencies of this bug).
With out implementing them, we cannot land the patches, because they are too crashy. (It crashes locally because about:newtab accesses those unimplemented functions.)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → xidorn+moz
Assignee | ||
Comment 2•8 years ago
|
||
Simon, I'd like to discuss with you about how property names should be passed between Servo and Gecko.
Currently, Gecko starts handling property and custom property at very early stage in CSSOM: it parses property name and tries to convert it to nsCSSPropertyID. If it is a property, then call functions for properties. If it is a custom property, call functions for custom properties. And the property accessors (Element.style.xxx) calls functions for properties directly with corresponding nsCSSPropertyID.
To implement CSSStyleDeclaration, then, I think there are three options:
(1) pass a string to Servo for all cases, and let Servo do everything else.
Pros: changes in Servo side is minimal, so maximized code sharing between Gecko and Servo
Cons: need a UTF16-to-UTF8 conversion for all property names, and Servo is currently using string comparison, which is slow
(2) pass nsCSSPropertyID to Servo, then diverge property matching for Servo and Gecko
Pros: probably minimal Gecko side change, and reasonably fast because no string comparison anymore for properties
Cons: nontrivial change to Servo for adding functions to handle nsCSSPropertyID, and that also decreases the code sharing
(3) have Servo use atoms for properties, and have Gecko add CSS property names into predefined atoms
Pros: probably also maximize code sharing, and remove the string comparison work
Cons: some nontrivial change to Servo as well? and also diverge some code paths in Gecko side
Having written this down, I think using atom sounds like the best solution in long term. Before that, I was thinking about passing nsCSSPropertyID, but it seems to quickly become more complicated than I thought.
What do you think?
If we agree that we should go with atoms, I'll start adding static atoms for CSS properties in Gecko (and expose them to Servo via gecko_string_cache). What else do you need for using atoms for property names in Servo code?
Also we need to determine what would the atom string be for custom properties. If we have the string include the "--" prefix, we can have a unified path in both Gecko and Servo side. Otherwise, Gecko would need to distinguish between property and custom property before atomize it, and we also need different binding functions (or functions with an additional parameter) to handle them separately. I guess it needs to be considered for Servo itself as well.
Flags: needinfo?(simon.sapin)
Assignee | ||
Comment 3•8 years ago
|
||
I've discussed this with Simon on IRC this morning, here's the log:
xidorn>
thoughts about property name?
SimonSapin>
I think I agree with your conclusion of going with simply atoms
xidorn>
and what about custom properties?
SimonSapin>
so custom property names are atomized with the -- prefix, which for servo atoms leaves two bytes fewer for inline atoms but oh well
(string_cache can store up to 7 bytes inline)
xidorn>
so you think it's probably worth trying to handle that separately?
SimonSapin>
ideally, in a world where stylo is gecko’s only style system, PropertyID should be a C-like enum generated in the style crate, but in the meantime dealing with nsCSSPropertyID defined in C++ would probably be a pain
I mean use e.g. atom!("font-size") (non-custom property names are static atoms) and Atom::from("--foo")
xidorn>
in a world stylo is gecko's only style system, shouldn't the C++ enum be generated from the servo side data?
SimonSapin>
style::PropertyDeclarationBlock would have a method fn get<'a>(&'a self, property_name: &Atom) -> (&'a PropertyDeclaration, Importance)
right, ideally it should be generated from the definition of every other property data, which is in the mako templates in style::properties
but as long as gecko’s C++ style system is still around, that might be difficult
so I’d say don’t do that now, use atoms instead, and maybe later we’ll revisit
xidorn>
actually Gecko can use atom as well I guess
I mean, we can use atoms in gecko as property name everywhere we need, when that day comes, I guess
hmm, but static atoms in gecko is still more expensive than enum constants. probably not a big issue
SimonSapin>
yeah I assumed that an enum with consecutive integer values would be cheaper (matching can optimize to a jump table) but maybe it’s not significant
anyway, the apis right now in style take &str or String (I don’t remember), and I’m gonna change them to &Atom soon
Flags: needinfo?(simon.sapin)
Assignee | ||
Comment 4•8 years ago
|
||
One further note that, I later realized that we actually need to distinguish between property and custom property before atomizing, because non-custom property needs to be lowercased, while custom property doesn't. So I would handle them separately, then.
Comment 5•8 years ago
|
||
It looks like not all property names are static atoms in Gecko yet, or at least not with bindings generated by Servo’s components/style/binding_tools/regen_atoms.py. It would help to have them, though in the meantime I can use the less efficient Atom::from(&str). Matt, are you still looking for something to do?
Flags: needinfo?(mbrubeck)
Comment 6•8 years ago
|
||
> Matt, are you still looking for something to do?
Sure. Would you like to file a new bug for that and assign it to me?
Flags: needinfo?(mbrubeck)
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
Simon, Matt, FYI, the atoms are done in part 7.
It seems currently touching Element.style doesn't update computed value. I suppose this is in the scope of restyling? Since I've been reusing the existing logic as much as possible, I guess restyling probably should hook in somewhere else in the pipeline to make changing to Element.style effective?
Comment 20•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #19)
> It seems currently touching Element.style doesn't update computed value. I
> suppose this is in the scope of restyling? Since I've been reusing the
> existing logic as much as possible, I guess restyling probably should hook
> in somewhere else in the pipeline to make changing to Element.style
> effective?
In Servo’s components/script/dom/cssstyledeclaration.rs, methods that make changes to the a style attribute end up calling `node.dirty(NodeDamage::NodeStyleDamaged)`, which marks the element’s computed style as out of date and in need of restyling. I assume Gecko has a similar mechanism?
Assignee | ||
Comment 21•8 years ago
|
||
I noticed that there is nsINode::SetIsDirtyForServo and nsINode::SetHasDirtyDescendantsForServo. I guess they are the mechanism we need, but I'm not currently confident on where they should be called. Probably some where inside Element::SetAttrAndNotify. I think we may also need to handle mapped attributes.
Assignee | ||
Comment 22•8 years ago
|
||
So in the original style system of Gecko, the invalidation is done in the following path:
Element::SetAttrAndNotify -> somehow ->
RestyleManager::AttributeChanged ->
nsStyleSet::HasAttributeDependentStyle -> somehow ->
nsHTMLCSSStyleSheet::HasAttributeDependentStyle
The last function would return eRestyle_StyleAttribute as a restyle hint, which would later be used in the next restyling.
So we probably should do something inside ServoRestyleManager::AttributeChanged.
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.
https://reviewboard.mozilla.org/r/88266/#review87266
The servo side looks good to me.
::: layout/style/nsCSSProps.cpp:736
(Diff revision 1)
> +#undef CSS_PROP
> +
> +static const nsStaticAtom CSSProps_info[] = {
> +#define CSS_PROP(name_, id_, ...) \
> + NS_STATIC_ATOM(id_##_buffer, (nsIAtom**)&nsCSSProps::id_),
> +#define CSS_PROP_SHORTHAND(name_, id_, ...) CSS_PROP(name_, id_, ...)
nit: I think this is not passing to `CSS_PROP` what you think it's passing (you should be using `__VA_ARGS__` instead of `...`).
Of course it doesn't matter because you don't use it, but...
::: servo/components/style/binding_tools/regen_atoms.py:73
(Diff revision 1)
> + CLASS = "nsCSSProps"
> + TYPE = "nsICSSProperty"
> +
> + @classmethod
> + def list_atoms(cls, content):
> + pattern = re.compile('^CSS_PROP_[A-Z]+\(\s*([^,]+),\s*([^,]+)', re.M)
Could you use `PATTERN` to be consistent?
Attachment #8804139 -
Flags: review?(ecoal95) → review+
Comment 24•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #22)
> So in the original style system of Gecko, the invalidation is done in the
> following path:
> Element::SetAttrAndNotify -> somehow ->
> RestyleManager::AttributeChanged ->
> nsStyleSet::HasAttributeDependentStyle -> somehow ->
> nsHTMLCSSStyleSheet::HasAttributeDependentStyle
>
> The last function would return eRestyle_StyleAttribute as a restyle hint,
> which would later be used in the next restyling.
>
> So we probably should do something inside
> ServoRestyleManager::AttributeChanged.
Special-casing the `style` attribute in `ServoRestyleManager::AttributeWillChange` sounds good to me. The rest of the attributes are only used to compute restyle hints based on the previous snapshot. We could also make the restyle hint code in servo be aware of the style attribute as a special case.
Assignee | ||
Comment 25•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.
https://reviewboard.mozilla.org/r/88266/#review87266
> Could you use `PATTERN` to be consistent?
There are two issues with using `PATTERN`: 1) the order is different in this case (1 is string value, 2 is identifier); 2) there could be duplicate because of conditional compile in C++ code.
We could, though, extend `PATTERN` to handle those cases... (2) is probably easy: just add deduplicate to the general algorithm. (1) would need patterns to use named group.
Probably doing that is better.
Comment 26•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.
https://reviewboard.mozilla.org/r/88266/#review87266
> There are two issues with using `PATTERN`: 1) the order is different in this case (1 is string value, 2 is identifier); 2) there could be duplicate because of conditional compile in C++ code.
>
> We could, though, extend `PATTERN` to handle those cases... (2) is probably easy: just add deduplicate to the general algorithm. (1) would need patterns to use named group.
>
> Probably doing that is better.
I just meant to move the definition of the regular expression where the rest are, even though you handled it differently via your method, but I guess making it completely consistent is even better :P
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 34•8 years ago
|
||
Comment on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.
emilio, you may want to review the updated patch again.
Attachment #8804139 -
Flags: review+ → review?(ecoal95)
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8804218 [details]
Bug 1294299 part 13 - Post restyle event with style attribute hint for style change.
bholley, I discussed with emilio about restyling for style attribute change, and he told me that you are currently working on something closely related to this, and we may support the eRestyle_StyleAttribute in the Servo side with your work. Is that correct? Could you provide some feedback about this patch?
Attachment #8804218 -
Flags: feedback?(bobbyholley)
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8804136 [details]
Bug 1294299 part 4 - Implement length and item getter.
https://reviewboard.mozilla.org/r/88260/#review87300
::: servo/ports/geckolib/glue.rs:473
(Diff revision 1)
> +#[no_mangle]
> +pub extern "C" fn Servo_DeclarationBlock_GetNthProperty(declarations: RawServoDeclarationBlockBorrowed,
> + index: u32, result: *mut nsAString) -> bool {
> + let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> + if let Some(&(ref decl, _)) = declarations.read().declarations.get(index as usize) {
> + let name = decl.name().to_string();
Since nsAString implements std::fmt::Write, you can use it with the write!() macro to avoid allocating this intermediate string:
let result = unsafe { result.as_mut().unwrap() };
write!(result, "{}", decl.name()).unwrap();
true
I also used `.unwrap(); true` here because the `Write for nsAString` string always returns `Ok(())`. But `.is_ok()` is fine if you prefer. (This `Result` is used for errors during writing, for example I/O errors when writing to a file or socket.)
Attachment #8804136 -
Flags: review?(simon.sapin) → review+
Comment 37•8 years ago
|
||
mozreview-review |
Comment on attachment 8804144 [details]
Bug 1294299 part 12 - Implemnet setter of properties.
https://reviewboard.mozilla.org/r/88276/#review87308
Attachment #8804144 -
Flags: review?(simon.sapin) → review+
Comment 38•8 years ago
|
||
mozreview-review |
Comment on attachment 8804143 [details]
Bug 1294299 part 11 - Implement getting and removing property.
https://reviewboard.mozilla.org/r/88274/#review87306
::: layout/style/ServoDeclarationBlock.h:50
(Diff revision 1)
> return Servo_DeclarationBlock_GetNthProperty(mRaw, aIndex, &aReturn);
> }
>
> + void GetPropertyValue(const nsAString& aProperty, nsAString& aValue) const;
> + void GetPropertyValueById(nsCSSPropertyID aPropID, nsAString& aValue) const;
> + void GetAuthoredPropertyValue(const nsAString& aProperty,
Why does GetAuthoredPropertValue exist if it’s the same as GetPropertyValue?
::: layout/style/ServoDeclarationBlock.cpp:65
(Diff revision 1)
> + nsIAtom* mAtom;
> + bool mIsCustomProperty;
> +};
> +
> +void
> +ServoDeclarationBlock::GetPropertyValue(const nsAString& aProperty,
Should this method support shorthands and do serialization per CSSOM? Does nsCSSPropertyID / nsCSSProps::LookupProperty include shorthands?
Attachment #8804143 -
Flags: review?(simon.sapin) → review+
Assignee | ||
Comment 39•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8804143 [details]
Bug 1294299 part 11 - Implement getting and removing property.
https://reviewboard.mozilla.org/r/88274/#review87306
> Why does GetAuthoredPropertValue exist if it’s the same as GetPropertyValue?
GetAuthoredPropertyValue is for providing an alternative serialization which is closer to what the author writes, rather than the standard normalized way. Gecko still has this method, and we may want to support that with stylo eventually, for devtools. Probably I should push this difference a bit further, and pass a flag to the Servo side?
> Should this method support shorthands and do serialization per CSSOM? Does nsCSSPropertyID / nsCSSProps::LookupProperty include shorthands?
The spec doesn't explicitly say so, but from the wording I suppose it should support shorthands. And yes, nsCSSPropertyID and nsCSSProps::LookupProperty includes shorthands, but excludes aliases. LookupProperty handles alias itself, and always return the corresponding non-alias property.
Comment 40•8 years ago
|
||
Comment on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.
LGTM :)
Attachment #8804139 -
Flags: review?(ecoal95) → review+
Comment 41•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8804143 [details]
Bug 1294299 part 11 - Implement getting and removing property.
https://reviewboard.mozilla.org/r/88274/#review87306
> GetAuthoredPropertyValue is for providing an alternative serialization which is closer to what the author writes, rather than the standard normalized way. Gecko still has this method, and we may want to support that with stylo eventually, for devtools. Probably I should push this difference a bit further, and pass a flag to the Servo side?
Ok, I see. No need to change this for now, we’ll see when we want this method to do something different with Stylo.
> The spec doesn't explicitly say so, but from the wording I suppose it should support shorthands. And yes, nsCSSPropertyID and nsCSSProps::LookupProperty includes shorthands, but excludes aliases. LookupProperty handles alias itself, and always return the corresponding non-alias property.
To clarify, I was refering to "If property is a shorthand property, then follow these substeps:" in https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface.
(Your reply tells me what I wanted to know, Thanks.)
Comment 42•8 years ago
|
||
Comment on attachment 8804218 [details]
Bug 1294299 part 13 - Post restyle event with style attribute hint for style change.
Yep, looks fine. The basic plan is for Servo to become more like Gecko in this respect, where we queue up restyle hints on various restyle hints, and expand them during traversal.
Attachment #8804218 -
Flags: feedback?(bobbyholley) → feedback+
Comment 43•8 years ago
|
||
mozreview-review |
Comment on attachment 8804133 [details]
Bug 1294299 part 1 - Make nsDOMCSSDeclaration use DeclarationBlock.
https://reviewboard.mozilla.org/r/88254/#review89484
Attachment #8804133 -
Flags: review?(cam) → review+
Comment 44•8 years ago
|
||
mozreview-review |
Comment on attachment 8804134 [details]
Bug 1294299 part 2 - Use DeclarationBlock for SMIL override style.
https://reviewboard.mozilla.org/r/88256/#review89514
Attachment #8804134 -
Flags: review?(cam) → review+
Comment 45•8 years ago
|
||
mozreview-review |
Comment on attachment 8804135 [details]
Bug 1294299 part 3 - Make it possible to create empty ServoDeclarationBlock.
https://reviewboard.mozilla.org/r/88258/#review89516
s/able/possible/ in the commit message.
::: layout/style/nsDOMCSSAttrDeclaration.cpp:135
(Diff revision 1)
> + css::Declaration* declaration = new css::Declaration();
> + declaration->InitializeEmpty();
> + decl = declaration;
Can we put the Declaration object in decl to start with? Although it's safe in this case, I'm generally wary of calling methods on objects with refcnt = 0 in case they do an AddRef/Release somewhere within the method, causing it to be deleted.
Attachment #8804135 -
Flags: review?(cam) → review+
Comment 46•8 years ago
|
||
mozreview-review |
Comment on attachment 8804136 [details]
Bug 1294299 part 4 - Implement length and item getter.
https://reviewboard.mozilla.org/r/88260/#review89520
Attachment #8804136 -
Flags: review?(cam) → review+
Comment 47•8 years ago
|
||
mozreview-review |
Comment on attachment 8804137 [details]
Bug 1294299 part 5 - Implement getter and setter of cssText.
https://reviewboard.mozilla.org/r/88262/#review89524
Attachment #8804137 -
Flags: review?(cam) → review+
Comment 48•8 years ago
|
||
mozreview-review |
Comment on attachment 8804138 [details]
Bug 1294299 part 6 - Change ident of float property to float_.
https://reviewboard.mozilla.org/r/88264/#review89526
Attachment #8804138 -
Flags: review?(cam) → review+
Comment 49•8 years ago
|
||
mozreview-review |
Comment on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.
https://reviewboard.mozilla.org/r/88266/#review89530
r=me on the C++ bits.
::: layout/style/nsCSSProps.h:684
(Diff revision 2)
> +#define CSS_PROP(name_, id_, ...) static nsICSSProperty* id_;
> +#define CSS_PROP_SHORTHAND(name_, id_, ...) CSS_PROP(name_, id_, ...)
> +#define CSS_PROP_LIST_INCLUDE_LOGICAL
> +#include "nsCSSPropList.h"
> +#undef CSS_PROP_LIST_INCLUDE_LOGICAL
> +#undef CSS_PROP_SHORTHAND
> +#undef CSS_PROP
> +
> +private:
> + static nsICSSProperty* gPropertyAtomTable[eCSSProperty_COUNT];
I think the list of separate |static nsICSSProperty* id_| variables should be just the same as the array of the gPropertyAtomTable. Is that right? Can we share the storage for these with a union, so that we don't have to have two copies of the pointers? There's probably some reason it doesn't work (and maybe you already tried).
Attachment #8804139 -
Flags: review?(cam) → review+
Assignee | ||
Comment 50•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.
https://reviewboard.mozilla.org/r/88266/#review89530
> I think the list of separate |static nsICSSProperty* id_| variables should be just the same as the array of the gPropertyAtomTable. Is that right? Can we share the storage for these with a union, so that we don't have to have two copies of the pointers? There's probably some reason it doesn't work (and maybe you already tried).
Hmmm, they are the same array, but the issue is we need this to be linkable from Rust, so we may need to live with this duplication... Adding two levels of types (a union and a struct) may significantly complicate the atom binding generator code.
Comment 51•8 years ago
|
||
mozreview-review |
Comment on attachment 8804140 [details]
Bug 1294299 part 8 - Refactor interface provided by css::Declaration.
https://reviewboard.mozilla.org/r/88268/#review89536
Looks good, I like the removal of the regular-vs-custom property checks we were doing all over the place.
::: layout/style/Declaration.h:125
(Diff revision 2)
> * for this declaration. aProperty must not be a shorthand.
> */
> void ValueAppended(nsCSSPropertyID aProperty);
>
> - void RemoveProperty(nsCSSPropertyID aProperty);
> + void GetPropertyValue(const nsAString& aProperty, nsAString& aValue) const;
> + void GetPropertyValueById(nsCSSPropertyID aPropID, nsAString& aValue) const;
Maybe "GetPropertyValueByID"? That would be more in line with Gecko's usual capitalization style, and also matches the nsCSSPropertyID type spelling. Similarly for RemovePropertyById and GetPropertyIsImportantById.
::: layout/style/Declaration.cpp:176
(Diff revision 2)
> +Declaration::GetPropertyValue(const nsAString& aProperty,
> + nsAString& aValue) const
> +{
> + DispatchPropertyOperation(aProperty,
> + [&](nsCSSPropertyID propID) { GetPropertyValueById(propID, aValue); },
> + [&](const nsAString& name) { GetVariableDeclaration(name, aValue); });
Since we're renaming some of these functions, it would be good if we could align the names of the variable-related functions too, e.g. use GetVariableValue, GetVariableIsImportant, RemoveVariable. Feel free to do that here or as a followup.
Attachment #8804140 -
Flags: review?(cam) → review+
Comment 52•8 years ago
|
||
mozreview-review |
Comment on attachment 8804141 [details]
Bug 1294299 part 9 - Implement Clone for ServoDeclarationBlock.
https://reviewboard.mozilla.org/r/88270/#review89540
Attachment #8804141 -
Flags: review?(cam) → review+
Comment 53•8 years ago
|
||
mozreview-review |
Comment on attachment 8804142 [details]
Bug 1294299 part 10 - Implement DeclarationBlock.EnsureMutable.
https://reviewboard.mozilla.org/r/88272/#review89546
Attachment #8804142 -
Flags: review?(cam) → review+
Comment 54•8 years ago
|
||
mozreview-review |
Comment on attachment 8804143 [details]
Bug 1294299 part 11 - Implement getting and removing property.
https://reviewboard.mozilla.org/r/88274/#review89552
::: layout/style/ServoDeclarationBlock.cpp:56
(Diff revision 2)
> + NS_RELEASE(mAtom);
> + }
> + }
> +
> + explicit operator bool() const { return !!mAtom; }
> + nsIAtom* Atom() const { return mAtom; }
Maybe MOZ_ASSERT(mAtom) in here, since we didn't call the method "GetAtom"?
::: layout/style/ServoDeclarationBlock.cpp:68
(Diff revision 2)
> +
> +void
> +ServoDeclarationBlock::GetPropertyValue(const nsAString& aProperty,
> + nsAString& aValue) const
> +{
> + if (PropertyAtomHolder holder{aProperty}) {
I learned something new: that inside here you must use the uniform initialization syntax.
::: servo/ports/geckolib/glue.rs:502
(Diff revision 2)
> + result.push_str("--");
> + write!(result, "{}", atom).unwrap();
Maybe just put the "--" inside the write!() call?
Attachment #8804143 -
Flags: review?(cam) → review+
Comment 55•8 years ago
|
||
mozreview-review |
Comment on attachment 8804144 [details]
Bug 1294299 part 12 - Implemnet setter of properties.
https://reviewboard.mozilla.org/r/88276/#review89556
::: layout/style/nsDOMCSSDeclaration.cpp:357
(Diff revision 2)
> - env.mBaseURI, env.mPrincipal, decl->AsGecko(),
> + env.mBaseURI, env.mPrincipal, decl->AsGecko(),
> - &changed, aIsImportant);
> + &changed, aIsImportant);
> + } else {
> + RefPtr<nsIAtom> atom = NS_Atomize(propName);
> + NS_ConvertUTF16toUTF8 value(aPropValue);
> + changed = Servo_DeclarationBlock_SetProperty(
I guess it's a little confusing that we are passing the variable name (i.e. the bit after the "--" here) to a function with "Property" in the name. I wonder if we should just pass the entire custom property name down to Servo functions?
Attachment #8804144 -
Flags: review?(cam) → review+
Comment 56•8 years ago
|
||
mozreview-review |
Comment on attachment 8804218 [details]
Bug 1294299 part 13 - Post restyle event with style attribute hint for style change.
https://reviewboard.mozilla.org/r/88308/#review89560
Attachment #8804218 -
Flags: review?(cam) → review+
Assignee | ||
Comment 57•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8804144 [details]
Bug 1294299 part 12 - Implemnet setter of properties.
https://reviewboard.mozilla.org/r/88276/#review89556
> I guess it's a little confusing that we are passing the variable name (i.e. the bit after the "--" here) to a function with "Property" in the name. I wonder if we should just pass the entire custom property name down to Servo functions?
I think we want to pass atom to Servo functions, and since we have to distinguish between normal property and custom property before atomizing, it makes the most of sense to only atomize the part without "--". What might be less confusing here is probably having two different functions for normal property and custom property correspondingly rather than using a boolean parameter for that.
Anyway, parameters in this function is same as other binding functions on DeclarationBlock, so I don't think that's a big issue.
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) |
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 71•8 years ago
|
||
Comment on attachment 8804218 [details]
Bug 1294299 part 13 - Post restyle event with style attribute hint for style change.
Added a hack to convert StyleAttribute hint to Subtree hint so that restyle works. You may want to re-review this change.
Attachment #8804218 -
Flags: review+ → review?(cam)
Comment 72•8 years ago
|
||
mozreview-review |
Comment on attachment 8804218 [details]
Bug 1294299 part 13 - Post restyle event with style attribute hint for style change.
https://reviewboard.mozilla.org/r/88308/#review89946
r=me but maybe remove the StyleAttribute bit at the same time.
Attachment #8804218 -
Flags: review?(cam) → review+
Assignee | ||
Comment 73•8 years ago
|
||
Comment 74•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5d9862e411
part 1 - Make nsDOMCSSDeclaration use DeclarationBlock. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/409dcb59fb9b
part 2 - Use DeclarationBlock for SMIL override style. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/90e4f363df5c
part 3 - Make it possible to create empty ServoDeclarationBlock. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a582c5930e5
part 4 - Implement length and item getter. r=SimonSapin,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc84ed83a79b
part 5 - Implement getter and setter of cssText. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3df5f8f1767
part 6 - Change ident of float property to float_. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/091b1a0f3e87
part 7 - Generate static atoms for CSS properties. r=emilio,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/257cfb683437
part 8 - Refactor interface provided by css::Declaration. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe544496b0e
part 9 - Implement Clone for ServoDeclarationBlock. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/521987ad1ac6
part 10 - Implement DeclarationBlock.EnsureMutable. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/90dcb2e27f42
part 11 - Implement getting and removing property. r=SimonSapin,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f673f5c6b3f
part 12 - Implemnet setter of properties. r=SimonSapin,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcbd28c7f79e
part 13 - Post restyle event with style attribute hint for style change. r=heycam
Assignee | ||
Updated•8 years ago
|
See Also: → https://github.com/servo/servo/pull/14038
Comment 75•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d5d9862e411
https://hg.mozilla.org/mozilla-central/rev/409dcb59fb9b
https://hg.mozilla.org/mozilla-central/rev/90e4f363df5c
https://hg.mozilla.org/mozilla-central/rev/6a582c5930e5
https://hg.mozilla.org/mozilla-central/rev/dc84ed83a79b
https://hg.mozilla.org/mozilla-central/rev/f3df5f8f1767
https://hg.mozilla.org/mozilla-central/rev/091b1a0f3e87
https://hg.mozilla.org/mozilla-central/rev/257cfb683437
https://hg.mozilla.org/mozilla-central/rev/dfe544496b0e
https://hg.mozilla.org/mozilla-central/rev/521987ad1ac6
https://hg.mozilla.org/mozilla-central/rev/90dcb2e27f42
https://hg.mozilla.org/mozilla-central/rev/0f673f5c6b3f
https://hg.mozilla.org/mozilla-central/rev/fcbd28c7f79e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 76•8 years ago
|
||
Attachment #8808032 -
Flags: review?(cam)
Updated•8 years ago
|
Attachment #8808032 -
Flags: review?(cam) → review+
Assignee | ||
Comment 77•8 years ago
|
||
Set checkin-needed for the followup patch.
Keywords: checkin-needed
Comment 78•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d10212aea2
Followup: Serialize style attribute for Element.getAttribute. r=heycam
Keywords: checkin-needed
Comment 79•8 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•