Closed Bug 91419 Opened 23 years ago Closed 10 years ago

replace NS_FRAME_IS_SPECIAL and friends with more meaningful names.

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: waterson, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 2 obsolete files)

17.47 KB, patch
Details | Diff | Splinter Review
11.88 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
3.21 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
38.36 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
13.00 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
2.90 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
9.36 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
47.02 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
22.32 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
16.16 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Marc say: ``NS_FRAME_IS_SPECIAL is a terrible name!''. Boy is he right.
Thanks for creating the bug. Since I'm the disgruntled one, I'll take it.
Assignee: waterson → attinasi
Well, the patch compiled :-)  Pretty simple editor jockeying really.
Status: NEW → ASSIGNED
Yay! r/sr=waterson
Might introduce a confusion with the CSS "display: inline-block". 
What about "SpecialIB" ?
I think that when 'display: inline-block' is a reality, and we choose to deal
with it in Mozilla, then all of our current 'specialIB' code is in jeopardy
anyway. Given that, and the fact that 'specialIB' is our current notion of
inline-block, I'd prefer to leave it. If you find it confusing, then I could
adopt another name, but can't we find one that does not have the work 'special'
in it - please?

BTW: we have another notion of special frames, referring to firstletter and
firstline stuff. I cannot think of a better name for that either.
Except this splitting business is really very different from 'inline-block' (and
frankly, probably harder to do).
But even once |display: inline-block| is defined, we'll still need to do the
frame-tree splitting if someone tries to put a |display: block| element in a
|display: inline| element. (Barring major re-thinking of this solution, that is.)

How about...

  s/NS_FRAME_IS_SPECIAL/NS_FRAME_BLOCK_INLINE_SPLIT/
  s/IsFrameSpecial/IsFrameBlockInlineSplit/
  s/GetSpecialSibling/GetBlockInlineSplitSibling/
  s/special/block-in-inline split/ (for comments)
  etc...

Great suggestion - I like it.

RE: the notion of the frame-tree splitting in the presence of inline-block, I
wonder if the implementation of inline-block could be desigend to handle as well
this tricky case? I'd really hate to have the splitting code we have now PLUS
the additional complexity of the formal inline-block stuff - there _seems_ to be
some similarities from the layout perspective, but I have not really thought
about it much. Later on that, I like your suggested names and should not confuse.
Target Milestone: --- → Future
.
Assignee: attinasi → misc
Status: ASSIGNED → NEW
Component: Layout → Layout: Misc Code
QA Contact: cpetersen0953 → nobody
Target Milestone: Future → ---
.
Assignee: misc → block-and-inline
Component: Layout: Misc Code → Layout: Block & Inline
QA Contact: nobody → ian
Assignee: layout.block-and-inline → nobody
QA Contact: ian → layout.block-and-inline
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Patches 1 and 2 were written with sed, with hand fixup of whitespace afterwards.

Patches 3, 4, 5, 6, and 7 were written entirely with sed.

Patches 8 and 9 were written by hand.
So part 1 and 2 use the term BlockInInlineSplit, and part 3 to 7
is using the term IBSplit.  I don't see why they should use
different terms - can we settle on one please?  BlockInInlineSplit
is a mouthful to read, so I'd prefer IBSplit.  So I'd suggest
NS_FRAME_BLOCK_IN_IBSPLIT or NS_FRAME_IBSPLIT_BLOCK for part 1, 
and IsFramePartOfIBSplit for part 2.
Flags: needinfo?(dbaron)
(In reply to Mats Palmgren (:mats) from comment #23)
> So part 1 and 2 use the term BlockInInlineSplit, and part 3 to 7
> is using the term IBSplit.  I don't see why they should use
> different terms - can we settle on one please?  BlockInInlineSplit
> is a mouthful to read, so I'd prefer IBSplit.  So I'd suggest
> NS_FRAME_BLOCK_IN_IBSPLIT or NS_FRAME_IBSPLIT_BLOCK for part 1, 
> and IsFramePartOfIBSplit for part 2.

I'm ok with your suggestion for part 2, but that doesn't work for part 1 because the bit is set on both blocks and inlines.  We could use NS_FRAME_PART_OF_IBSPLIT though.
Flags: needinfo?(dbaron)
Thanks, that sounds good to me.
Attachment #8369777 - Attachment is obsolete: true
Attachment #8369777 - Flags: review?(matspal)
Attachment #8369778 - Attachment is obsolete: true
Attachment #8369778 - Flags: review?(matspal)
Comment on attachment 8371911 [details] [diff] [review]
patch 1:  Rename NS_FRAME_IS_SPECIAL to NS_FRAME_PART_OF_IB_SPLIT.

r=mats with s/IB_SPLIT/IBSPLIT/  (I'm assuming it's a typo)
Attachment #8371911 - Flags: review?(matspal) → review+
Attachment #8371912 - Flags: review?(matspal) → review+
Attachment #8369780 - Flags: review?(matspal) → review+
Attachment #8369781 - Flags: review?(matspal) → review+
Attachment #8369783 - Flags: review?(matspal) → review+
Attachment #8369784 - Flags: review?(matspal) → review+
Comment on attachment 8369786 [details] [diff] [review]
patch 8:  Miscellaneous function and variable name changes.

FYI, there's a IsFramePartOfBlockInInlineSplit here that needs to be
renamed.  r=mats
Attachment #8369786 - Flags: review?(matspal) → review+
Attachment #8369787 - Flags: review?(matspal) → review+
Attachment #8369782 - Flags: review?(matspal) → review+
I think it's clearer to have the underscore; "split" is a word, and it ought to stay separated somehow.
The reason I want the same term is mostly for search, so one can query MXR, or grep
over a source tree, or search in an editor for "ibsplit" and it would find all the
places you're changing here.  I don't really care about the bit name per se.
Can we make IsFramePartOfIBSplit (in part 2) a method on nsIFrame and replace the
explicit bit checks with that?
OK, I'll drop the underscore.

I'd rather not do any more refactoring now; we don't need to make everything perfect related to ib-splits all at once and ignore all the other needed cleanup.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: