Closed Bug 1549267 Opened 1 year ago Closed 1 year ago

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

Categories

(Core :: Layout, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

Details

Attachments

(2 files)

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?

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

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

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

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

  1. ./mach clang-format
Pushed by aethanyc@gmail.com:
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
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.