Closed Bug 1383837 Opened 2 years ago Closed 2 years ago

Keep about:telemetry state when reloading

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: flyingrub, Assigned: flyingrub)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

We should keep :
* the previous section where we were before the reload
* the previous search filter

For the section we could use an anchor in the url like about:telemetry#general-section.
We should also keep the search filter when changing parent / content processes selector.
Comment on attachment 8889632 [details]
Bug 1383837 - Remove old statebox

https://reviewboard.mozilla.org/r/160652/#review166414
Attachment #8889632 - Flags: review?(chutten) → review+
Comment on attachment 8889633 [details]
Bug 1383837 - Remove unused code in about:telemetry

https://reviewboard.mozilla.org/r/160654/#review166416

I do so enjoy patches that remove unused code.
Attachment #8889633 - Flags: review?(chutten) → review+
Comment on attachment 8889634 [details]
Bug 1383837 - Save the state after a reload in about:telemetry

https://reviewboard.mozilla.org/r/160656/#review166420

This appears to be incomplete, as though it will only work for home and doesn't yet have a complete restore story

::: toolkit/content/aboutTelemetry.js:1864
(Diff revision 1)
> +function changeUrlPath(selectedSection, subSection) {
> +  if (subSection) {
> +    let hash = window.location.hash.split("_")[0] + "_" +  selectedSection;
> +    window.location.hash = hash;
> +  } else {
> +    window.location.hash = selectedSection.replace("-section", "-tab");

why are you performing this substitution? It seems as though the only purpose is so that you must reverse it when you restore.
Attachment #8889634 - Flags: review?(chutten) → review-
Comment on attachment 8889634 [details]
Bug 1383837 - Save the state after a reload in about:telemetry

https://reviewboard.mozilla.org/r/160656/#review166420

> why are you performing this substitution? It seems as though the only purpose is so that you must reverse it when you restore.

It's needed to prevent the page to scroll to the anchor point.
Comment on attachment 8890986 [details]
Bug 1383837 - Remove unused classes in about:telemetry

https://reviewboard.mozilla.org/r/162166/#review167442

::: commit-message-e3b91:1
(Diff revision 1)
> +Bug 1383837 - Remove unused class in about:telemetry r?chutten

s/class/classes

::: commit-message-e3b91:2
(Diff revision 1)
> +Bug 1383837 - Remove unused class in about:telemetry r?chutten
> +

You're also sneaking in some not-class-removal-related changes. You should at least mention them :)
Attachment #8890986 - Flags: review?(chutten) → review-
Comment on attachment 8889634 [details]
Bug 1383837 - Save the state after a reload in about:telemetry

https://reviewboard.mozilla.org/r/160656/#review166420

> It's needed to prevent the page to scroll to the anchor point.

What do you think about using the History API instead? (https://developer.mozilla.org/en/docs/Web/API/History and https://developer.mozilla.org/en-US/docs/Web/API/History_API)
Comment on attachment 8890986 [details]
Bug 1383837 - Remove unused classes in about:telemetry

https://reviewboard.mozilla.org/r/162166/#review167456
Attachment #8890986 - Flags: review?(chutten) → review+
Comment on attachment 8889634 [details]
Bug 1383837 - Save the state after a reload in about:telemetry

https://reviewboard.mozilla.org/r/160656/#review166420

> What do you think about using the History API instead? (https://developer.mozilla.org/en/docs/Web/API/History and https://developer.mozilla.org/en-US/docs/Web/API/History_API)

and don't edit the url at all then ?
Comment on attachment 8891017 [details]
Bug 1383837 - Keep search state in about:telemetry

https://reviewboard.mozilla.org/r/162186/#review167460

::: commit-message-669e1:3
(Diff revision 1)
> +Bug 1383837 - Keep search state in about:telemetry r?chutten
> +
> +Keep the state of the search when switching between processes.

This isn't what this patch does, though, is it? This seems more like a fixup for patch 3 which is where the state saving actually happens... this is making sure we restore the search state after loading the page.
Attachment #8891017 - Flags: review?(chutten)
Comment on attachment 8889634 [details]
Bug 1383837 - Save the state after a reload in about:telemetry

https://reviewboard.mozilla.org/r/160656/#review166420

> and don't edit the url at all then ?

It will edit the url, it just won't trigger the navigation or "scroll-to-anchor" code

(and it gives us handy event handling (onpopstate) for lightweight back/forward transitions)

If it doesn't make sense, it's fine not to use. It seems like a cool thing to learn about that happens to be designed to solve the "I want to save state in the URL but not induce navigation or scrolling" problem
Comment on attachment 8891017 [details]
Bug 1383837 - Keep search state in about:telemetry

https://reviewboard.mozilla.org/r/162186/#review167460

> This isn't what this patch does, though, is it? This seems more like a fixup for patch 3 which is where the state saving actually happens... this is making sure we restore the search state after loading the page.

It is what this patch does. Previously the state was restored in the `show(selected)` function. I could squash them together if you want ?
Comment on attachment 8891017 [details]
Bug 1383837 - Keep search state in about:telemetry

https://reviewboard.mozilla.org/r/162186/#review167460

> It is what this patch does. Previously the state was restored in the `show(selected)` function. I could squash them together if you want ?

The message says "keep the state" whereas this patch is about making sure we restore from the state that was kept by patch 3. Just a wording thing... but hey, if you're offering to squash it into patch 3, I'm happy with that too :)
Comment on attachment 8891017 [details]
Bug 1383837 - Keep search state in about:telemetry

https://reviewboard.mozilla.org/r/162186/#review167460

> The message says "keep the state" whereas this patch is about making sure we restore from the state that was kept by patch 3. Just a wording thing... but hey, if you're offering to squash it into patch 3, I'm happy with that too :)

Path 3 is saving the state after a reload whereas this one is when switching between processes. But let's squash them :)
Attachment #8891017 - Attachment is obsolete: true
Comment on attachment 8889634 [details]
Bug 1383837 - Save the state after a reload in about:telemetry

https://reviewboard.mozilla.org/r/160656/#review167818
Attachment #8889634 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ebc577da908c
Remove old statebox r=chutten
https://hg.mozilla.org/integration/autoland/rev/f8637a01b5c8
Remove unused code in about:telemetry r=chutten
https://hg.mozilla.org/integration/autoland/rev/85ed2d9aef85
Save the state after a reload in about:telemetry r=chutten
https://hg.mozilla.org/integration/autoland/rev/960d5cf9540c
Remove unused classes in about:telemetry r=chutten
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.