Closed
Bug 1388255
Opened 8 years ago
Closed 8 years ago
Move mGridTemplate{Columns,Rows} into a UniquePtr
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(1 file)
This sounds like an easy win to just move the two nsStyleGridTemplates behind a UniquePtr, so that we can make nsStylePosition 10 words smaller.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.
https://reviewboard.mozilla.org/r/166380/#review171798
It looks good to me from stylo's point of view. But Daniel's review is probably more important :)
::: servo/components/style/properties/gecko.mako.rs:1711
(Diff revision 1)
> - ${self_grid}.set_mIsAutoFill(false);
> - ${self_grid}.set_mIsSubgrid(false);
> -
> let max_lines = nsStyleGridLine_kMaxLine as usize - 1; // for accounting the final <line-names>
>
> - match v {
> + let result: *mut nsStyleGridTemplate = match v {
Can't ructc infer this type?
Attachment #8895214 -
Flags: review?(canaltinova) → review+
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.
https://reviewboard.mozilla.org/r/166380/#review171784
::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js:185
(Diff revision 1)
> - ["Gecko_SetStyleGridTemplateArrayLengths", "aValue", null],
> + ["Gecko_SetStyleGridTemplate", "aGridTemplate", null],
> - ["Gecko_SetGridTemplateLineNamesLength", "aValue", null],
This looks like a servo thing, based on nearby code-comments, so I'll defer to canaltinova here.
::: layout/generic/nsGridContainerFrame.cpp:46
(Diff revision 1)
> const uint32_t nsGridContainerFrame::kAutoLine = kTranslatedMaxLine + 3457U;
> typedef nsTHashtable< nsPtrHashKey<nsIFrame> > FrameHashtable;
> typedef mozilla::CSSAlignUtils::AlignJustifyFlags AlignJustifyFlags;
> typedef nsLayoutUtils::IntrinsicISizeType IntrinsicISizeType;
>
> +static UniquePtr<const nsStyleGridTemplate> sDefaultGridTemplate;
This should be StaticAutoPtr, I think.
Per https://dxr.mozilla.org/mozilla-central/rev/a921bfb8a2cf3db4d9edebe9b35799a3f9d035da/xpcom/base/StaticPtr.h#21-30 , StaticAutoPtr doesn't add a new static initializer -- but I suspect UniquePtr does (because it has a non-trivial default constructor).
So I think StaticAutoPtr is still preferable to UniquePtr for this use-case.
::: layout/generic/nsGridContainerFrame.cpp:2585
(Diff revision 1)
> +/* static */ void
> +nsGridContainerFrame::ShutdownStatic()
> +{
> + MOZ_ASSERT(sDefaultGridTemplate);
> + sDefaultGridTemplate = nullptr;
> +}
It looks like you can get rid of this ShutdownStatic method entirely, and just replace it with:
ClearOnShutdown(&sDefaultGridTemplate);
(if sDefaultGridTemplate becomes StaticAutoPtr, that is)
That seems cleaner to me -- it lets us do initialization and cleanup all at once in one method call, with one fewer action-at-a-distance that nsLayoutStatics needs to worry about getting correct.
I'm fine either way, though, I suppose.
::: layout/generic/nsGridContainerFrame.cpp:3141
(Diff revision 1)
> aComputedMinSize.ISize(aState.mWM),
> aComputedSize.ISize(aState.mWM),
> aComputedMaxSize.ISize(aState.mWM));
> mGridColEnd = mExplicitGridColEnd =
> aState.mColFunctions.ComputeExplicitGridEnd(areas ? areas->mNColumns + 1 : 1);
> - LineNameMap colLineNameMap(gridStyle->mGridTemplateColumns, numRepeatCols);
> + LineNameMap colLineNameMap(GetGridTemplateColumns(gridStyle), numRepeatCols);
I think this (and most of this patch) would be cleaner if you represented these as accessors like so:
gridStyle->GridTemplateColumns()
gridStyle->GridTemplateRows()
...rather than with a getter that has
...which could still return "const nsStyleGridTemplate&". And it'd just be an implementation detail that the underlying data happens to default to null (which-means-use-the-singleton).
Benefits:
- Usages would be a little shorter (due to lack of "Get") and more declarative.
- You wouldn't need to expose nsGridContainerFrame::DefaultGridTemplate() publicly like you do now (for nsComputedDOMStyle)
- Most of your changes to nsComputedDOMStyle (s/aTrackList/trackList/) would become unnecessary.
This *would* mean you'd have to move the static global variable over to nsStyleStruct.cpp, but that's fine I think. (It could even just be a lazily-initialized static local variable, if you like -- we don't necessarily need the nsLayoutStatics initialization dance.)
Comment 5•8 years ago
|
||
> ...rather than with a getter that has
(Sorry for the abruptly ending sentence there -- poor self-editing on my part. Just ignore that line.)
Comment 6•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.
https://reviewboard.mozilla.org/r/166380/#review171784
> This looks like a servo thing, based on nearby code-comments, so I'll defer to canaltinova here.
We have to whitelist some bindings with their out parameter names. Also we were whitelisting previous bindings too. Looks okay.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.
https://reviewboard.mozilla.org/r/166380/#review171798
> Can't ructc infer this type?
I thought it couldn't, because `null_mut()` returns `*mut _`, while other two branches return `&mut nsStyleGridTemplate`, and I didn't believe rustc is able to merge them into `*mut nsStyleGridTemplate`... That looks magic...
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.
https://reviewboard.mozilla.org/r/166380/#review171784
> I think this (and most of this patch) would be cleaner if you represented these as accessors like so:
> gridStyle->GridTemplateColumns()
> gridStyle->GridTemplateRows()
> ...rather than with a getter that has
>
> ...which could still return "const nsStyleGridTemplate&". And it'd just be an implementation detail that the underlying data happens to default to null (which-means-use-the-singleton).
>
> Benefits:
> - Usages would be a little shorter (due to lack of "Get") and more declarative.
> - You wouldn't need to expose nsGridContainerFrame::DefaultGridTemplate() publicly like you do now (for nsComputedDOMStyle)
> - Most of your changes to nsComputedDOMStyle (s/aTrackList/trackList/) would become unnecessary.
>
> This *would* mean you'd have to move the static global variable over to nsStyleStruct.cpp, but that's fine I think. (It could even just be a lazily-initialized static local variable, if you like -- we don't necessarily need the nsLayoutStatics initialization dance.)
Yeah, you're right that it is much cleaner this way.
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.
https://reviewboard.mozilla.org/r/166380/#review172558
r=me on the non-servo-related stuff.
Attachment #8895214 -
Flags: review?(dholbert) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years ago
|
||
Comment 14•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02097a6c83f4
Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr. r=canaltinova,dholbert
Comment 15•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•