Factor out the timeouts linked list into a separate data structure

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
This is in preparation of splitting the timeouts list into normal and
tracking timeouts.
(Assignee)

Updated

a year ago
Blocks: 1312514
(Assignee)

Comment 1

a year ago
Created attachment 8818270 [details] [diff] [review]
Factor out the timeouts linked list into a separate data structure
Attachment #8818270 - Flags: review?(bkelly)
Comment on attachment 8818270 [details] [diff] [review]
Factor out the timeouts linked list into a separate data structure

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

Can any more of the larger operations be moved into the list class?  For example, it seems Insert() should be possible.

Right now it feels like this refactor does not accomplish much.  The external TimeoutManager class is still calling internal list node details like getNext() which suggests they are not very separated.

Do you feel this patch helps your work as is?

::: dom/base/TimeoutManager.cpp
@@ +735,3 @@
>    Timeout* prevSibling;
> +  for (prevSibling = mTimeouts.GetLast();
> +       prevSibling && prevSibling != mTimeouts.InsertionPoint() &&

Can we move this entire method into Timeouts?  For example:

  enum class SortBy
  {
    TimeRemaining,
    TimeWhen
  };

  void
  Timeouts::Insert(Timeout* aTimeout, SortBy::When);

Then TimeoutManager would call:

  mTimeouts->Insert(aTimeout, mWindow.IsFrozen() ? SortBy::TimeRemaining
                                                 : SortBy::When);

::: dom/base/TimeoutManager.h
@@ +137,4 @@
>    // Each nsGlobalWindow object has a TimeoutManager member.  This reference
>    // points to that holder object.
>    nsGlobalWindow&             mWindow;
> +  Timeouts                    mTimeouts;

I find it a bit confusing that the TimeoutManager has mTimeouts and the Timeouts class also has an mTimeouts all in the same source files.  Could we use Timeouts::mTimeoutList to match the type?
Attachment #8818270 - Flags: review?(bkelly)
(Assignee)

Comment 3

a year ago
(In reply to Ben Kelly [:bkelly] from comment #2)
> Comment on attachment 8818270 [details] [diff] [review]
> Factor out the timeouts linked list into a separate data structure
> 
> Review of attachment 8818270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can any more of the larger operations be moved into the list class?  For
> example, it seems Insert() should be possible.

Yes.

> Right now it feels like this refactor does not accomplish much.  The
> external TimeoutManager class is still calling internal list node details
> like getNext() which suggests they are not very separated.
> 
> Do you feel this patch helps your work as is?

Well, I'm not quite done with this code yet.  I was working on moving more into this class.  The reason why I asked review on this patch was to get an r+ on this part so that I can avoid having to go back and change a whole bunch of stuff...  Iterating on this code is turning into a headache, since I have to keep going back and make changes to the earlier patches and everything on top of them over and over again.

So in the interest of being able to hopefully finish all of this stuff very soon can you please give me a good idea of exactly how you'd like these changes to be split up?  i.e., do you want to only review each patch when the full set is done?  Or are you OK with reviewing things incrementally as we go?

(Note that I'm personally optimizing for being able to finish redoing the original patch series as fast as possible since I'd like to make some progress on this project).

> ::: dom/base/TimeoutManager.h
> @@ +137,4 @@
> >    // Each nsGlobalWindow object has a TimeoutManager member.  This reference
> >    // points to that holder object.
> >    nsGlobalWindow&             mWindow;
> > +  Timeouts                    mTimeouts;
> 
> I find it a bit confusing that the TimeoutManager has mTimeouts and the
> Timeouts class also has an mTimeouts all in the same source files.  Could we
> use Timeouts::mTimeoutList to match the type?

Sure.
Flags: needinfo?(bkelly)

Updated

a year ago
Attachment #8818270 - Flags: review+
(Assignee)

Comment 4

a year ago
Thanks Ben.  I'll file follow-ups for further refactoring of this stuff.
Flags: needinfo?(bkelly)
(Assignee)

Updated

a year ago
Assignee: nobody → ehsan

Comment 5

a year ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fda9283e646d
Factor out the timeouts linked list into a separate data structure; r=bkelly

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fda9283e646d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.