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

This makes it clear they are never null.

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 on attachment 8895662 [details] [diff] [review] (part 1) - Change |nsWindowSizes*| arguments to |nsWindowSizes&| Review of attachment 8895662 [details] [diff] [review]: ----------------------------------------------------------------- Sounds reasonable.

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

(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.

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 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.)

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

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