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)
Core
Layout: Block and Inline
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.
Comment 1•23 years ago
|
||
Thanks for creating the bug. Since I'm the disgruntled one, I'll take it.
Assignee: waterson → attinasi
Comment 2•23 years ago
|
||
Comment 3•23 years ago
|
||
Well, the patch compiled :-) Pretty simple editor jockeying really.
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•23 years ago
|
||
Yay! r/sr=waterson
Might introduce a confusion with the CSS "display: inline-block". What about "SpecialIB" ?
Comment 6•23 years ago
|
||
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.
Assignee | ||
Comment 7•23 years ago
|
||
Except this splitting business is really very different from 'inline-block' (and frankly, probably harder to do).
Reporter | ||
Comment 8•23 years ago
|
||
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...
Comment 9•23 years ago
|
||
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.
Updated•23 years ago
|
Target Milestone: --- → Future
Comment 10•21 years ago
|
||
.
Assignee: attinasi → misc
Status: ASSIGNED → NEW
Component: Layout → Layout: Misc Code
QA Contact: cpetersen0953 → nobody
Target Milestone: Future → ---
Assignee | ||
Comment 11•21 years ago
|
||
.
Assignee: misc → block-and-inline
Component: Layout: Misc Code → Layout: Block & Inline
QA Contact: nobody → ian
Updated•15 years ago
|
Assignee: layout.block-and-inline → nobody
QA Contact: ian → layout.block-and-inline
Assignee | ||
Comment 12•10 years ago
|
||
https://groups.google.com/d/topic/mozilla.dev.tech.layout/Cmvapo3fWeg/discussion has discussion from July 2013.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8369777 -
Flags: review?(matspal)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8369778 -
Flags: review?(matspal)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8369780 -
Flags: review?(matspal)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8369781 -
Flags: review?(matspal)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8369782 -
Flags: review?(matspal)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8369783 -
Flags: review?(matspal)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8369784 -
Flags: review?(matspal)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8369786 -
Flags: review?(matspal)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8369787 -
Flags: review?(matspal)
Assignee | ||
Comment 22•10 years ago
|
||
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.
Comment 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
(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)
Comment 25•10 years ago
|
||
Thanks, that sounds good to me.
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8371911 -
Flags: review?(matspal)
Assignee | ||
Updated•10 years ago
|
Attachment #8369777 -
Attachment is obsolete: true
Attachment #8369777 -
Flags: review?(matspal)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8371912 -
Flags: review?(matspal)
Assignee | ||
Updated•10 years ago
|
Attachment #8369778 -
Attachment is obsolete: true
Attachment #8369778 -
Flags: review?(matspal)
Comment 28•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8371912 -
Flags: review?(matspal) → review+
Updated•10 years ago
|
Attachment #8369780 -
Flags: review?(matspal) → review+
Updated•10 years ago
|
Attachment #8369781 -
Flags: review?(matspal) → review+
Updated•10 years ago
|
Attachment #8369783 -
Flags: review?(matspal) → review+
Updated•10 years ago
|
Attachment #8369784 -
Flags: review?(matspal) → review+
Comment 29•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8369787 -
Flags: review?(matspal) → review+
Updated•10 years ago
|
Attachment #8369782 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 30•10 years ago
|
||
I think it's clearer to have the underscore; "split" is a word, and it ought to stay separated somehow.
Comment 31•10 years ago
|
||
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?
Assignee | ||
Comment 32•10 years ago
|
||
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.
Assignee | ||
Comment 33•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8dbff705562 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/82bf6b59e477 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/28819a989013 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/070e165dd4c9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/26b793631c6e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a724b647685 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3edd2f23e992 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1731313882e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/55bc35c4c65f
Comment 34•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e8dbff705562 https://hg.mozilla.org/mozilla-central/rev/82bf6b59e477 https://hg.mozilla.org/mozilla-central/rev/28819a989013 https://hg.mozilla.org/mozilla-central/rev/070e165dd4c9 https://hg.mozilla.org/mozilla-central/rev/26b793631c6e https://hg.mozilla.org/mozilla-central/rev/6a724b647685 https://hg.mozilla.org/mozilla-central/rev/3edd2f23e992 https://hg.mozilla.org/mozilla-central/rev/a1731313882e https://hg.mozilla.org/mozilla-central/rev/55bc35c4c65f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•