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

RESOLVED FIXED in Firefox 62

Status

enhancement
P3
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: jdescottes, Assigned: ladybenko)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
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)

Updated

a year ago
Assignee: nobody → balbeza
Status: NEW → ASSIGNED
(Reporter)

Updated

11 months ago
Summary: Application panel: show last-modified file timestamp → Application panel: show last updated time for a given service worker registration
(Reporter)

Comment 2

11 months ago
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
(Reporter)

Updated

11 months ago
Blocks: 1450070
Comment hidden (mozreview-request)
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+
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

11 months ago
Pushed by balbeza@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7cf5547bb7a
Show last update timestamp for a sw. r=nchevobbe

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a7cf5547bb7a
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

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