RequestListContent could instanciate RequestListContextMenu lazily

RESOLVED FIXED in Firefox 61

Status

P2
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: ochameau, Assigned: glowka.tom)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Today we instanciate RequestListContextMenu on componentWillMount:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListContent.js#65-71
It forces loading its module immediately.

But I think we can delay its load and instanciation in onContextMenu function:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListContent.js#200-203

So that we speed up netmonitor opening.
(Assignee)

Comment 1

a year ago
Hey, this seems pretty clear, just replacing pure require with lazyGetter, right? I could do that, if it makes sense.
Flags: needinfo?(odvarko)
(In reply to glowka.tom from comment #1)
> Hey, this seems pretty clear, just replacing pure require with lazyGetter,
> right? I could do that, if it makes sense.

lazyGetter is not available in this scope. But, AFAIU this is about moving the `new RequestListContextMenu({...})` code from componentWillMount to onContextMenu.

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 3

a year ago
OK, so please assign it to me.
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8959561 - Flags: review?(odvarko)
Assignee: nobody → glowka.tom
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Comment on attachment 8959561 [details]
Bug 1420291 - instanciate RequestListContextMenu lazily

https://reviewboard.mozilla.org/r/228362/#review234686

Looks good, thanks for the patch!

R+ assuming try is green

Honza
Attachment #8959561 - Flags: review?(odvarko) → review+
Also, please rebase on top of m-c before landing.
Honza
Btw. you already know you need to set checkin-needed, right?

Honza
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> Also, please rebase on top of m-c before landing.
> Honza

Rebased.

(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> Btw. you already know you need to set checkin-needed, right?
> 
> Honza

Yeah, but good to be reminded, thanks!
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 10

a year ago
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e63bab1349dd
instanciate RequestListContextMenu lazily r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e63bab1349dd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

9 months ago
Product: Firefox → DevTools
Depends on: 1467522
You need to log in before you can comment on or make changes to this bug.