Closed Bug 1115812 Opened 10 years ago Closed 9 years ago

make RebuildAllStyleData better share existing restyle processing code

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(19 files)

patch 1 - Remove obsolete comment that was fixed by bug 1047928
1.43 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 2 - Move the eRestyle_ChangeAnimationPhaseDescendants hint in DoRebuildAllStyleData so that the new rebuild-all codepaths will keep it
3.55 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 3 - Pass the hints to DoRebuildAllStyleData via the member variables, in preparation for future refactoring
8.27 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 4 - Rename mRebuildAllStyleData to mDoRebuildAllStyleData
4.89 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 5 - Store the state of whether we're currently rebuilding all style data in a member variable, to prepare for future merging of the rebuild into other code
4.33 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 6 - Move ProcessRestyles from RestyleTracker to RestyleManager
5.83 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 7 - Always call DoProcessRestyles if mInRebuildAllStyleData
1.34 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 8 - Call BeginProcessingRestyles and EndProcessingRestyles from DoRebuildAllStyleData
2.45 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 9 - Move the end part of the rebuild-all process to RestyleManager::EndProcessingRestyles
2.80 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 10 - Add RestyleTracker parameter to BeginProcessingRestyles
3.47 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 11 - Move the beginning part of the rebuild-all process to StartRebuildAllStyleData
7.51 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 12 - Null-check the root frame in StartRebuildAllStyleData
2.33 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 13 - Make RebuildAllStyleData use the normal ProcessPendingRestyles() codepath
3.26 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 14 - For rem unit change handling, repost min hint to root just like change hint
1.66 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 15 - Change the rebuild-all that we do for 'rem' unit changes to use the new way
2.74 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 16 - Remove the now-unused DoRebuildAllStyleData
3.54 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 17 - Do animation-only update properly for a rebuild all
1.49 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 18 - We only need to call ProcessPendingRestyles recursively, rather than RebuildAllStyleData
1.52 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 19 - Avoid bad kungFuDeathGrip pattern
1.92 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
bug 1110277 patch 6 adds a FIXME in RebuildAllStyleData, noting that the initial bug isn't completely fixed in the RebuildAllStyleData case, because the scope of the ReframingStyleContexts is wrong.

To fix this, we should refactor things so that RebuildAllStyleData uses the standard restyle processing rather than rolling its own; it's been a bit of a pain for quite a while since there are always two places to fix.
Summary: fix scope of ReframingStyleContexts in RebuildAllStyleData by making RebuildAllStyleData better use existing restyle processing → make RebuildAllStyleData better share existing restyle processing code
The patches in this series refactor the process of rebuilding all style
data (RestyleManager::RebuildAllStyleData and
RestyleManager::DoRebuildAllStyleData) so that the process of rebuilding
all style data uses the existing restyle processing loops in
ProcessPendingRestyles.  (Rebuilding all style data is what we do when
we need to throw away the rule tree because something has invalidated
the cached data in it.)  This removes (increasing, especially with bug
960465 coming) code duplicated between the two codepaths, fixes some
omissions from the separate rebuild-all codepath, and (more immediately)
allows fixing lifetime issues of ReframingStyleContexts objects in bug
1110277 so that we can have a single ReframingStyleContexts for all of
the restyle processing in each restyle processing operation.  In other
words, the goal is to change the rebuild-all process from a separate
codepath to a few variables that modify the way ProcessPendingRestyles
works (and make it do the extra work).

This is just a small first step in that process, which moves one piece
of code from a chunk of duplicated and to-be-removed code into a chunk
of code that will be preserved.
Attachment #8542419 - Flags: review?(cam)
Part of this refactoring involves the ability to start the rebuild-all
process within the processing of restyles.  This means we can't pass
parameters directly from RebuildAllStyleData into DoRebuildAllStyleData.
So this continues storing the hints as member variables a little bit
deeper into the process.

