Closed Bug 1388255 Opened 7 years ago Closed 7 years ago

Move mGridTemplate{Columns,Rows} into a UniquePtr

Categories

(Core :: CSS Parsing and Computation, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

Bug 1388255 - Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr.
59 bytes, text/x-review-board-request
canova
: review+
dholbert
: review+
Details 
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 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 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.)
> ...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 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 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 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 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+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02097a6c83f4
Make nsStylePosition::mGridTemplate{Columns,Rows} a UniquePtr. r=canaltinova,dholbert
https://hg.mozilla.org/mozilla-central/rev/02097a6c83f4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: