Closed Bug 1286468 Opened 7 years ago Closed 7 years ago

Rename line related functions in nsBlockFrame to conform to Mozilla coding style

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- affected
firefox52 --- fixed

People

(Reporter: TYLin, Assigned: chenpighead)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1286468 - Remove the old vertical alignment coding style.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1286468 - Rename line related typedefs in nsBlockFrame.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1286468 - Rename line related functions in nsBlockFrame.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1286468 - Trim trailing whitspaces in nsBlockFrame.h and nsBlockFrame.cpp.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Summary: Rename line related functions nsBlockFrame to conform to Mozilla coding style → Rename line related functions in nsBlockFrame to conform to Mozilla coding style
Priority: -- → P3
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #0)
> Rename |begin_lines| to |BeginLines|, and |end_lines| to |EndLines|, etc.
> 
> http://searchfox.org/mozilla-central/rev/
> c44d0b1673fef5e0e2e19fa82d6780a74c186151/layout/generic/nsBlockFrame.h#87-96

I think |BeginLine| would be better than |BeginLines| since the function return the begin of a nsLineList, which no doubt is singular. I do get confused by these names a little bit while tracing codes around here.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Comment on attachment 8802988 [details]
Bug 1286468 - Remove the old vertical alignment coding style.

https://reviewboard.mozilla.org/r/87206/#review86292

Hi Mats, I found few whitespaces that needs to be purged while checking my patches. I decided to make a separated patch, and put it in the last part. Normally I shall probably put it before the real change patches... :-/

Please let me know if this arrangement would distract you while doing the review. If so, I could upload another version tomorrow.
Comment on attachment 8802988 [details]
Bug 1286468 - Remove the old vertical alignment coding style.

https://reviewboard.mozilla.org/r/87206/#review86470

Hi Mats, considering this is just some renaming and coding style change, I'd like to forward the review request to Ting-Yu. Please feel free to comment/feedback.
Attachment #8802988 - Flags: review?(mats) → review?(tlin)
Attachment #8802989 - Flags: review?(mats) → review?(tlin)
Attachment #8802990 - Flags: review?(mats) → review?(tlin)
Attachment #8802991 - Flags: review?(mats) → review?(tlin)
Comment on attachment 8802988 [details]
Bug 1286468 - Remove the old vertical alignment coding style.

https://reviewboard.mozilla.org/r/87208/#review86482

::: layout/generic/nsBlockFrame.h:337
(Diff revision 1)
>    /**
>     * Creates a contination for aFloat and adds it to the list of overflow floats.
>     * Also updates aState.mReflowStatus to include the float's incompleteness.
>     * Must only be called while this block frame is in reflow.
>     * aFloatStatus must be the float's true, unmodified reflow status.
> -   * 
> +   *

Nit: Let's kill this empty line.
Attachment #8802988 - Flags: review?(tlin) → review+
Comment on attachment 8802989 [details]
Bug 1286468 - Rename line related typedefs in nsBlockFrame.

https://reviewboard.mozilla.org/r/87210/#review86490

::: layout/generic/nsBlockFrame.cpp:2789
(Diff revision 1)
>    }
>  }
>  
>  nsIFrame*
>  nsBlockFrame::PullFrame(BlockReflowInput& aState,
> -                        line_iterator       aLine)
> +                        LineIterator       aLine)

I just realize that the part 1 only clean up the header :)
Attachment #8802989 - Flags: review?(tlin) → review+
Comment on attachment 8802990 [details]
Bug 1286468 - Rename line related functions in nsBlockFrame.

https://reviewboard.mozilla.org/r/87212/#review86492

Looks good to me!
Attachment #8802990 - Flags: review?(tlin) → review+
Comment on attachment 8802991 [details]
Bug 1286468 - Trim trailing whitspaces in nsBlockFrame.h and nsBlockFrame.cpp.

https://reviewboard.mozilla.org/r/87230/#review86496

I found other whitespaces in both the header and cpp. You might want to your text editor's command/plugin/etc to nuke all the whitespace in the file at once.

r=me with the comment above.
Attachment #8802991 - Flags: review?(tlin) → review+
Comment on attachment 8802991 [details]
Bug 1286468 - Trim trailing whitspaces in nsBlockFrame.h and nsBlockFrame.cpp.

https://reviewboard.mozilla.org/r/87230/#review86498

::: layout/generic/nsBlockFrame.h:598
(Diff revision 1)
>  
>    // Remove a float, abs, rel positioned frame from the appropriate block's list
>    static void DoRemoveOutOfFlowFrame(nsIFrame* aFrame);
>  
>    /** set up the conditions necessary for an resize reflow
>      * the primary task is to mark the minimumly sufficient lines dirty. 

Exmaple: this whitespace not purged.
Comment on attachment 8802988 [details]
Bug 1286468 - Remove the old vertical alignment coding style.

https://reviewboard.mozilla.org/r/87208/#review86482

> Nit: Let's kill this empty line.

Will do.
Comment on attachment 8802991 [details]
Bug 1286468 - Trim trailing whitspaces in nsBlockFrame.h and nsBlockFrame.cpp.

https://reviewboard.mozilla.org/r/87230/#review86496

Will use a plugin to do this once for all. Thank you for the review. :)
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/061d2183312b
Remove the old vertical alignment coding style. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/cdd81ce1b841
Rename line related typedefs in nsBlockFrame. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/741c66d1e10e
Rename line related functions in nsBlockFrame. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/9639e2670d5e
Trim trailing whitspaces in nsBlockFrame.h and nsBlockFrame.cpp. r=TYLin
Fwiw, I think aligning parameters vertically improves readability, for example:

  void ComputeFinalSize(const ReflowInput& aReflowInput,
                        BlockReflowInput&  aState,
                        ReflowOutput&      aMetrics,
                        nscoord*           aBottomEdgeOfChildren);

is more readable than:

  void ComputeFinalSize(const ReflowInput& aReflowInput,
                        BlockReflowInput& aState,
                        ReflowOutput& aMetrics,
                        nscoord* aBottomEdgeOfChildren);
(In reply to Mats Palmgren (:mats) from comment #21)
> Fwiw, I think aligning parameters vertically improves readability, for
> example:
> 
>   void ComputeFinalSize(const ReflowInput& aReflowInput,
>                         BlockReflowInput&  aState,
>                         ReflowOutput&      aMetrics,
>                         nscoord*           aBottomEdgeOfChildren);
> 
> is more readable than:
> 
>   void ComputeFinalSize(const ReflowInput& aReflowInput,
>                         BlockReflowInput& aState,
>                         ReflowOutput& aMetrics,
>                         nscoord* aBottomEdgeOfChildren);

To be honest, I'm not strongly against the vertical alignment.
The thing is, while tracing codes, seeing two different styles (w/ and w/o vertical alignment) in one file do distracts me a little bit. Maybe this could be a coding style issue that worth to be raised in the dev-platform channel?
These should really be BeginLines and EndLines, not BeginLine and EndLine, since they're the Begin and End iterator operations (as for C++ containers) for the lines of the block.  Could you put the s back?
Flags: needinfo?(jeremychen)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #23)
> These should really be BeginLines and EndLines, not BeginLine and EndLine,
> since they're the Begin and End iterator operations (as for C++ containers)
> for the lines of the block.  Could you put the s back?

Hi David, could you enlighten me a bit more? I'd like to do any followup fix, as long as I can confirm what your concern is.

It's just the plural naming seems a bit confused to me since a singular iterator object is returned from BeginLines (and EndLines). I'm not sure what's the meaning of putting 's' here.

Are you saying that the original BeginLines stands for BeginLineIteratorOpertions?

Or, you're saying that the original BeginLines stands for BeginOfTheLines?
Flags: needinfo?(jeremychen) → needinfo?(dbaron)
They're iterators for 'mLines', that's where the "lines" comes from.

Perhaps LinesBegin / LinesEnd would be clearer?
I'm saying that in English, "first" and "last" are used with a single item in the collection (e.g., "first child") while "begin" and "end" are used with the collection as a whole (e.g., "begin children").
Flags: needinfo?(dbaron)
(In reply to Mats Palmgren (:mats) from comment #25)
> They're iterators for 'mLines', that's where the "lines" comes from.
> 
> Perhaps LinesBegin / LinesEnd would be clearer?

I prefer BeginLines() to LinesBegin() since it's valid English.  But I'd be OK with LinesBegin() since it's sort of like a shorthand for Lines().Begin(), which would make sense.

(But it should use the word "Begin" since it works the way C++ containers work, and they use begin() and end().)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #27)
> (In reply to Mats Palmgren (:mats) from comment #25)
> > They're iterators for 'mLines', that's where the "lines" comes from.
> > 
> > Perhaps LinesBegin / LinesEnd would be clearer?
> 
> I prefer BeginLines() to LinesBegin() since it's valid English.  But I'd be
> OK with LinesBegin() since it's sort of like a shorthand for
> Lines().Begin(), which would make sense.
> 
> (But it should use the word "Begin" since it works the way C++ containers
> work, and they use begin() and end().)

Mats and David, thank you for the explanation.
Using LinesBegin()/LinesEnd() makes sense to me. I'll push a followup then.
You need to log in before you can comment on or make changes to this bug.