(I tried to move in a different direction in this patch queue, and store
these hints in mPendingRestyles, for the root element.  But that broke
layout/style/test/test_counter_style.html and
layout/style/test/test_font_loading_api.html, and I didn't want to
figure out why.  It would be somewhat better in the long run, since
currently these hints will get processed if we do a rebuild-all on a
RestyleTracker other than mPendingRestyles, which can happen if we have
'rem' units and have a root element font size change in the
animation-only update or in mPendingAnimationRestyles.)
Attachment #8542420 - Flags: review?(cam)
This is the variable that says we *need to* rebuild style data.  Since
the next patch will introduce a variable that says we're *currently*
rebuilding all style data, renaming this one makes things clearer.
Attachment #8542421 - Flags: review?(cam)
This adds a member variable that is currently only used within a single
function, but that function will be split apart so that different parts
of it can be called from different places within ProcessPendingRestyles.
Attachment #8542422 - Flags: review?(cam)
This is needed for the following patch, so that it can access a member
variable of RestyleManager.
Attachment #8542423 - Flags: review?(cam)
This is needed for patch 9 (once patch 9 is used via the
ProcessPendingRestyles() codepath); it ensures that when we use the new
way of rebuilding, we don't bail out early because we think we have
nothing to do.
Attachment #8542424 - Flags: review?(cam)
This fixes one of the omissions in the rebuild-all codepaths (where it
incorrectly differs from the regular ProcessPendingRestyles codepath).
Note that the explicit FlushOverflowChangedTracker() is no longer needed
because that's part of EndProcessingRestyles.

(This will all get refactored more substantially in the following
patches.)
Attachment #8542425 - Flags: review?(cam)
This moves the code that finishes the rebuild-all process into
EndProcessingRestyles(), which is part of the main restyling codepath.

Patch 7 ensures that we'll always get to EndProcessingRestyles in this
case, when we're going through the normal ProcessPendingRestyles()
codepath rather than the special DoRebuildAllStyleData() codepath (which
will be removed later in this patch series).
Attachment #8542426 - Flags: review?(cam)
Here we call StartRebuildAllStyleData from BeginProcessingRestyles (much
like patch 9 and EndProcessingRestyles).  But we will later also call it
from the code that handles a root element font size change when we have
'rem' units.  That's because it's fine to *start* the rebuild process in
the middle of processing the queue of pending restyles.  (We have to end
after the whole process is done, though, in order to avoid wanting to
destroy the old rule tree while we still have style contexts referencing
it.)

