To make it easier to share what you're looking at, we should include the search terms in the url the same way we include the section name.

I'd like to take up this bug, can I get assigned to this?

Certainly! Let me know if you have any questions.

Should the search query be seperated by a pound sign, like the section name?

Use whatever you think would be clear to people reading the URLs and easy for developers to maintain in the future.

I've added a function that does the job. I'm relatively new here, so please let me know how I could improve. Thanks!

Comment on attachment 8911544 [details] [diff] [review] bug_1396855.patch Review of attachment 8911544 [details] [diff] [review]: ----------------------------------------------------------------- Looking good so far! You don't need to set both feedback and review to the same person, it's more for asking someone else to take a quick look. Review is a deeper dive. This is one half of the necessary code. The second half is to place code within urlStateRestore so that loading an about:telemetry url that contains a search term will perform the search on load. ::: toolkit/content/aboutTelemetry.js @@ +1368,4 @@ > clearTimeout(this.idleTimeout); > } > this.idleTimeout = setTimeout(() => Search.search(e.target.value), FILTER_IDLE_TIMEOUT); > + changeUrlSearch(e.target.value); This isn't the only place search is called. It might be better to put this within Search.search itself to catch every case. Also, that will allow you to properly handle the case when the section that has been switched to doesn't actually have a search field :)

Thanks for the feedback, Chris! I'll make the necessary changes and submit another patch.

Comment on attachment 8911544 [details] [diff] [review] bug_1396855.patch Marking the old patch as obsolete.

Comment on attachment 8912741 [details] Bug 1396855 - about:telemetry : Added search terms in the URL fragment. https://reviewboard.mozilla.org/r/184050/#review189232 Thank you for your patch! This is a good first draft, I have mostly just small things to say about it. ::: commit-message-bc567:4 (Diff revision 1) > +Bug 1396855 - about:telemetry : Added search terms in the URL fragment. r=chutten > + > +The search from the url hash doesn't work, probably because of Bug 1403585. However, searching in particular sections works, since it ensures the sections load. > +For example, about:telemetry#environment-data-tab_search=searchTerms works, but about:telemetry#search=searchTerms and about:telemetry#environment-data-tab_partner_search=searchTerms do not. Before submitting you should fix it so about:telemetry#search=searchTerms works. Subsection you can't do anything about until bug 1403585 is taken care of, but home search is an important new feature of about:telemetry in Firefox 57. ::: toolkit/content/aboutTelemetry.js:1928 (Diff revision 1) > /** > + * Change the url according to the current search text > + */ > +function changeUrlSearch(searchText){ > + let currentHash = window.location.hash; > + let baseSearchString = "search="; If this is a constant, declare it with const and give it a NAME_IN_CAPS_AND_UNDERSCORES ::: toolkit/content/aboutTelemetry.js:1931 (Diff revision 1) > +function changeUrlSearch(searchText){ > + let currentHash = window.location.hash; > + let baseSearchString = "search="; > + let hash = ""; > + > + if(currentHash.includes(baseSearchString)){ This if/elseif/else block can be rewritten more concisely as let hashWithoutSearch = currentHash.split(baseSearchString)[0]; hash = hashWithoutSearch + baseSearchString + searchText.replace(/ /g, "+"); ::: toolkit/content/aboutTelemetry.js:1932 (Diff revision 1) > + let currentHash = window.location.hash; > + let baseSearchString = "search="; > + let hash = ""; > + > + if(currentHash.includes(baseSearchString)){ > + hash = currentHash.slice(0, currentHash.indexOf(baseSearchString)) + there's some end-of-line whitespace here that should be removed. ::: toolkit/content/aboutTelemetry.js:1933 (Diff revision 1) > + let baseSearchString = "search="; > + let hash = ""; > + > + if(currentHash.includes(baseSearchString)){ > + hash = currentHash.slice(0, currentHash.indexOf(baseSearchString)) + > + baseSearchString + searchText.replace(/ /g, "_"); We're using underscores as delimiters for subsections. Using them for spaces as well will likely cause more problems than it will solve. ::: toolkit/content/aboutTelemetry.js:1941 (Diff revision 1) > + hash = baseSearchString + searchText.replace(/ /g, "_"); > + } > + } else { > + hash = currentHash + "_" + baseSearchString + searchText.replace(/ /g, "_"); > + } > + A little more EOL whitespace ::: toolkit/content/aboutTelemetry.js:1942 (Diff revision 1) > + } > + } else { > + hash = currentHash + "_" + baseSearchString + searchText.replace(/ /g, "_"); > + } > + > + if(!searchText){ This can be combined as well with the earlier logic. if (!searchText) { hash = hashWithoutSearch; } ::: toolkit/content/aboutTelemetry.js:2114 (Diff revision 1) > -// Restore sections states > +// Restore sections states and search terms > function urlStateRestore() { > - if (window.location.hash) { > - let section = window.location.hash.slice(1).replace("-tab", "-section"); > + let hash = window.location.hash; > + let searchQuery = ""; > + if (hash) { > + if(hash.includes("search=")){ If baseSearchString were a constant (say, within the Search object, as Search.HASH_SEARCH or a better name) then you could use it here. ::: toolkit/content/aboutTelemetry.js:2115 (Diff revision 1) > function urlStateRestore() { > - if (window.location.hash) { > - let section = window.location.hash.slice(1).replace("-tab", "-section"); > + let hash = window.location.hash; > + let searchQuery = ""; > + if (hash) { > + if(hash.includes("search=")){ > + searchQuery = hash.slice(hash.indexOf("search=") + 7).replace(/_/g, " "); the 7 could be Search.HASH_SEARCH.length ::: toolkit/content/aboutTelemetry.js:2118 (Diff revision 1) > + if (hash) { > + if(hash.includes("search=")){ > + searchQuery = hash.slice(hash.indexOf("search=") + 7).replace(/_/g, " "); > + hash = hash.slice(0, hash.indexOf("search=")); > + } > + if(hash){ need a space between if and (. Run `./mach eslint -wo` on your machine to check the style of your submitted JavaScript. ::: toolkit/content/aboutTelemetry.js:2133 (Diff revision 1) > - showSubSection(subcategory); > + showSubSection(subcategory); > - } > + } > - } > + } > - } > + } > + let search = document.getElementById("search"); > + search.value = searchQuery; Does this result in a search happening, or the search string just being present in the input field?

Thank you for the patient feedback, Chris! I'll make sure the changes are made and update the patch. Also I'll keep in mind these tips for future patches in general.

(In reply to Chris H-C :chutten from comment #10) > Before submitting you should fix it so about:telemetry#search=searchTerms > works. Subsection you can't do anything about until bug 1403585 is taken > care of, but home search is an important new feature of about:telemetry in > Firefox 57. about:telemetry#search=searchTerms works with this patch. :) > This if/elseif/else block can be rewritten more concisely as > > let hashWithoutSearch = currentHash.split(baseSearchString)[0]; > hash = hashWithoutSearch + baseSearchString + searchText.replace(/ /g, "+"); > Since the function also had to ensure that the underscores that delimit the search and subsection portions of the hashes, I have tried to make the if/else constructs as concise as possible, while ensuring minimal bugs. Please let me know if there's anything I could do better. > there's some end-of-line whitespace here that should be removed. I've run my code through eslint this time. :) > We're using underscores as delimiters for subsections. Using them for spaces > as well will likely cause more problems than it will solve. Changed the delimiters to '+'. > Does this result in a search happening, or the search string just being > present in the input field? Yes, since search is implemented dynamically with an eventListener, it results in the search occuring, rather than just changing the value of the input. Please let me know if there are any more issues. Thanks. :)

Comment on attachment 8912741 [details] Bug 1396855 - about:telemetry : Added search terms in the URL fragment. https://reviewboard.mozilla.org/r/184050/#review190190 Just a little whitespace problem here, and then it should be good to go. ::: toolkit/content/aboutTelemetry.js:1942 (Diff revision 2) > + hashWithoutSearch += "_"; > + } > + if (searchText) { > + hash = hashWithoutSearch + Search.HASH_SEARCH + searchText.replace(/ /g, "+"); > + } else if (hashWithoutSearch) { > + hash = hashWithoutSearch.slice(0, hashWithoutSearch.length - 1); This line is double-indented.

Done.

Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5c767e969819 about:telemetry : Added search terms in the URL fragment. r=chutten

I've told the commit to head for autoland, so it should soon be in the tree. Thank you for your contribution! Do you need help finding your next task, or do you have a feel for where to find bugs now?

Thanks for helping me with this. I think I do have a good idea of how to find bugs, so that's okay. :)