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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file)
16.25 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
This is in preparation of splitting the timeouts list into normal and tracking timeouts.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8818270 -
Flags: review?(bkelly)
Comment 2•7 years ago
|
||
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•7 years 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•7 years ago
|
Attachment #8818270 -
Flags: review+
Assignee | ||
Comment 4•7 years ago
|
||
Thanks Ben. I'll file follow-ups for further refactoring of this stuff.
Flags: needinfo?(bkelly)
Assignee | ||
Updated•7 years ago
|
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
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fda9283e646d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•