Closed
Bug 1280582
Opened 8 years ago
Closed 7 years ago
Implement browser.history.onTitleChanged
Categories
(WebExtensions :: Untriaged, defect, P5)
WebExtensions
Untriaged
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bsilverberg, Assigned: shatur, Mentored)
Details
(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [history] triaged)
Attachments
(1 file)
This is a new API for Firefox that does not exist for Chrome. The reason we might want to implement this is that the onVisited event does not provide a title for pages, as we do not know the title when the page is first visited. Chrome does something similar, where it provides a title for previously visited pages, but not for pages visited for the first time. If an API consumer really needs the title of a page, and wants to be sure of getting an accurate title, they would be able to do so by listening to onTitleChanged. This would be a fairly simple thing to implement, but I'm not sure how much value it provides. We should discuss it in the next triage meeting.
Reporter | ||
Updated•8 years ago
|
Mentor: bob.silverberg
Priority: -- → P5
Whiteboard: [good first bug][history]
Hi Bob, I am a first time contributor to Mozilla and Id like to work on this ticket. Not quite sure if the value discussion has taken place yet and if anything was decided on that front. Please let me know! Cheers!
Comment 2•8 years ago
|
||
We're agreed that this should be implemented, so feel free to start working on it. Let us know if you have any questions. Thanks!
Assignee: nobody → dindrala
Kris, Sounds great! Excited to have my first Mozilla ticket. Would you mind giving me the instructions on how to get set up with the repo/workflow? Thanks!
Comment 4•8 years ago
|
||
(In reply to dindrala from comment #3) > Kris, > > Sounds great! Excited to have my first Mozilla ticket. > > Would you mind giving me the instructions on how to get set up with the > repo/workflow? > > Thanks! You need to setup your mozilla-central development environment as described in the following MDN doc page (if you don't have already done previously): - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds and the following is the Hacking page which give you more information about coding style conventions and the code layout of the WebExtensions internals: - https://wiki.mozilla.org/WebExtensions/Hacking Feel free to join and ask for help in #webextensions irc channel on irc.mozilla.org (you can look https://wiki.mozilla.org/IRC if you need help to setup an irc client).
Updated•8 years ago
|
Priority: P5 → P4
Whiteboard: [good first bug][history] → [good first bug][history] triaged
Hi Shell, What does this mean? "Priority: P5 → P4 Whiteboard: [good first bug][history] → [good first bug][history] triaged"
Reporter | ||
Comment 7•8 years ago
|
||
(In reply to dindrala from comment #6) > Hi Shell, > > What does this mean? > > "Priority: P5 → P4 > Whiteboard: [good first bug][history] → [good first bug][history] triaged" Hi dindrala, it just means that the priority of this was changed from a P5 to a P4, and that the bug was triaged, which means it was discussed by our team. It is definitely available for you to work on. Once you've got a development environment set up (by following Luca's advice above), you can start to work on the code. To implement this event you'll need to address the following: - Implement the onTitleChanged function in the extension observer, which is found at [1]. It will be similar to the code for onDeleteURI as found at [2]. - Add an onTitleChanged event to the API code which will be similar to the onVisited event as found at [3]. - Add a test for the new onTitleChanged event to browser_ext_history.js [4]. It might make sense to simply enhance the test at [5] to add some checks for the onTitleChanged event. Let me know if you run into any issues or have any questions about these suggestions. [1] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-history.js#116 [2] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-history.js#98 [3] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-history.js#223 [4] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_history.js [5] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_history.js#403
Bob, Thank you so much!! These links are absolutely great. Again thank you.
Comment 9•8 years ago
|
||
Are you still working on this bug? Do you need further help?
Flags: needinfo?(dindrala)
Comment 10•8 years ago
|
||
Hi Andreas, Yes I am. I do need a little help. Is there a way for me to share code with someone to make sure I am on the right track before committing it?
Comment 11•8 years ago
|
||
Bob, your mentor will follow up with you shortly.
Flags: needinfo?(bob.silverberg)
Updated•8 years ago
|
Flags: needinfo?(dindrala)
Comment 12•8 years ago
|
||
dindrala, Bob is on PTO at the moment but you can create a patch with your changes and add is as an attachment to this bug. If you have set up the mozilla version control tools, you can automate this by creating a local mercurial commit with your changes, including the bug number in the commit message, and then running "hg bzexport ." from your local tree. Or feel free to pop into #webextensions on IRC for more interactive help.
Comment 13•8 years ago
|
||
Andrew, I will give that shot! Thank you so much for the help.
Updated•8 years ago
|
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 14•8 years ago
|
||
dindrala, my apologies for not responding sooner, but I was away on vacation. I am back now and happy to help you in any way that I can. Let me know if you have any specific questions. As Andrew mentioned, you can attach your code to this bug if you'd like me to take a look at it.
Updated•8 years ago
|
Keywords: good-first-bug
Whiteboard: [good first bug][history] triaged → [history] triaged
Assignee | ||
Comment 15•7 years ago
|
||
Hi Bob, I would like to work on this bug. I am not able to find the file [1] [if it is rebased or something], can you please point it to me. Thanks!! [1] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_history.js
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 16•7 years ago
|
||
Hi Tushar, Thanks for your interest in working on this bug. The test was switched from a browser mochitest to an xpcshell test, and hence was moved to a different folder (and renamed). You can find the new file at [1], with the test that I suggested updating at [2]. Please let me know if you have any other questions. [1] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/xpcshell/test_ext_history.js [2] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/xpcshell/test_ext_history.js#403
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 17•7 years ago
|
||
As discussed on IRC, another thing that will be needed for this is an addition to the history.json file which defines the event. This would be similar to the definition of the `onVisitRemoved` [1] event in the json file. The event available to the history observer provides the url and title when the event is fired, so it would make sense to provide those to our onTitleChanged event as well. [1] http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/history.json#292
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Assignee: dindrala → tushar.saini1285
Priority: P4 → P5
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
Hey Bob!! I have modified the test for testing onTitleChanged event. This is just proof of concept. And I know that it will need refactoring. Please have a look and let me know what changes are required. Also, I have updated the descriptions in json file. I hope now it make sense now :-) Regards.
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8844960 [details] Bug 1280582 - Implement browser.history.onTitleChanged. https://reviewboard.mozilla.org/r/118202/#review121016 This is a great start, Tushar. I made some suggestions for changes to the descriptions in the json file, and have made some comments on the test as well. The basics of what you're doing in the test look good, but some changes are needed, and I think the logic for verifying onTitleChanged should be on its own in the test, as per my comments below. FYI, I will be away for the next two weeks, so please don't be concerned if your next review request is not addressed until after my return. ::: browser/components/extensions/schemas/history.json:317 (Diff revision 2) > ] > + }, > + { > + "name": "onTitleChanged", > + "type": "function", > + "description": "Fired when title of a URL is changed from the history service.", Nit: Perhaps change this to "Fired when the title of a URL is changed in the browser history." ::: browser/components/extensions/schemas/history.json:325 (Diff revision 2) > + "name": "changed", > + "type": "object", > + "properties": { > + "url": { > + "type": "string", > + "description": "The URL of webpage." "The URL for which the title has changed." ::: browser/components/extensions/schemas/history.json:329 (Diff revision 2) > + "type": "string", > + "description": "The URL of webpage." > + }, > + "title": { > + "type": "string", > + "description": "The changed title of the page." "The new title for the URL." ::: browser/components/extensions/test/xpcshell/test_ext_history.js:448 (Diff revision 2) > browser.test.sendMessage("on-visited-data", onVisitedData); > } > }); > > + // Verifying onTitleChange Event along with onVisited event > + let onTitleChangedData = []; You don't need this array as only one item ever gets added to it. ::: browser/components/extensions/test/xpcshell/test_ext_history.js:451 (Diff revision 2) > > + // Verifying onTitleChange Event along with onVisited event > + let onTitleChangedData = []; > + > + browser.history.onTitleChanged.addListener(data => { > + onTitleChangedData.push(data); There's no need to store this in an array, just send the data back to the test as you are doing on the next line. ::: browser/components/extensions/test/xpcshell/test_ext_history.js:476 (Diff revision 2) > let onVisitedData = yield extension.awaitMessage("on-visited-data"); > + let onTitleChangedData = yield extension.awaitMessage("on-title-changed-data"); > > function checkOnVisitedData(index, expected) { > let onVisited = onVisitedData[index]; > + let onChanged = onTitleChangedData[0]; This `checkOnVisitedData` is called for each visit that we want to check, but you only need to check for the titleChanged data once, as it will only happen once. Therefore, I think it makes more sense to take all of the logic for verifying onTitleChanged out of this method, and just do the check in the test itself. ::: browser/components/extensions/test/xpcshell/test_ext_history.js:485 (Diff revision 2) > // https://bugzilla.mozilla.org/show_bug.cgi?id=1287928 > equal(onVisited.title, "", "onVisited received a blank title"); > equal(onVisited.lastVisitTime, expected.time, "onVisited received the expected time"); > equal(onVisited.visitCount, expected.visitCount, "onVisited received the expected visitCount"); > + // Matching updated title, obtained from onTitleChange Event > + equal(onChanged.title, "Title Changed", "onTitleChanged received the title as expected") Missing semicolon at the end of this statement. Please check your code with eslint before finalizing your commit.
Attachment #8844960 -
Flags: review?(bob.silverberg)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg - on PTO until 03/27] from comment #21) > FYI, I will be away for the next two weeks, so please don't be concerned if > your next review request is not addressed until after my return. No problem Bob. I will wait for your return. I have updated the patch as told. Thanks for mentoring!!
Reporter | ||
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8844960 [details] Bug 1280582 - Implement browser.history.onTitleChanged. https://reviewboard.mozilla.org/r/118202/#review121042 This looks good, but I noticed one other thing that should be done, about which I have added a comment below. ::: browser/components/extensions/test/xpcshell/test_ext_history.js:496 (Diff revision 3) > expected.time = PAGE_INFOS[1].visits[0].date.getTime(); > checkOnVisitedData(1, expected); > > expected.time = PAGE_INFOS[1].visits[1].date.getTime(); > expected.visitCount = 2; > checkOnVisitedData(2, expected); Because you have added an additional visit to the `PAGE_INFOS` array, you should add an additional call to `checkOnVisitedData` for that visit as well.
Attachment #8844960 -
Flags: review?(bob.silverberg)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8844960 [details] Bug 1280582 - Implement browser.history.onTitleChanged. https://reviewboard.mozilla.org/r/118202/#review121058 Looks good, Shatur. I'm going to give it my r+ and then pass it over to a colleage who can give it a final r? to land. Thanks for all your work on this.
Attachment #8844960 -
Flags: review?(bob.silverberg) → review+
Reporter | ||
Comment 27•7 years ago
|
||
Comment on attachment 8844960 [details] Bug 1280582 - Implement browser.history.onTitleChanged. Hey Shane, do you mind taking a look at this patch to see if it's good to land as is? Thanks.
Attachment #8844960 -
Flags: review?(mixedpuppy)
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8844960 [details] Bug 1280582 - Implement browser.history.onTitleChanged. https://reviewboard.mozilla.org/r/118202/#review121740
Attachment #8844960 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Comment 29•7 years ago
|
||
Hey Bob! Do we need to do anything else in here? Also, do you have any other bugs, on which I can work! Regards
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 30•7 years ago
|
||
Hi Tushar. Sorry for the delay. I was away for the past couple of weeks and am in meetings all week this week, but I'll try to get this landed. I will also look for another bug for you to work on. If you want to try to find one on your own, you can visit [1] to see some WebExtensions bugs that are considered to be fairly simple. [1] https://bugzilla.mozilla.org/buglist.cgi?quicksearch=component%3AWebExtensions%20keyword%3Agood-first-bug&list_id=13512591
Comment 31•7 years ago
|
||
Pushed by bsilverberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7d34d4561ee2 Implement browser.history.onTitleChanged. r=bsilverberg,mixedpuppy
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bob.silverberg)
Comment 32•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d34d4561ee2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Reporter | ||
Comment 33•7 years ago
|
||
Tushar, this has now landed on Nightly, great work! Thanks for all of your time and effort on this bug.
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #33) > Tushar, this has now landed on Nightly, great work! Thanks for all of your > time and effort on this bug. That's great! I have a question, do we need to update MDN documentation for this new API? If yes, I would like to update the documentation. Please, can you guide me through the steps?:) Thanks!
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 35•7 years ago
|
||
There are a couple of options for documentation. If you want to add it yourself, you can simply do so. MDN is a wiki and anyone can edit it. If you don't want to update it yourself then you can simply add the "dev-doc-needed" keyword to this bug, and then someone at Mozilla will get the documentation done.
Flags: needinfo?(bob.silverberg)
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 36•7 years ago
|
||
I've added a page on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/history/onTitleChanged Please let me know if we need anything else here.
Flags: needinfo?(bob.silverberg)
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•