Implement browser.history.onTitleChanged

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
P5
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bsilverberg, Assigned: shatur, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [history] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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

a year ago
Mentor: bob.silverberg@gmail.com
Priority: -- → P5
Whiteboard: [good first bug][history]

Comment 1

11 months ago
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!
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

Comment 3

11 months ago
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

11 months 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).

Comment 5

11 months ago
Sounds good Lucas. Thank you for the resources!

Updated

11 months ago
Priority: P5 → P4
Whiteboard: [good first bug][history] → [good first bug][history] triaged

Comment 6

11 months ago
Hi Shell,

What does this mean? 

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

Comment 7

11 months 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

Comment 8

11 months ago
Bob,

Thank you so much!! These links are absolutely great. Again thank you.
Are you still working on this bug? Do you need further help?
Flags: needinfo?(dindrala)

Comment 10

10 months 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?
Bob, your mentor will follow up with you shortly.
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(dindrala)

Comment 12

10 months 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

10 months ago
Andrew,

I will give that shot! Thank you so much for the help.
Flags: needinfo?(bob.silverberg)
(Reporter)

Comment 14

10 months 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

10 months ago
Keywords: good-first-bug
Whiteboard: [good first bug][history] triaged → [history] triaged
(Assignee)

Comment 15

3 months 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

3 months 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

3 months 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

3 months ago
Assignee: dindrala → tushar.saini1285
Priority: P4 → P5
Comment hidden (mozreview-request)
(Assignee)

Comment 20

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

2 months 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

2 months 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

2 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d34d4561ee2
Implement browser.history.onTitleChanged. r=bsilverberg,mixedpuppy
(Assignee)

Updated

2 months ago
Flags: needinfo?(bob.silverberg)

Comment 32

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d34d4561ee2
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Comment 33

2 months ago
Tushar, this has now landed on Nightly, great work! Thanks for all of your time and effort on this bug.
(Assignee)

Comment 34

2 months 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

2 months 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)
You need to log in before you can comment on or make changes to this bug.