Closed
Bug 1383837
Opened 7 years ago
Closed 7 years ago
Keep about:telemetry state when reloading
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: flyingrub, Assigned: flyingrub)
References
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8889632 [details] Bug 1383837 - Remove old statebox https://reviewboard.mozilla.org/r/160652/#review166414
Attachment #8889632 -
Flags: review?(chutten) → review+
Comment 6•7 years ago
|
||
mozreview-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 7•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review-reply |
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 16•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
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 18•7 years ago
|
||
mozreview-review |
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 19•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
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 21•7 years ago
|
||
mozreview-review-reply |
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 :)
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
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 :)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8891017 -
Attachment is obsolete: true
Comment 25•7 years ago
|
||
mozreview-review |
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
Comment 26•7 years ago
|
||
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
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ebc577da908c https://hg.mozilla.org/mozilla-central/rev/f8637a01b5c8 https://hg.mozilla.org/mozilla-central/rev/85ed2d9aef85 https://hg.mozilla.org/mozilla-central/rev/960d5cf9540c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•