Closed Bug 926907 Opened 6 years ago Closed 6 years ago

Handle push updates in batches

Categories

(Tree Management Graveyard :: TBPL, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Swatinem, Assigned: Swatinem)

References

Details

Attachments

(1 file)

Attached patch batch.patchSplinter Review
We can avoid a lot of redundant UserInterface__updateFailingBuildsDisplay calls if we process pushes in batches.
Attachment #817157 - Flags: review?(emorley)
Comment on attachment 817157 [details] [diff] [review]
batch.patch

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

::: js/UserInterface.js
@@ +420,5 @@
>    },
>  
>    _updateUnstarredFilter: function UserInterface__updateOnlyStarredFilter(state) {
> +    if (state == this._onlyUnstarred)
> +      return;

Just as a funny side-note: This change means that we used to redraw everything if we filter by pusher (no redraw needed) or we redraw everything TWICE if we filtered by machine.
Comment on attachment 817157 [details] [diff] [review]
batch.patch

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

Yet another nice win! :-)

(r+ with the one change below)

::: js/UserInterface.js
@@ +420,5 @@
>    },
>  
>    _updateUnstarredFilter: function UserInterface__updateOnlyStarredFilter(state) {
> +    if (state == this._onlyUnstarred)
> +      return;

Ah good spot - we hit this function every time any of the other params change, even if onlyUnstarred didn't... whoops! :-)

@@ +468,2 @@
>      var pushes = Object.values(this._data.getPushes());
> +    this.handleUpdatedPush(pushes);

s/handleUpdatedPush/handleUpdatedPushes/
Attachment #817157 - Flags: review?(emorley) → review+
(In reply to Ed Morley [:edmorley UTC+1] from comment #2)
> @@ +468,2 @@
> >      var pushes = Object.values(this._data.getPushes());
> > +    this.handleUpdatedPush(pushes);
> 
> s/handleUpdatedPush/handleUpdatedPushes/

Ooops, that was the only filter I didn’t manually test, good catch.
Landed with that change.
Depends on: 931759
In production :-)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.