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)

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(14 files)

Bug 1294299 part 1 - Make nsDOMCSSDeclaration use DeclarationBlock.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 2 - Use DeclarationBlock for SMIL override style.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 3 - Make it possible to create empty ServoDeclarationBlock.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 4 - Implement length and item getter.
58 bytes, text/x-review-board-request
heycam
: review+
SimonSapin
: review+
Details
Bug 1294299 part 5 - Implement getter and setter of cssText.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 6 - Change ident of float property to float_.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 7 - Generate static atoms for CSS properties.
58 bytes, text/x-review-board-request
heycam
: review+
emilio
: review+
Details
Bug 1294299 part 8 - Refactor interface provided by css::Declaration.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 9 - Implement Clone for ServoDeclarationBlock.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 10 - Implement DeclarationBlock.EnsureMutable.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1294299 part 11 - Implement getting and removing property.
58 bytes, text/x-review-board-request
heycam
: review+
SimonSapin
: review+
Details
Bug 1294299 part 12 - Implemnet setter of properties.
58 bytes, text/x-review-board-request
heycam
: review+
SimonSapin
: review+
Details
Bug 1294299 part 13 - Post restyle event with style attribute hint for style change.
58 bytes, text/x-review-board-request
heycam
: review+
Details
followup patch for fixing getAttribute
1.24 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
      No description provided.
Priority: -- → P3
Summary: Stylo: Implement CSSStyleDeclaration interface → Stylo: Implement CSSStyleDeclaration for Element.style
Depends on: 1295865
Depends on: 1294742
Depends on: 1296186
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.)
Depends on: 1309109
Assignee: nobody → xidorn+moz
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)
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)
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.
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)
> 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)
Blocks: 1310886
Depends on: 1312338
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?
Blocks: 1307357
(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?
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.
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 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+
(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.
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 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 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)
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 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 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 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+
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 on attachment 8804139 [details]
Bug 1294299 part 7 - Generate static atoms for CSS properties.

LGTM :)
Attachment #8804139 - Flags: review?(ecoal95) → review+
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 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 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 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 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 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 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 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 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+
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 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 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 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 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 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 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+
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 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 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+
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
Attachment #8808032 - Flags: review?(cam) → review+
Set checkin-needed for the followup patch.
Keywords: checkin-needed
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: