1085774 - Add more history URLs to tab records

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Tabs already allows for an array of URIs, so this is even in spec. We just need to add the code to grab the tab's history from session store.

Comment 1

[prasanna moharana]

hello am interested for this ..can u help me a little to solve this bug

Comment 2

[Richard Newman [:rnewman]]

Prasanna: this is a desktop bug involving extending Firefox Sync.

You should start here:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

You'll need to get a checkout of mozilla-central, get it to build, and to the point that you can run

  ./mach xpcshell-test services/sync/tests/unit

to run Sync's test suite.

Your goal after that is to produce a patch:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

that I can review. The patch will consist of changes to

  services/sync/modules/engines/tabs.js

and some tests. Those changes will fetch more values and insert them into each tab record.

Let me know when you've got Firefox building and have a patch underway!

Comment 3

[prasanna moharana]

hello @rnewman i think mozilla central git repo is expired so can u tell me from which repo i should again start my work??

Comment 4

[Edouard Oger [:eoger]]

Attachment: 1085774.patch

I also added a more complete mockGetWindowEnumeratorHash function for testing.

Comment 5

[Edouard Oger [:eoger]]

Attachment: 1085774_v2.patch

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8512276 [details] [diff] [review]
1085774_v2.patch

Review of attachment 8512276 [details] [diff] [review]:
-----------------------------------------------------------------

Great start, Edouard!

I think we can make this a bit neater (not your fault, the original code is messy), and I'd like to see reuse.

::: services/sync/modules/engines/tabs.js
@@ +141,5 @@
>  
> +        // Initial element is the current URL. Subsequent URLs were
> +        // previously visited URLs.
> +        let entries = [];
> +        entries.push(curEntry.url);

What about something like this?


// Define this outside the `for` loop for efficiency.
let acceptable = !filter ? (url) => url : 
                           (url) => url && filteredUrls.test(url);


let entries = tabState.entries;
let index = tabState.index;
let current = entries[index - 1];

// Short-circuit if the current URL isn't acceptable.
if (!acceptable(current)) {
  continue;
}

// Exploit 1-indexedness.
// Slice down so future entries are excluded, then process into
// an array in the right order.
let candidates = (entries.length == index) ? entries : entries.slice(0, index);
let urls = candidates.map((entry) => entry.url).filter(acceptable).reverse();

::: services/sync/tests/unit/head_helpers.js
@@ +155,5 @@
> +*        }]
> +*    }]
> +* }]
> +*/
> +function mockGetWindowEnumeratorHash(hash) {

I'd rather see code reuse with the version in head_helpers.

::: services/sync/tests/unit/test_tab_store.js
@@ +51,5 @@
>  function test_getAllTabs() {
>    let store = getMockStore();
>    let tabs;
>  
> +  var allTabs = [{

`let` not `var`, always always always.

Comment 7

[Edouard Oger [:eoger]]

Attachment: 1085774_v3.patch

Thank you for your review Richard,

I switched to the functionnal style your proposed (tabs.js) and also ditched the mockGetWindowEnumeratorHash method (head_helpers.js).
Instead 2 extra parameters were added to mockGetWindowEnumerator (for index and additionnal urls).

If you think the mockGetWindowEnumeratorHash function could be useful in the future, I could also convert all the tests using mockGetWindowEnumerator to the new signature and remove the old mock method.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8512734 [details] [diff] [review]
1085774_v3.patch

Review of attachment 8512734 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great with these two comments addressed.

(Sorry for not noting the safety limit earlier.)

::: services/sync/modules/engines/tabs.js
@@ +146,5 @@
>  
> +        // Initial element is the current URL. Subsequent URLs were
> +        // previously visited URLs.
> +        let candidates = (entries.length == index) ? entries : entries.slice(0, index);
> +        let urls = candidates.map((entry) => entry.url).filter(acceptable).reverse();

I think we need a safety layer here. Before, after or both, we should be trimming over-long arrays.

If `entries` has a thousand items, we don't want to be including the whole thing.

A tabs record can be no more than 256K. Let's pick a sane limit for `entries` (25?) and use that to complicate line 149.

::: services/sync/tests/unit/head_helpers.js
@@ +137,5 @@
>  function mockGetTabState (tab) {
>    return tab;
>  }
>  
> +function mockGetWindowEnumerator(url, numWindows, numTabs, indexes, moreUrls) {

moreURLs.

Comment 9

[Edouard Oger [:eoger]]

Attachment: 1085774.patch

Hi Richard,

No problem, I added the limit feature and an associated test. The moreURLs arg was also corrected.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8522370 [details] [diff] [review]
1085774.patch

Review of attachment 8522370 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in reviewing this.

::: services/sync/modules/engines/tabs.js
@@ +149,5 @@
> +        // previously visited URLs.
> +        let candidates = ((entries.length == index) ?
> +          entries : 
> +          entries.slice(0, index)).slice(0, TAB_ENTRIES_LIMIT);
> +        

Nit: trailing whitespace.

Also I think we can do this in one go, rather than slicing twice.

::: services/sync/tests/unit/test_tab_store.js
@@ +82,5 @@
> +  store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://bar.com", 1, 1, () => 45, () => fiftyUrls);
> +  tabs = store.getAllTabs();
> +  _("Sliced: " + JSON.stringify(tabs));
> +  do_check_eq(tabs.length, 1);
> +  do_check_eq(tabs[0].urlHistory.length, 25);

I think you also want to make sure that the limited array is the set of most recent URLs, not just 25 items long!

That is: the first element should be the current URL.

Comment 11

[Edouard Oger [:eoger]]

Attachment: 1085774.patch

Hello Richard,

Please excuse me for the delay.
I moved the TAB_ENTRIES_LIMIT slicing after the filtering, so we always get the maximum number of possible history entries.

Tell me what you think about it.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8527654 [details] [diff] [review]
1085774.patch

Review of attachment 8527654 [details] [diff] [review]:
-----------------------------------------------------------------

Good job! Thanks for finishing this up, and I apologize for the delay in review.

I'll get this ready to land.

::: services/sync/modules/engines/tabs.js
@@ +145,5 @@
>            continue;
>          }
>  
> +        // Initial element is the current URL. Subsequent URLs were
> +        // previously visited URLs.

This comment isn't really right.

The *indexed* element is the current URL. Previous URLs (i.e., those at a lower index) were previously visited. Subsequent URLs are in the "forward" stack, and Sync's record format can't model those.

So what this little chunk of code does is drop any 'forward' URLs.

@@ +151,5 @@
> +          entries :
> +          entries.slice(0, index);
> +
> +        let urls = candidates.map((entry) => entry.url).filter(acceptable)
> +          .reverse().slice(0, TAB_ENTRIES_LIMIT);

This should do, but I'll clean it up to be column-aligned.

It might also be worth discarding consecutive duplicates here, because my testing indicates that they can happen, but they're of no use to other clients.

::: services/sync/tests/unit/head_helpers.js
@@ +142,5 @@
>    let elements = [];
> +
> +  function url2entry(url) {
> +    return {
> +      url: ((typeof url == "string") ? url : url()),

I'd probably invert this test: typeof url == "function".

::: services/sync/tests/unit/test_tab_store.js
@@ +75,5 @@
>    do_check_eq(tabs.length, 0);
> +
> +  _("Get all tabs, and check that the entries safety limit works.");
> +  let allUrls = [];
> +  for(let i = 0; i < 50; i++) {

Nit: space before "(".

@@ +87,5 @@
> +  do_check_eq(tabs.length, 1);
> +  do_check_eq(tabs[0].urlHistory.length, 25);
> +  do_check_eq(tabs[0].urlHistory[0], "http://foo40.bar");
> +  do_check_eq(tabs[0].urlHistory[24], "http://foo16.bar");
> +

No trailing blank line.

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/b0e899a4f717
https://hg.mozilla.org/integration/fx-team/rev/9dffef3538c3

Comment 14

[Edouard Oger [:eoger]]

Great! Looks good, thank you.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b0e899a4f717
https://hg.mozilla.org/mozilla-central/rev/9dffef3538c3