1450717 - Remove unneeded / inconsistent arguments to nsCSSFrameConstructor.

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1450717: Remove unneeded / inconsistent arguments from nsCSSFrameConstructor.

Much in the spirit of bug 1442207.

They're not only unneeded, and cheap to get, but also we call them
inconsistently with the light DOM and flattened tree parent (like ContentRemoved
for display: contents), so they're really confusing, and kind of a footgun.

Review commit: https://reviewboard.mozilla.org/r/233042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233042/

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8964318 [details]
Bug 1450717: Remove unneeded / inconsistent arguments from nsCSSFrameConstructor.

https://reviewboard.mozilla.org/r/233042/#review239038

::: layout/base/PresShell.cpp:4508
(Diff revision 1)
> +  // We never call ContentAppended with a document as the container, so we can
> +  // assert that we have an nsIContent parent.
> +  MOZ_ASSERT(aFirstNewContent->GetParent());
> +  MOZ_ASSERT(aFirstNewContent->GetParent()->IsElement() ||
> +             aFirstNewContent->GetParent()->IsShadowRoot());
> +

Is there a reason these were moved to after the "if (!mDidInitialize)" early return?
If not, I'd prefer to leave them before, b/c I think these invariants should always hold.

::: layout/base/RestyleManager.h:154
(Diff revision 1)
>    // following sibling in addition to the old child.  |aContainer| must be
>    // non-null; when the container is null, no work is needed.  aFollowingSibling

Please update the comment.

::: layout/base/RestyleManager.h:160
(Diff revision 1)
>    // for some CharacterDataChanged.  |aContainer| must be non-null; when
>    // the container is null, no work is needed.

Please update the comment.

::: layout/base/RestyleManager.cpp:56
(Diff revision 1)
> +  MOZ_ASSERT(aFirstNewContent->GetParentNode());
> +  MOZ_ASSERT(aFirstNewContent->GetParent());

The first one is redundant so I think it should be removed to avoid confusing the reader.

::: layout/base/RestyleManager.cpp:374
(Diff revision 1)
> -RestyleManager::ContentRemoved(nsINode* aContainer,
> +RestyleManager::ContentRemoved(nsIContent* aOldChild,
> -                               nsIContent* aOldChild,
>                                 nsIContent* aFollowingSibling)
>  {

nit: might be worth adding a MOZ_ASSERT(aOldChild->GetParentNode()) here, for consistency with the other notification methods?

::: layout/base/nsCSSFrameConstructor.h:124
(Diff revision 1)
>    void CheckBitsForLazyFrameConstruction(nsIContent* aParent);
>  #else
>    void CheckBitsForLazyFrameConstruction(nsIContent*) {}
>  #endif
>  
>    // Issues a single ContentInserted for each child of aContainer in the range

Please update the comment.

::: layout/base/nsCSSFrameConstructor.h:261
(Diff revision 1)
>    // Like ContentInserted but handles inserting the children of aContainer in
>    // the range [aStartChild, aEndChild).  aStartChild must be non-null.

Please update the comment.

::: layout/base/nsCSSFrameConstructor.h:280
(Diff revision 1)
>      REMOVE_CONTENT,
>      REMOVE_FOR_RECONSTRUCTION,
>    };
>  
>    /**
>     * Recreate or destroy frames for aChild in aContainer.

Please update the comment.

::: layout/base/nsCSSFrameConstructor.h:1459
(Diff revision 1)
>    // If aParentContent's child aContent is a text node and
>    // doesn't have a frame, try to create a frame for it.

Please update the comment.

::: layout/base/nsCSSFrameConstructor.cpp:7094
(Diff revision 1)
>  
>  #ifdef DEBUG
>    if (gNoisyContentUpdates) {
>      printf("nsCSSFrameConstructor::ContentAppended container=%p "
>             "first-child=%p lazy=%d\n",
> -           static_cast<void*>(aContainer), aFirstNewContent,
> +           static_cast<void*>(aFirstNewContent->GetParent()),

nit: while we're here, we could remove the static_cast<void*>() too.  I don't think it's needed on any platforms.  Feel free to leave it though if you wish.

::: layout/base/nsCSSFrameConstructor.cpp:7472
(Diff revision 1)
> -           static_cast<void*>(aContainer),
> +           static_cast<void*>(aStartChild->GetParent()),
>             static_cast<void*>(aStartChild), static_cast<void*>(aEndChild),

nit: more static_cast<void*> to remove if you please

::: layout/base/nsCSSFrameConstructor.cpp:7525
(Diff revision 1)
>        IssueSingleInsertNofications(
> -        aContainer, aStartChild, aEndChild, InsertionKind::Sync);
> +        aStartChild, aEndChild, InsertionKind::Sync);

This looks like it might fit on one line now?

::: layout/base/nsCSSFrameConstructor.cpp:7985
(Diff revision 1)
> -           static_cast<void*>(aContainer),
> +           static_cast<void*>(aChild->GetParent()),
>             static_cast<void*>(aChild),
>             static_cast<void*>(aOldNextSibling));

ditto

::: layout/base/nsCSSFrameConstructor.cpp:8013
(Diff revision 1)
> -  // sufficient.  Due to how we process reframes, the content node might not
> -  // even be in our document by now.  So explicitly check whether the viewport's
> +  //
> +  // Due to how we process reframes, the content node might not even be in our

I'd prefer to not split the paragraph here, since the "Due to..." text is part of the "Detecting removal of a root..." context (IIUC), and that gets a bit lost now.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bededc45f35a
Remove unneeded / inconsistent arguments from nsCSSFrameConstructor. r=mats

Comment 4

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/bededc45f35a