1280582 - Implement browser.history.onTitleChanged

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P5)

Description

[Bob Silverberg [:bsilverberg]]

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.

Comment 1

[dindrala]

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

[Kris Maglione [:kmag]]

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!

Comment 3

[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

[Luca Greco [:rpl] [:luca] [:lgreco]]

(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).

Comment 5

[dindrala]

Sounds good Lucas. Thank you for the resources!

Comment 6

[dindrala]

Hi Shell,

What does this mean? 

"Priority: P5 → P4
Whiteboard: [good first bug][history] → [good first bug][history] triaged"

Comment 7

[Bob Silverberg [:bsilverberg]]

(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

Comment 8

[dindrala]

Bob,

Thank you so much!! These links are absolutely great. Again thank you.

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

Are you still working on this bug? Do you need further help?

Comment 10

[dindrala]

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

[Andreas Wagner [:TheOne] [use NI]]

Bob, your mentor will follow up with you shortly.

Comment 12

[Andrew Swan [:aswan]]

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

[dindrala]

Andrew,

I will give that shot! Thank you so much for the help.

Comment 14

[Bob Silverberg [:bsilverberg]]

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.

Comment 15

[Tushar Saini (:shatur)]

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

Comment 16

[Bob Silverberg [:bsilverberg]]

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

Comment 17

[Bob Silverberg [:bsilverberg]]

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 18

[Tushar Saini (:shatur)]

Attachment: Bug 1280582 - Implement browser.history.onTitleChanged.

Review commit: https://reviewboard.mozilla.org/r/118202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118202/

Comment 19

[Tushar Saini (:shatur)]

Comment on attachment 8844960 [details]
Bug 1280582 - Implement browser.history.onTitleChanged.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118202/diff/1-2/

Comment 20

[Tushar Saini (:shatur)]

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.

Comment 21

[Bob Silverberg [:bsilverberg]]

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.

Comment 22

[Tushar Saini (:shatur)]

Comment on attachment 8844960 [details]
Bug 1280582 - Implement browser.history.onTitleChanged.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118202/diff/2-3/

Comment 23

[Tushar Saini (:shatur)]

(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!!

Comment 24

[Bob Silverberg [:bsilverberg]]

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.

Comment 25

[Tushar Saini (:shatur)]

Comment on attachment 8844960 [details]
Bug 1280582 - Implement browser.history.onTitleChanged.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118202/diff/3-4/

Comment 26

[Bob Silverberg [:bsilverberg]]

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.

Comment 27

[Bob Silverberg [:bsilverberg]]

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.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8844960 [details]
Bug 1280582 - Implement browser.history.onTitleChanged.

https://reviewboard.mozilla.org/r/118202/#review121740

Comment 29

[Tushar Saini (:shatur)]

Hey Bob!

Do we need to do anything else in here?

Also, do you have any other bugs, on which I can work!

Regards

Comment 30

[Bob Silverberg [:bsilverberg]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d34d4561ee2
Implement browser.history.onTitleChanged. r=bsilverberg,mixedpuppy

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d34d4561ee2

Comment 33

[Bob Silverberg [:bsilverberg]]

Tushar, this has now landed on Nightly, great work! Thanks for all of your time and effort on this bug.

Comment 34

[Tushar Saini (:shatur)]

(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!

Comment 35

[Bob Silverberg [:bsilverberg]]

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.

Comment 36

[Will Bamberg [:wbamberg]]

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.

Comment 37

[Bob Silverberg [:bsilverberg]]

Looks good Will, thanks!