1388255 - Move mGridTemplate{Columns,Rows} into a UniquePtr

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

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 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.

Review commit: https://reviewboard.mozilla.org/r/166380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166380/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5c25a18b313e8de300a462ea3ace7c9cedc2719

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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?

Comment 4

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

> ...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

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166380/diff/1-2/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

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...

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

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 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166380/diff/2-3/

Comment 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8895214 [details]
Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166380/diff/3-4/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

https://github.com/servo/servo/pull/18042

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02097a6c83f4
Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr. r=canaltinova,dholbert

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02097a6c83f4