Closed Bug 1306212 Opened 3 years ago Closed 3 years ago

Simplify StyleSheetInfo

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(3 files)

After bug 1304302, StyleSheetInfo becomes a simple aggregate data struct. We can simplify that struct, removing its visibility restriction as well as friend classes, and make it a member rather than a base class of ServoStyleSheet.
Assignee: nobody → xidorn+moz
Comment on attachment 8796005 [details]
Bug 1306212 part 1 - Make StyleSheetInfo a member of ServoStyleSheet.

https://reviewboard.mozilla.org/r/81984/#review80888

::: layout/style/ServoStyleSheet.h:60
(Diff revision 1)
>  
>  private:
>    void DropSheet();
>  
>    RefPtr<RawServoStyleSheet> mSheet;
> +  StyleSheetInfo mInner;

Can we call this something other than mInner?  Maybe mSheetInfo?  Although the symmetry in StyleSheet::SheetInfo() looks nice, I think a "style sheet inner" is different from what the StyleSheetInfo is.  (The former is nearly the entire data related to the style sheet, and the latter is just a few pieces of metadata.)
Attachment #8796005 - Flags: review?(cam) → review+
Comment on attachment 8796006 [details]
Bug 1306212 part 2 - Remove visibility restriction of StyleSheetInfo.

https://reviewboard.mozilla.org/r/81986/#review80890
Attachment #8796006 - Flags: review?(cam) → review+
Comment on attachment 8796007 [details]
Bug 1306212 part 3 - Move the only constructor of StyleSheetInfo to StyleSheet.cpp.

https://reviewboard.mozilla.org/r/81988/#review80892

I'm not sure what the advantage is of moving this constructor away from the file whose name matches the class name, but OK.
Attachment #8796007 - Flags: review?(cam) → review+
Comment on attachment 8796007 [details]
Bug 1306212 part 3 - Move the only constructor of StyleSheetInfo to StyleSheet.cpp.

https://reviewboard.mozilla.org/r/81988/#review80892

That constructor is so trivial that I almost want to merge it into the header directly... But I don't want to add another dependency to that header, so moved it to another cpp.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37ea2043dfb9
part 1 - Make StyleSheetInfo a member of ServoStyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6e3d1f53047f
part 2 - Remove visibility restriction of StyleSheetInfo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7833dc154154
part 3 - Move the only constructor of StyleSheetInfo to StyleSheet.cpp. r=heycam
You need to log in before you can comment on or make changes to this bug.