(In reply to Emilio Cobos Álvarez (:emilio) from comment #35) > I added https://github.com/w3c/csswg-drafts/issues/3686 to the agenda to get > a resolution on that issue. If you want to comment the details of the > solution you ended up using (in particular the important detail of mapping > <li value> to two declarations) that's great, otherwise let me know and I'll > do that. I amended my last comment with that detail. > > + if (!aDecls.PropertyIsSet(eCSSProperty_counter_set)) { > > I think you could just assert that they're not set, but we should do that > all over the place basically, so this is fine for now. Yeah, I noticed this too, but figured I should follow the established pattern. But you're right, it's probably better to assert in general. I'll file that as a follow-up bug. > > + use crate::values::CustomIdent; > > nit: Maybe move all the `use` statements to the of the function? We don't > have many mid-function, but not a big deal. OK, idk what the preferred style is for these, so I followed a "closest to first use" rule of thumb. > A trivial improvement that avoids a copy and would also make it slightly > easier to follow IMO would be: Sounds good. Fwiw, the part that I couldn't figure out how to do was: > increments.0.push(..); I didn't realizes I could access the internal vector like that...
Bug 288704 Comment 36 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #35) > I added https://github.com/w3c/csswg-drafts/issues/3686 to the agenda to get > a resolution on that issue. If you want to comment the details of the > solution you ended up using (in particular the important detail of mapping > <li value> to two declarations) that's great, otherwise let me know and I'll > do that. I amended my last comment with that detail. > > + if (!aDecls.PropertyIsSet(eCSSProperty_counter_set)) { > > I think you could just assert that they're not set, but we should do that > all over the place basically, so this is fine for now. Yeah, I noticed this too, but figured I should follow the established pattern. But you're right, it's probably better to assert in general. I'll file that as a follow-up bug. > > + use crate::values::CustomIdent; > > nit: Maybe move all the `use` statements to the of the function? We don't > have many mid-function, but not a big deal. OK, idk what the preferred style is for these, so I followed a "closest to first use" rule of thumb. > A trivial improvement that avoids a copy and would also make it slightly > easier to follow IMO would be: Sounds good. Fwiw, the part that I couldn't figure out how to do was: > increments.0.push(..); I didn't realize I could access the internal vector like that...