Closed Bug 1450067 Opened 2 years ago Closed 2 years ago

Application panel: show last updated time for a given service worker registration

Categories

(DevTools :: Application Panel, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: jdescottes, Assigned: ladybenko)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Follow up to Bug 1445197. See mockups in https://projects.invisionapp.com/share/ZQGIRLJ2KBU#/screens/287275492_Artboard

The last updated time should be displayed below the source.
Mass move to the new application panel component.
Component: Developer Tools → Developer Tools: Application Panel
Assignee: nobody → balbeza
Status: NEW → ASSIGNED
Summary: Application panel: show last-modified file timestamp → Application panel: show last updated time for a given service worker registration
I needed a minimal implementation of this bug to help me test Bug 1450070. This  might help you get started, so attaching my WIP patch here. Main things from this: the lastUpdateTime is already exposed from the ServiceWorkerRegistrationInfo [1], so we can add it to the form of our ServiceWorkerRegistration actor [2]. 

[1] https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/dom/interfaces/base/nsIServiceWorkerManager.idl#91

[2] https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/devtools/server/actors/worker.js#298-309
Blocks: 1450070
Comment on attachment 8980254 [details]
Bug 1450067 - Show last update timestamp for a sw.

https://reviewboard.mozilla.org/r/246418/#review252788

Thanks Belén
I have a few minor comments but this looks good to me. 
I played with the patch for a bit and it seems to work well.

::: devtools/client/application/src/components/Worker.js:126
(Diff revision 1)
> +    let hours = `${d.getHours()}`.padStart(2, "0");
> +    let minutes = `${d.getMinutes()}`.padStart(2, "0");
> +    let seconds = `${d.getSeconds()}`.padStart(2, "0");
> +    let milli = `${d.getMilliseconds()}`.padStart(3, "0");
> +
> +    return `${hours}:${minutes}:${seconds}.${milli} ${year}-${month}-${day}`;

Can you tell me what is the rationale for putting the time before the date ? It feel a bit odd (probably because I'm not used to it), but I guess that's something you want for some reason :)
Maybe we can also put a comment explaining why we format things this way (not sure about that though)

::: devtools/client/application/src/components/Worker.js:168
(Diff revision 1)
>              span({ title: worker.scope, className: "js-source-url" },
>                this.formatSource(worker.url)),
> -            debugLink),
> +            debugLink,
> +            br({}),
> +            span({ className: "worker__data__updated" },
> +              "Updated ",
> +              time({ className: "js-sw-updated"},
> +                this.formatTimestamp(worker.lastUpdateTime) || "N/A"))),

this is getting a bit hard to read, maybe we could move this to a `renderWorker` function ?

::: devtools/client/application/src/components/Worker.js:171
(Diff revision 1)
> +            br({}),
> +            span({ className: "worker__data__updated" },
> +              "Updated ",
> +              time({ className: "js-sw-updated"},
> +                this.formatTimestamp(worker.lastUpdateTime) || "N/A"))),

In which case worker.lastUpdateTime is falsy ?

In those case, maybe we could not display the br|updated elements , or having another label that reflects the state of the worker when the lastUpdateTime is falsy
Attachment #8980254 - Flags: review?(nchevobbe) → review+
Comment on attachment 8980254 [details]
Bug 1450067 - Show last update timestamp for a sw.

https://reviewboard.mozilla.org/r/246418/#review252788

> Can you tell me what is the rationale for putting the time before the date ? It feel a bit odd (probably because I'm not used to it), but I guess that's something you want for some reason :)
> Maybe we can also put a comment explaining why we format things this way (not sure about that though)

Hi! thanks for the review :) This format was something that appeared in our wireframes, but after speaking with Julian we have decided to go for a normal `toString()`

> In which case worker.lastUpdateTime is falsy ?
> 
> In those case, maybe we could not display the br|updated elements , or having another label that reflects the state of the worker when the lastUpdateTime is falsy

I'm not sure in which cases it might be empty, but it was something that "patch draft" that Julian uploaded handled, so I decided to keep it. You are right it might make more sense to not render the whole thing at all if this is the case.
Pushed by balbeza@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7cf5547bb7a
Show last update timestamp for a sw. r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/a7cf5547bb7a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.