Closed Bug 839839 Opened 7 years ago Closed 7 years ago

Minor nsPlaceholderFrame code cleanup

Categories

(Core :: Layout, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(1 file)

No description provided.
Attached patch fixSplinter Review
nsPlaceholderFrame.cpp changes, from top to bottom:

* remove descriptive comment - it can be found in header

* #include nsPlaceholderFrame.h first, separate it from the rest by one
  line, remove unnecessary includes, sort the rest in alphabetical order

* remove empty dtor, it's not needed

* remove GetMinWidth, GetPrefWidth because they are identical to nsFrame
  versions which we inherit

* s/Type *arg/Type* arg/ for consistency with surrounding code

* store the frame manager in a local to make the code less verbose,
  don't null-check it

* remove trailing space

* fix too wide columns

* make BuildDisplayList more readable

* remove old DEBUG_waterson code and s/nullptr != X/X/

nsPlaceholderFrame.h:
  * make it slightly more readable and make nsPlaceholderFrame MOZ_FINAL
    and a couple of missing MOZ_OVERRIDE

====

No functional change is intended except for the removed null-check
of the frame manager because it should always outlive all frames.
Attachment #712222 - Flags: review?(dholbert)
Comment on attachment 712222 [details] [diff] [review]
fix

>+++ b/layout/generic/nsPlaceholderFrame.cpp
>@@ -1,57 +1,30 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>-/*
>- * rendering object for the point that anchors out-of-flow rendering
>- * objects such as floats and absolutely positioned elements
>- */

I'd keep this comment -- it doesn't hurt, and it's handy for MXR, which grabs the first non-license-boilerplate comment and displays it alongside the filename in file listings, e.g. here:
  https://mxr.mozilla.org/mozilla-central/source/layout/generic/

> public:
[...]
>-  nsPlaceholderFrame(nsStyleContext* aContext, nsFrameState aTypeBit) :
>-    nsFrame(aContext)
>+  nsPlaceholderFrame(nsStyleContext* aContext, nsFrameState aTypeBit)
>+    : nsFrame(aContext)
>   {
>     NS_PRECONDITION(aTypeBit == PLACEHOLDER_FOR_FLOAT ||
>                     aTypeBit == PLACEHOLDER_FOR_ABSPOS ||
>                     aTypeBit == PLACEHOLDER_FOR_FIXEDPOS ||
>                     aTypeBit == PLACEHOLDER_FOR_POPUP,
>                     "Unexpected type bit");
>     AddStateBits(aTypeBit);
>   }

While you're cleaning this up, it'd actually be worth moving the constructor down to the "protected" section, to enforce that nsPlaceholderFrame can only be instantiated via NS_NewPlaceholderFrame (and not on the stack or via 'new').  That could happen in a separate patch/bug if you prefer, though.

>   /**
>-   * Get the "type" of the frame
>-   *
>-   * @see nsGkAtoms::placeholderFrame
>+   * @return nsGkAtoms::placeholderFrame
>    */
>   virtual nsIAtom* GetType() const MOZ_OVERRIDE;

There's something about a pseudocode-only javadoc comment that seems odd to me. :) I'd probably just drop the comment entirely, rather than leaving it with _just_ a @return statement.

r=me with the above.
Attachment #712222 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #2)
> I'd keep this comment -- it doesn't hurt, and it's handy for MXR

Fair enough, restored the comment.

> [...] to enforce that nsPlaceholderFrame can only be instantiated
> via NS_NewPlaceholderFrame (and not on the stack or via 'new').

I'll address this in a separate patch.

> I'd probably just drop the comment entirely, rather than leaving it
> with _just_ a @return statement.

I think it's essential to have the value documented in the header so
I reverted this change.
https://hg.mozilla.org/mozilla-central/rev/bf5d5599aa6a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(In reply to Mats Palmgren [:mats] from comment #3)
> > [...] to enforce that nsPlaceholderFrame can only be instantiated
> > via NS_NewPlaceholderFrame (and not on the stack or via 'new').
> 
> I'll address this in a separate patch.

Following up on this -- did this end up getting a new bug or a patch somewhere?  (not a huge deal, but worthwhile cleanup to do, IMHO; just want to make sure we don't lose track of it)
Target Milestone: mozilla21 → ---
You need to log in before you can comment on or make changes to this bug.