bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Use struct rather than nsCSSValue::Array to store counter functions

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

53 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

a year ago
So that we don't need to resolve counter style for each counter use node, and we can share anonymous counter styles among use nodes.

I make it block bug 1363596 because I feel it would make that one easier to implement.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8870212 [details]
Bug 1366735 part 1 - Change counter functions to use struct rather than nsCSSValue::Array.

https://reviewboard.mozilla.org/r/141662/#review145388

r=me, since I accidentally reviewed this before realising it wasn't addressed to me...

::: layout/style/nsRuleNode.cpp:8922
(Diff revision 1)
> +              CounterStyleManager()->BuildCounterStyle(style.GetAtomValue());
> +          } else if (style.GetUnit() == eCSSUnit_Symbols) {
> +            counterFunc->mCounterStyle =
> +              new AnonymousCounterStyle(style.GetArrayValue());
> +          } else {
> +            MOZ_ASSERT_UNREACHABLE("Unknow counter style");

Unknown
Attachment #8870212 - Flags: review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

https://reviewboard.mozilla.org/r/141664/#review145400

::: layout/style/nsStyleStruct.h:3065
(Diff revision 1)
> +    // One and only one of mCounterStyle and mCounterStyleName must be
> +    // non-null at any time.
>      mozilla::CounterStylePtr mCounterStyle;
> +    nsCOMPtr<nsIAtom> mCounterStyleName;

At this point I wonder whether this should be a class, encapsulating that requirement, rather than a struct.

::: servo/components/style/gecko_bindings/sugar/ns_com_ptr.rs:19
(Diff revision 1)
>          self.mRawPtr
>      }
> +
> +    /// Set this pointer from an addrefed raw pointer.
> +    #[inline]
> +    pub unsafe fn set_raw<U>(&mut self, ptr: *mut T) {

I think the name set_raw doesn't make it obvious that it's assigning something without AddRefing.

For RefPtr, we have RefPtr::from_addrefed, so we do

  let refptr = RefPtr::from_addrefed(raw_ptr);
  some_gecko_thing.mSomething.set_move(refptr);

Should we follow the same pattern?  (Well, it is pretty wordy, but I think we should be consistent.)
(Assignee)

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

https://reviewboard.mozilla.org/r/141664/#review145400

> At this point I wonder whether this should be a class, encapsulating that requirement, rather than a struct.

Maybe... but it seems to me the code is pretty short, and this pattern would only be used in these two places, so encapsulating that may not save us any line of code.

I was actually considering merging `nsCOMPtr<nsIAtom>` into `CounterStylePtr`, which is doable and would save us a word in memory for the structs. But I think we can do that later. If we eventually want to do something like that, we probably don't want to encapsulate it for now.

> I think the name set_raw doesn't make it obvious that it's assigning something without AddRefing.
> 
> For RefPtr, we have RefPtr::from_addrefed, so we do
> 
>   let refptr = RefPtr::from_addrefed(raw_ptr);
>   some_gecko_thing.mSomething.set_move(refptr);
> 
> Should we follow the same pattern?  (Well, it is pretty wordy, but I think we should be consistent.)

Probably `set_from_addrefed`?
(Assignee)

Updated

a year ago
Attachment #8870213 - Flags: review?(cam)
(Assignee)

Comment 8

a year ago
Not sure why MozReview cancels r? heycam for part 2...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> Probably `set_from_addrefed`?

OK.  (Maybe we should have that for RefPtr too then.  Unless there's some advantage to the pattern of x.move(RefPtr::from_addrefed(...)).)
(Assignee)

Comment 10

a year ago
(In reply to Cameron McCormack (:heycam) from comment #9)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> > Probably `set_from_addrefed`?
> 
> OK.  (Maybe we should have that for RefPtr too then.  Unless there's some
> advantage to the pattern of x.move(RefPtr::from_addrefed(...)).)

Maybe. In long term, I think we want something like AddRefed<T> similar to our already_AddRefed in Gecko, and make things return that (e.g. "impl From<Atom> for AddRefed<nsIAtom>" and verse vice, and make binding functions use that as well), so we can have less unsafe code, and stronger type-safety.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8870213 [details]
Bug 1366735 part 2 - Use the new struct in stylo.

https://reviewboard.mozilla.org/r/141664/#review145736
Attachment #8870213 - Flags: review?(cam) → review+
(Assignee)

Updated

a year ago
Attachment #8870212 - Flags: review?(dbaron)
Attachment #8870214 - Flags: review?(dbaron) → review?(cam)

Comment 15

a year ago
mozreview-review
Comment on attachment 8870214 [details]
Bug 1366735 part 3 - Remove SetCounterStyleDirty.

https://reviewboard.mozilla.org/r/141666/#review145830

Can you please mention in a comment in nsStyleContent::CalcDifference that we rely on returning ReconstructFrame so that we don't have counter use nodes with references to stale counter styles.
Attachment #8870214 - Flags: review?(cam) → review+
(Assignee)

Comment 16

a year ago
Servo side: servo/servo#17016
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a year ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/787d3a0c5dd1
part 1 - Change counter functions to use struct rather than nsCSSValue::Array. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3da40efe9851
part 2 - Use the new struct in stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ed34125549db
part 3 - Remove SetCounterStyleDirty. r=heycam
https://hg.mozilla.org/mozilla-central/rev/787d3a0c5dd1
https://hg.mozilla.org/mozilla-central/rev/3da40efe9851
https://hg.mozilla.org/mozilla-central/rev/ed34125549db
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.