Closed Bug 1323202 Opened 7 years ago Closed 7 years ago

Factor out the timeouts linked list into a separate data structure

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

This is in preparation of splitting the timeouts list into normal and
tracking timeouts.
Blocks: 1312514
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)
(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)
Thanks Ben.  I'll file follow-ups for further refactoring of this stuff.
Flags: needinfo?(bkelly)
Assignee: nobody → ehsan
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
https://hg.mozilla.org/mozilla-central/rev/fda9283e646d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: