The default bug view has changed. See this FAQ.

Implement browser.history.onTitleChanged

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Untriaged
P5
normal
9 months ago
11 days ago

People

(Reporter: bsilverberg, Assigned: shatur, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [history] triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

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.
Mentor: bob.silverberg@gmail.com
Priority: -- → P5
Whiteboard: [good first bug][history]

Comment 1

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

Comment 2

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

Comment 3

9 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

9 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

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

Updated

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

Comment 6

9 months ago
Hi Shell,

What does this mean? 

"Priority: P5 → P4
Whiteboard: [good first bug][history] → [good first bug][history] triaged"
(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

9 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

8 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

8 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

8 months ago
Andrew,

I will give that shot! Thank you so much for the help.
Flags: needinfo?(bob.silverberg)
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 months ago
Keywords: good-first-bug
Whiteboard: [good first bug][history] triaged → [history] triaged
(Assignee)

Comment 15

28 days 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)
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)
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)
Assignee: dindrala → tushar.saini1285
Priority: P4 → P5
Comment hidden (mozreview-request)
(Assignee)

Comment 20

15 days 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

14 days 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

14 days 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

14 days 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

14 days 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+
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

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