1549267 - Some thoughts on the usage of the constants in LayoutConstants.h

Status: RESOLVED FIXED

(Core :: Layout, defect, P5)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Inspired by dholbert's comment in https://phabricator.services.mozilla.com/D29616?id=98435#inline-172862,

nscoord_MAX is semantically "giant number", whereas NS_INTRINSICSIZE is semantically "sentinel value, whose underlying representation you should not depend on"

So here's my thoughts on the constants define in LayoutConstants.h:

1. We should add document to NS_UNCONSTRAINEDSIZE to warn people not to rely on its value, and point to nscoord_MAX if a giant number is needed.
2. We should just replace NS_MAXSIZE with nscoord_MAX because it doens't have many usage per https://searchfox.org/mozilla-central/search?q=symbol:M_1833fe56%2CM_a4599bec68c82a18&redirect=false
3. Do we really need NS_INTRINSICSIZE, NS_AUTOHEIGHT, NS_AUTOOFFSET? Since they're all equal to NS_UNCONSTRAINEDSIZE, can we just remove them? Without proper documentation, it's difficult (at least to me) to choose when to use which constant. Mnemonics are good unless we have a proper way to enforce them, but they are all nscoord...

Any other thoughts?

Comment 1

[Mats Palmgren (inactive)]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #0)

1. We should add document to NS_UNCONSTRAINEDSIZE to warn people not to rely on its value, and point to nscoord_MAX if a giant number is needed.

Documenting it and advising people to not depend on its numeric value is good,
however I don't think we should use nscoord_MAX directly in layout (in general).
If a new symbolic value is needed for some purpose then it's better to add it
to this header with documentation on what it represents symbolically.

2. We should just replace NS_MAXSIZE with nscoord_MAX because it doens't have many usage per https://searchfox.org/mozilla-central/search?q=symbol:M_1833fe56%2CM_a4599bec68c82a18&redirect=false

I don't think replacing it with nscoord_MAX is an improvement, since neither
convey any semantics or intention. We should probably replace NS_MAXSIZE
with some of the other symbolic values instead, or introduce new ones that
represent the specific use cases, if needed. (Like ASK_FOR_BASELINE.)

3. Do we really need NS_INTRINSICSIZE, NS_AUTOHEIGHT, NS_AUTOOFFSET? Since they're all equal to NS_UNCONSTRAINEDSIZE, can we just remove them? Without proper documentation, it's difficult (at least to me) to choose when to use which constant. Mnemonics are good unless we have a proper way to enforce them, but they are all nscoord...

NS_AUTOHEIGHT and NS_INTRINSICSIZE can probably be replaced by
NS_UNCONSTRAINEDSIZE, since they have (roughly) the same meaning.
NS_AUTOOFFSET is a specific use case though, that we might want to
keep as a distinct constant.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1549267 Part 1 - Remove NS_INTRINSICSIZE and NS_AUTOHEIGHT. r=mats

This patch is generated by the following steps.

1. Manually delete NS_INTRINSICSIZE and NS_AUTOHEIGHT in LayoutConstants.

2. Run the following script.
   #!/bin/bash
   function rename() {
   find .
   -type f
   ! -path "./obj*"
   ! -path "./.git"
   ! -path "./.hg"
   ( -name ".cpp" -or
   -name ".h" )
   -exec sed -i -e "s/$1/$2/g" "{}" ;
   }

rename NS_INTRINSICSIZE NS_UNCONSTRAINEDSIZE
rename NS_AUTOHEIGHT NS_UNCONSTRAINEDSIZE

3. ./mach clang-format

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1549267 Part 2 - Improve comments in LayoutConstants.h. r=mats

Depends on D31696

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23bbfebbe583
Part 1 - Remove NS_INTRINSICSIZE and NS_AUTOHEIGHT. r=mats
https://hg.mozilla.org/integration/autoland/rev/778464d9aac8
Part 2 - Improve comments in LayoutConstants.h. r=mats

Comment 5

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/23bbfebbe583
https://hg.mozilla.org/mozilla-central/rev/778464d9aac8