We only call StartRebuildAllStyleData in this case when we're processing
our primary restyle queue (mPendingRestyles), not the animation restyles
(to be removed in bug 960465) or the animation-only restyles, since a
rebuild-all should be processed (in terms of animation phases, or in
terms of having an animation-only update before it) like a normal
restyle.  (This isn't true for the 'rem' unit restyle, which could
happen during any sort of update.)
Attachment #8542429 - Flags: review?(cam)
In the new way of doing a rebuild-all, StartRebuildAllStyleData might be
called directly from ProcessPendingRestyles rather than from
RebuildAllStyleData (which null-checks the root frame) or from within
processing restyles (which can only happen when there's a root frame).
This means it needs its own null-check of the root frame.
Attachment #8542430 - Flags: review?(cam)
This switches RebuildAllStyleData() to the normal
ProcessPendingRestyles() manner of restyle processing.  This means a
rebuild-all going through this codepath (the main rebuild-all codepath)
only sets up for non-animation restyle processing once rather than doing
it twice (and potentially having reframes posted in
DoRebuildAllStyleData() that don't get processed until
ProcessPendingRestyles(), which causes a variant of bug 1110277 with
transitions on reframed elements failing to start because it doesn't
match the lifetime of the ReframingStyleContexts).
Attachment #8542431 - Flags: review?(cam)
This changes what was probably a silly design choice when I wrote the
code for 'rem'-basis handling; we shouldn't try continuing through the
rest of RestyleElement() here, but instead repost the hint to the
rebuild-all process.
Attachment #8542432 - Flags: review?(cam)
This means that instead of recurring into DoRebuildAllStyleData, we'll
call StartRebuildAllStyleData in the middle of processing the restyle
queue (which is fine).  StartRebuildAllStyleData will move the old rule
tree out of the way and immediately do a full-tree restyle, before
returning to any queue processing that might be left (the full-tree
restyle should have consumed all remaining restyle hints, but might have
posted some new ones for handling reframes that require reframing
ancestors).  And, more importantly, the EndReconstruct() call to get rid
of the old rule tree won't happen until after we're done processing the
containing RestyleTracker's queue of restyles, which reduces the risk of
having dangling old style contexts and makes it easier (in bug 1110277)
to have a ReframingStyleContexts with the right lifetime.
Attachment #8542433 - Flags: review?(cam)
This fixes another pre-existing bug in the rebuild-all codepath; it
didn't handle the animation-only update correctly, which could have
caused bugs in transitions with OMT animations enabled.
Attachment #8542435 - Flags: review?(cam)
If we discover that we've set mDoRebuildAllStyleData in the middle of
ProcessPendingRestyles(), now that ProcessPendingRestyles() fully
handles mDoRebuildAllStyleData, we only need to make a recursive call to
ProcessPendingRestyles, rather than calling RebuildAllStyleData to call
ProcessPendingRestyles.
Attachment #8542436 - Flags: review?(cam)
If we need a strong reference to the pres shell, we may as well do it
the first time rather than bothering with an extra variable.
Attachment #8542437 - Flags: review?(cam)
Comment on attachment 8542422 [details] [diff] [review]
patch 5 - Store the state of whether we're currently rebuilding all style data in a member variable, to prepare for future merging of the rebuild into other code

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

::: layout/base/RestyleManager.h
@@ +466,5 @@
>  private:
>    nsPresContext* mPresContext; // weak, disconnected in Disconnect
>  
>    bool mDoRebuildAllStyleData : 1;
> +  bool mInRebuildAllStyleData : 1;

These two could have a comment explaining the difference.
Attachment #8542418 - Flags: review?(cam) → review+
Comment on attachment 8542419 [details] [diff] [review]
patch 2 - Move the eRestyle_ChangeAnimationPhaseDescendants hint in DoRebuildAllStyleData so that the new rebuild-all codepaths will keep it

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

Since RestyleManager::RestyleElement calls DoRebuildAllStyleData (if we're restyling the root and the font-size has changed), were we doing something wrong with animations and rem units that this fixes?
(In reply to Cameron McCormack (:heycam) from comment #23)
> Since RestyleManager::RestyleElement calls DoRebuildAllStyleData (if we're
> restyling the root and the font-size has changed), were we doing something
> wrong with animations and rem units that this fixes?

I think it probably does, actually, although constructing a testcase might be difficult.  Without that change, we might end up with animation styles still around when our phase says we should have non-animation styles.  (The whole issue goes away with bug 960465, which I'm hoping is actually soon now.)
Attachment #8542419 - Flags: review?(cam) → review+
Comment on attachment 8542420 [details] [diff] [review]
patch 3 - Pass the hints to DoRebuildAllStyleData via the member variables, in preparation for future refactoring

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

::: layout/base/RestyleManager.cpp
@@ +948,5 @@
>        if (oldContext->StyleFont()->mFont.size !=
>            newContext->StyleFont()->mFont.size) {
>          // The basis for 'rem' units has changed.
>          newContext = nullptr;
> +        mRebuildAllRestyleHint |= aRestyleHint;

Can mRebuildAllRestyleHint actually be anything other than 0 at this point?
Attachment #8542420 - Flags: review?(cam) → review+
Comment on attachment 8542421 [details] [diff] [review]
patch 4 - Rename mRebuildAllStyleData to mDoRebuildAllStyleData

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

::: layout/base/RestyleManager.h
@@ +465,5 @@
>  
>  private:
>    nsPresContext* mPresContext; // weak, disconnected in Disconnect
>  
> +  bool mDoRebuildAllStyleData : 1;

Per Ms2ger's comment, please add a comment for this bit.
Attachment #8542421 - Flags: review?(cam) → review+
Attachment #8542422 - Flags: review?(cam) → review+
Attachment #8542423 - Flags: review?(cam) → review+
Attachment #8542425 - Flags: review?(cam) → review+
Comment on attachment 8542424 [details] [diff] [review]
patch 7 - Always call DoProcessRestyles if mInRebuildAllStyleData

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

::: layout/base/RestyleManager.h
@@ +466,5 @@
>    void ProcessRestyles(RestyleTracker& aRestyleTracker) {
>      // Fast-path the common case (esp. for the animation restyle
>      // tracker) of not having anything to do.
> +    if (aRestyleTracker.Count() ||
> +        mInRebuildAllStyleData) {

Can you explain further why we need to call into DoProcessRestyles if there are no pending restyles but mInRebuildAllStyleData is true?  I can't tell from reading patch 9 why that is.
(In reply to Cameron McCormack (:heycam) from comment #27)
> Can you explain further why we need to call into DoProcessRestyles if there
> are no pending restyles but mInRebuildAllStyleData is true?  I can't tell
> from reading patch 9 why that is.

Patch 11 makes me think this should actually be mDoRebuildAllStyleData?
Attachment #8542429 - Flags: review?(cam) → review+
Attachment #8542427 - Flags: review?(cam) → review+
Attachment #8542430 - Flags: review?(cam) → review+
Attachment #8542431 - Flags: review?(cam) → review+
Attachment #8542432 - Flags: review?(cam) → review+
Attachment #8542433 - Flags: review?(cam) → review+
Attachment #8542434 - Flags: review?(cam) → review+
Comment on attachment 8542435 [details] [diff] [review]
patch 17 - Do animation-only update properly for a rebuild all

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

::: layout/base/RestyleManager.cpp
@@ +1589,5 @@
>    // running entirely on the compositor thread) is up-to-date, so that
>    // if any style changes we cause trigger transitions, we have the
>    // correct old style for starting the transition.
>    if (nsLayoutUtils::AreAsyncAnimationsEnabled() &&
> +      (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData)) {

This (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData) is used in a couple of places.  Maybe make a function for it?
Attachment #8542435 - Flags: review?(cam) → review+
Attachment #8542436 - Flags: review?(cam) → review+
Attachment #8542437 - Flags: review?(cam) → review+
Can we assert somewhere that both mDoRebuildAllStyleData and mInRebuildAllStyleData are false by the end of processing everything?
I think I find the naming of StartRebuildAllStyleData odd, because there's no corresponding EndRebuildAllStyleData.  It also actually does the building of all the style data, so it's not just starting the process.  I'm tempted to say it should be called DoRebuildAllStyleData.
(In reply to Cameron McCormack (:heycam) from comment #25)
> Can mRebuildAllRestyleHint actually be anything other than 0 at this point?

I'm not sure; there are some cases that can post restyles from the middle of the restyle processing queue (handling the change hints); it's possible some of them might be a rebuild-all, although I hope not.

(In reply to Cameron McCormack (:heycam) from comment #27)
> Comment on attachment 8542424 [details] [diff] [review]
> patch 7 - Always call DoProcessRestyles if mInRebuildAllStyleData
> 
> Review of attachment 8542424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/RestyleManager.h
> @@ +466,5 @@
> >    void ProcessRestyles(RestyleTracker& aRestyleTracker) {
> >      // Fast-path the common case (esp. for the animation restyle
> >      // tracker) of not having anything to do.
> > +    if (aRestyleTracker.Count() ||
> > +        mInRebuildAllStyleData) {
> 
> Can you explain further why we need to call into DoProcessRestyles if there
> are no pending restyles but mInRebuildAllStyleData is true?  I can't tell
> from reading patch 9 why that is.

Because patches 9, 11, and 13 move the rebuilding to stuff that happens inside DoProcessRestyles; following those patches, if we bail out of ProcessRestyles, then no rebuilding will happen.  (In the interim states, part of the work will happen and part won't.)

(In reply to Cameron McCormack (:heycam) from comment #28)
> (In reply to Cameron McCormack (:heycam) from comment #27)
> > Can you explain further why we need to call into DoProcessRestyles if there
> > are no pending restyles but mInRebuildAllStyleData is true?  I can't tell
> > from reading patch 9 why that is.
> 
> Patch 11 makes me think this should actually be mDoRebuildAllStyleData?

No, since we don't transform mDoRebuildAllStyleData into mInRebuildAllStyleData until BeginProcessingRestyles, which is called *inside* DoProcessRestyles.


(In reply to Cameron McCormack (:heycam) from comment #30)
> Can we assert somewhere that both mDoRebuildAllStyleData and
> mInRebuildAllStyleData are false by the end of processing everything?

Perhaps, although I'm not sure it will be true.  (I don't think this patch changes how harmful it is or whether it would happen; it would mean that things aren't completely flushed in response to a flush, but I don't think we'd lose changes.)  It's probably worth asserting if the assertion holds.

(In reply to Cameron McCormack (:heycam) from comment #31)
> I think I find the naming of StartRebuildAllStyleData odd, because there's
> no corresponding EndRebuildAllStyleData.  It also actually does the building
> of all the style data, so it's not just starting the process.  I'm tempted
> to say it should be called DoRebuildAllStyleData.

The EndRebuildAllStyleData is really just the call to EndReconstruct in RestyleManager::EndProcessingRestyles.  I'd rather move that (and maybe the mInRebuildAllStyleData = false) into a function called FinishRebuildAllStyleData than rename StartRebuildAllStyleData to DoRebuildAllStyleData.
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #32)
> > ::: layout/base/RestyleManager.h
> > @@ +466,5 @@
> > >    void ProcessRestyles(RestyleTracker& aRestyleTracker) {
> > >      // Fast-path the common case (esp. for the animation restyle
> > >      // tracker) of not having anything to do.
> > > +    if (aRestyleTracker.Count() ||
> > > +        mInRebuildAllStyleData) {
> > 
> > Can you explain further why we need to call into DoProcessRestyles if there
> > are no pending restyles but mInRebuildAllStyleData is true?  I can't tell
> > from reading patch 9 why that is.
> 
> Because patches 9, 11, and 13 move the rebuilding to stuff that happens
> inside DoProcessRestyles; following those patches, if we bail out of
> ProcessRestyles, then no rebuilding will happen.  (In the interim states,
> part of the work will happen and part won't.)

I am getting muddled.  As of patch 9, what is the control flow that sets mInRebuildAllStyleData to true and then calls ProcessRestyles (before EndProcessingRestyles, which sets it to false again)?

> (In reply to Cameron McCormack (:heycam) from comment #30)
> > Can we assert somewhere that both mDoRebuildAllStyleData and
> > mInRebuildAllStyleData are false by the end of processing everything?
> 
> Perhaps, although I'm not sure it will be true.  (I don't think this patch
> changes how harmful it is or whether it would happen; it would mean that
> things aren't completely flushed in response to a flush, but I don't think
> we'd lose changes.)  It's probably worth asserting if the assertion holds.

It would be a bug, right, if we didn't end up processing the mDoRebuildAllStyleData request?

I think at least we should assert !mInRebuildAllStyleData by the end.

> (In reply to Cameron McCormack (:heycam) from comment #31)
> > I think I find the naming of StartRebuildAllStyleData odd, because there's
> > no corresponding EndRebuildAllStyleData.  It also actually does the building
> > of all the style data, so it's not just starting the process.  I'm tempted
> > to say it should be called DoRebuildAllStyleData.
> 
> The EndRebuildAllStyleData is really just the call to EndReconstruct in
> RestyleManager::EndProcessingRestyles.  I'd rather move that (and maybe the
> mInRebuildAllStyleData = false) into a function called
> FinishRebuildAllStyleData than rename StartRebuildAllStyleData to
> DoRebuildAllStyleData.

Yes that sounds fine to me.
(In reply to Cameron McCormack (:heycam) from comment #33)
> I am getting muddled.  As of patch 9, what is the control flow that sets
> mInRebuildAllStyleData to true and then calls ProcessRestyles (before
> EndProcessingRestyles, which sets it to false again)?

It's not actually required until patch 13.  (I think that's what I meant by the "once patch 9 is used via the ProcessPendingRestyles() codepath" in the commit message in patch 7.)

(I reorganized the patch series after writing it to make it so that each step would still be fully functional and so that all of the steps would be simple; I tried to rewrite the commit messages appropriately afterwards, but I might not have gotten everything right.)

> > (In reply to Cameron McCormack (:heycam) from comment #30)
> > > Can we assert somewhere that both mDoRebuildAllStyleData and
> > > mInRebuildAllStyleData are false by the end of processing everything?
> > 
> > Perhaps, although I'm not sure it will be true.  (I don't think this patch
> > changes how harmful it is or whether it would happen; it would mean that
> > things aren't completely flushed in response to a flush, but I don't think
> > we'd lose changes.)  It's probably worth asserting if the assertion holds.
> 
> It would be a bug, right, if we didn't end up processing the
> mDoRebuildAllStyleData request?
> 
> I think at least we should assert !mInRebuildAllStyleData by the end.

Yes, it would be a bug.  I can see if the assertion sticks, although I'm not sure if it will.
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #34)
> It's not actually required until patch 13.  (I think that's what I meant by
> the "once patch 9 is used via the ProcessPendingRestyles() codepath" in the
> commit message in patch 7.)

OK.  Anyway, since it's replaced in patch 11 (with the check for mDoRebuildAllStyleData) I guess it doesn't matter.  And I'm happy with how it is in patch 13.

> (I reorganized the patch series after writing it to make it so that each
> step would still be fully functional and so that all of the steps would be
> simple; I tried to rewrite the commit messages appropriately afterwards, but
> I might not have gotten everything right.)

I appreciate the small individual patch sizes.

> > It would be a bug, right, if we didn't end up processing the
> > mDoRebuildAllStyleData request?
> > 
> > I think at least we should assert !mInRebuildAllStyleData by the end.
> 
> Yes, it would be a bug.  I can see if the assertion sticks, although I'm not
> sure if it will.

Sounds good.
Attachment #8542424 - Flags: review?(cam) → review+
Attachment #8542426 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #29)
> This (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData) is used in a
> couple of places.  Maybe make a function for it?

I'm not going to do this.  They're slightly different already (which isn't a big deal), so I could just turn the patch into:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/9fff4311386a/rebuild-all-animation-only-update
but they're going to become more different in bug 960465, which means I'd just need to revert the change there, in:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/767e8b20d633/flush-animation-only-before-restyle
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #36)
> (In reply to Cameron McCormack (:heycam) from comment #29)
> > This (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData) is used in a
> > couple of places.  Maybe make a function for it?
> 
> I'm not going to do this.  [...] they're going to become more different in bug 960465

OK.
(In reply to Cameron McCormack (:heycam) from comment #35)
> (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from
> comment #34)
> > It's not actually required until patch 13.  (I think that's what I meant by
> > the "once patch 9 is used via the ProcessPendingRestyles() codepath" in the
> > commit message in patch 7.)
> 
> OK.  Anyway, since it's replaced in patch 11 (with the check for
> mDoRebuildAllStyleData) I guess it doesn't matter.  And I'm happy with how
> it is in patch 13.

Yeah, I think patch 7 is probably a vestige of how I originally wrote the patch series, and shouldn't really exist as a separate patch, but rather just be replaced by what replaces it in patch 11.

That said, I've done a bunch of testing that everything works each step of the patch series that I'd rather not redo, so I'm inclined to just leave things as they are.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: