1388975 - Overhaul handling of nsWindowSizes

Status: RESOLVED DUPLICATE of bug 1387956

(Core :: DOM: Core & HTML, enhancement)

Description

[Nicholas Nethercote [inactive]]

I have some clean-ups that are precursors to some Stylo memory reporting work.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Change |nsWindowSizes*| arguments to |nsWindowSizes&|

This makes it clear they are never null.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Overhaul handling of nsWindowSizes

This patch does the following.

- Moves nsWindowSizes from nsWindowMemoryReporter.h to its own file,
  nsWindowSizes.h, so it can be included more widely without exposing
  nsWindowMemoryReporter.

- Merges nsArenaMemoryStats.h (which defines nsTabSizes and nsArenaMemoryStats)
  into nsWindowSizes.h.

- Renames nsArenaMemoryStats as nsArenaSizes, and nsWindowSizes::mArenaStats as
  nsWindowSizes::mArenaSizes. This is the more usual naming scheme for such
  types.

- Renames FRAME_ID_STAT_FIELD as NS_ARENA_SIZES_FIELD.

- Passes nsWindowSizes to PresShell::AddSizeOfIncludingThis() and
  nsPresArena::AddSizeOfExcludingThis(), instead of a bunch of smaller things.
  One nice consequence is that the odd nsArenaMemoryStats::mOther field is no
  longer necessary, because we can update nsWindowSizes::mLayoutPresShellSize
  directly in nsPresArena::AddSizeOfExcludingThis().

- Adds |const| to a few methods.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8895662 [details] [diff] [review]
(part 1) - Change |nsWindowSizes*| arguments to |nsWindowSizes&|

Review of attachment 8895662 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds reasonable.

Comment 4

[Nicholas Nethercote [inactive]]

mccr8: this blocks bug 1387956, which is high priority for Stylo. If you could review ASAP that would be very helpful. Thank you.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> mccr8: this blocks bug 1387956, which is high priority for Stylo. If you
> could review ASAP that would be very helpful. Thank you.

I've started reviewing it. You combined a bunch of changes into a single patch, which makes it difficult to review. I'm also not familiar with this code.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2, split up

I was having a lot of trouble understanding what was going on in part 2, so for my own edification I split the patch into 5 separate patches. These patches implement, in order, each of the bullet points in njn's list in the commit message for part 2 (the last two are merged together, because the last one is quite simple), and I mostly derived it exactly from njn's final patch, so the combined end result is identical to the original patch, modulo a few things I will mention as review comments. Feel free to use this or ignore this as you want, but I figured I might as well upload this in case anybody else was interested. The intermediate patches compile, but I haven't tested them beyond that.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8895663 [details] [diff] [review]
(part 2) - Overhaul handling of nsWindowSizes

Review of attachment 8895663 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsWindowMemoryReporter.cpp
@@ +373,2 @@
>                "Memory used by line boxes within a window.");
> +  aWindowTotalSizes->mArenaSizes.mLineBoxes

This is an example of a diff that would be much easier to understand in MozReview. :)

::: dom/base/nsWindowSizes.h
@@ +4,5 @@
> + * 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/. */
> +
> +#ifndef nsWindowSizes_h
> +#define nsWindowSizes_h

This file should have includes. I think it needs:
#include "mozilla/Assertions.h"
#include "mozilla/PodOperations.h"
#include "mozilla/SizeOfState.h"

::: layout/base/PresShell.h
@@ +375,5 @@
>    }
>  
>    virtual void LoadComplete() override;
>  
> +  void AddSizeOfIncludingThis(nsWindowSizes& aWindowSizes) const override;

Does this need to be marked "virtual"?

::: layout/base/nsPresArena.h
@@ +95,5 @@
>    void ClearArenaRefPtrs(mozilla::ArenaObjectID aObjectID);
>  
>    /**
>     * Increment aArenaStats with sizes of interesting objects allocated in this
>     * arena and its mOther field with the size of everything else.

This comment should be updated: mOther does not exist any more, and the argument is not named aArenaStats.

@@ +97,5 @@
>    /**
>     * Increment aArenaStats with sizes of interesting objects allocated in this
>     * arena and its mOther field with the size of everything else.
>     */
>    void AddSizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf,

In the PresShell version of this method, you dropped the MallocSizeOf argument and instead extracted it from the nsWindowSize argument, but here you left it in. Is there some reason for the inconsistency? (Feel free to leave it as is, I just thought I'd mention it.)

Comment 8

[Nicholas Nethercote [inactive]]

Thank you for the review. Apologies for the reviewing difficulty; I clearly underestimated the complexity of this refactoring.

Comment 9

[Nicholas Nethercote [inactive]]

I messed up the bug numbers and accidentally landed these patches under bug 1387956. So I'll dup this bug to that one.