Closed Bug 1217662 Opened 4 years ago Closed 4 years ago

clean up places that unnecessarily include Layers.h

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(6 files)

Layers.h triggers some include chains that result in gfx headers touching lots
of unrelated code (files in xpconnect, dom/html, accessible, various DOM
bindings code, etc.).  But by and large, everything in Layers.h can be forward
declared until you absolutely need to use it.  Shuffling some code around makes
the include chains significantly shorter.
LayerManagerUserDataDestroy is a static function in Layers.h that we
only ever take a pointer to, to use it as a destruction function for
gfx::UserData.  It's *possible* the compiler is smart enough to call it
directly, rather than through the function pointer stored in
gfx::UserData, but that seems highly unlikely.  And
LayerManagerUserDataDestroy does a virtual call anyway, so it's not as
though it being inlined is particularly important.

All of this is to say that we don't need to define
LayerManagerUserDataDestroy in Layers.h; defining it out-of-line will be
just as effective.  Defining it out-of-line also means that we don't
need the definition of LayerUserData anywhere in Layers.h, and we can
move LayerUserData to a separate header.
Attachment #8677786 - Flags: review?(matt.woodrow)
Having to include all of Layers.h just to get at the definition of
LayerUserData is inconvenient, especially as most of the interesting
things in Layers.h can be forward-declared.  Let's move LayerUserData to
its own header, so clients can include a small header for that,
forward-declare anything else they need from Layers.h, and reduce header
bloat.

Ideally, we'd forward-declare LayerUserData in Layers.h; we'll get there, but
we need some other changes first.  The forward declaration will come in a later
patch.
Attachment #8677787 - Flags: review?(matt.woodrow)
This method is virtual, so defining it inline isn't worth a whole lot.
Defining it inline also means that we require the complete definition of
LayerManager from Layers.h, and we're trying to avoid including Layers.h
whenever possible.  Let's move it out-of-line to solve this problem.
Attachment #8677788 - Flags: review?(matt.woodrow)
Attachment #8677786 - Flags: review?(matt.woodrow) → review+
Having these out-of-line means that we don't have to have
LayerUserData's destructor in scope for the destruction of the local
nsAutoPtr.  This change will enable us to forward-declare LayerUserData
in Layers.h and thereby encourage people to use the correct header for
LayerUserData.
Attachment #8677789 - Flags: review?(matt.woodrow)
This change means that we don't have to have the definition of
LayerManager visible when FrameLayerBuilder is declared (which would be
necessary for, e.g. the ctor/dtor of ClippedDisplayItem), which means we
can get rid of the Layers.h include here in a subsequent patch.
Attachment #8677790 - Flags: review?(matt.woodrow)
This change necessitates a few other header changes around the tree:
either places that we relying on FrameLayerBuilder.h to #include
ImageLayers.h for them, or places that were bootlegging headers from
ImageLayers.h.  Now we can forward-declare LayerUserData in Layers.h!
Attachment #8677791 - Flags: review?(matt.woodrow)
Attachment #8677787 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8677788 [details] [diff] [review]
part 3 - move nsDisplayBlendContainer::GetLayerState out-of-line

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

::: layout/base/nsDisplayList.cpp
@@ +4148,5 @@
> +    return mozilla::LAYER_ACTIVE;
> +  }
> +  return mozilla::LAYER_INACTIVE;
> +}
> +  

Trailing whitespace!
Attachment #8677788 - Flags: review?(matt.woodrow) → review+
Attachment #8677789 - Flags: review?(matt.woodrow) → review+
Attachment #8677790 - Flags: review?(matt.woodrow) → review+
Attachment #8677791 - Flags: review?(matt.woodrow) → review+
You need to log in before you can comment on or make changes to this bug.