Closed Bug 1366735 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(3 files)

Bug 1366735 part 1 - Change counter functions to use struct rather than nsCSSValue::Array.
59 bytes, text/x-review-board-request
Details
Bug 1366735 part 2 - Use the new struct in stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1366735 part 3 - Remove SetCounterStyleDirty.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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 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 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.)
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`?
Attachment #8870213 - Flags: review?(cam)
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(...)).)
(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 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+
Attachment #8870212 - Flags: review?(dbaron)
Attachment #8870214 - Flags: review?(dbaron) → review?(cam)
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+
Servo side: servo/servo#17016
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: