Update NewTabUtils.jsm for Activity Stream's needs

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ursula, Assigned: ursula)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The current PlacesProvider.jsm that lives in browser/components/newtab was originally used for remote newtab. We should update it with the new queries that Activity Stream needs.
(Assignee)

Updated

2 years ago
Assignee: nobody → usarracini
(Assignee)

Updated

2 years ago
Summary: Update PlacesProvider.jsm for Activity Stream's needs → Update NewTabUtils.jsm for Activity Stream's needs
(Assignee)

Comment 1

2 years ago
New plan: augment NewTabUtils.jsm with the new queries for Activity Stream's TopSites and remove browser/components/newtab/PlacesProvider.jsm altogether. This gives us a couple benefits:

1. We can easily make use of the singleton for BlockedLinks (and PinnedLinks in the future) that already exist within the file.
2. When we permanently take over about:newtab and need to gut out old tiles code, it'll all take place in one file with clear distinction which parts are for activity stream and which parts were for tiles.
3. Less code duplication, since we don't have to implement our own PlacesProvider that wraps some of the functions in the PlacesProvider that lives in NewTabUtils.jsm
4. We can gut out some of the dead code for remote newtab
Sounds reasonable to me, as long as we don't run into issues with race conditions from PlacesProvider not being initialized yet.
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review126240

I need a bit more explanation about how the various calls will be used, since this doesn't contain any consumers yet, apart from the test.
Also, I don't know much about the NewTabMessages.jsm removal, it would be great to have some overview of the changes, also for future reference. Feel free to add those to a multi-line commit message if it can be summarized in a useful way.

::: toolkit/components/places/tests/PlacesTestUtils.jsm:85
(Diff revision 1)
>    }),
>  
> +   /*
> +    * Add Favicons
> +    *
> +    * @param {Object} faviconURLs  keys are page URLs, values are associated favicon URLs.

this tells about keys and values, but doesn't specify whether this should be an object or a Map. For example by reading "keys" I'd think this is a Map, but instead it's an object (maybe it should state properties, or maybe just use Map that is made for that).

::: toolkit/components/places/tests/PlacesTestUtils.jsm:93
(Diff revision 1)
> +  addFavicons: Task.async(function*(faviconURLs) {
> +    let faviconPromises = [];
> +    if (faviconURLs) {
> +      for (let pageURL in faviconURLs) {
> +        if (!faviconURLs[pageURL]) {
> +          continue;

how can this happen? shouldn't this rather throw, since it would clearly be a coding mistake?

::: toolkit/components/places/tests/PlacesTestUtils.jsm:102
(Diff revision 1)
> +          let faviconURI = NetUtil.newURI(faviconURLs[pageURL]);
> +          try {
> +            PlacesUtils.favicons.setAndFetchFaviconForPage(
> +              uri, faviconURI, false,
> +              PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, () => {
> +                resolve();

I suspect you could just pass resolve as argument.

::: toolkit/components/places/tests/PlacesTestUtils.jsm:112
(Diff revision 1)
> +          }
> +        }));
> +      }
> +    }
> +
> +    if (faviconPromises.length) {

I'd prefer if this would throw if faviconUrls is empty, and throw if any value is empty. Then you should not need this check, and the callers would be better protected from coding mistakes.

::: toolkit/components/places/tests/PlacesTestUtils.jsm:138
(Diff revision 1)
>    },
>  
>    /**
> +   * Synchronous operation to clear all bookmarks
> +   */
> +  clearBookmarks() {

I'm not sure what's the scope here, is there a specific reason to not clear folders for example?
Please just use yield PlacesUtils.bookmarks.eraseEverything() if you need to remove any kind of added bookmarks, unless you really only need to clear URI bookmarks, but then we must discuss a better approach (this is using a synchronous statement, that is a deprecated way of doing it).

::: toolkit/modules/NewTabUtils.jsm:647
(Diff revision 1)
>      PlacesUtils.history.addObserver(this, true);
>    },
>  
> +  uninit: function PlacesProvider_uninit() {
> +    PlacesUtils.history.removeObserver(this);
> +  },

the added observer is a weak reference, it doesn't need to be removed explicitly.
Moreover, there is actually a more specific reason this observer should not be removed, that is https://bugzilla.mozilla.org/show_bug.cgi?id=1320481

Basically the clear history notification can arrive quite late in the shutdown process, if you stop observing before that point, you won't clear the thumbnails cache.

We could honestly also introduce an onBeforeClearHistory notification for these cases, to makes things less error prone.

::: toolkit/modules/NewTabUtils.jsm:831
(Diff revision 1)
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver,
>                                           Ci.nsISupportsWeakReference]),
>  };
>  
>  /**
> + * Queries history to retrieve the most visisted sites. Emits events when the

typo: visisted

also this is actually the most "frecent" sites, that is not exactly the same thing, but close.

::: toolkit/modules/NewTabUtils.jsm:902
(Diff revision 1)
> +  /*
> +   * Uninitializes Activity Stream provider - removes the history observer and
> +   * the bookmarks observer.
> +   */
> +  uninit: function ActivityStreamProvider_uninit() {
> +    PlacesUtils.history.removeObserver(this.historyObserver);

Same as above, if this is expected to clear any kind of disk cache, it should not be removed. Otherwise it's fine.

::: toolkit/modules/NewTabUtils.jsm:912
(Diff revision 1)
> +   * A set of functions called by @mozilla.org/browser/nav-historyservice
> +   * All history events are emitted from this object.
> +   */
> +  historyObserver: {
> +    onDeleteURI(uri) {
> +      Services.obs.notifyObservers(null, "deleteURI", {url: uri.spec});

It sounds a little bit strange to me that we forward all the events to the observer service, without even a prefix.
Provided there's no better internal channel to communicate with the other consumers, I'd prefer these notifications to have some prefix, "newtab-something".
The risk here is that other consumers start relying on these notifications thinking they directly come from history, and when we change something here they'll break.

I'm also not sure what's the reason the consumers couldn't directly observer history? Are we crossing a boundary? Are consumers in a different process? Can't we somehow keep a list of these consumers and directly invoke their observing methods? That'd be far more efficient.

To sum up, this needs a more global overview and description of the plan.

::: toolkit/modules/NewTabUtils.jsm:1000
(Diff revision 1)
> +    // defined in SQL, hence we are relying on sqlite implementation that may
> +    // change in the future.
> +    let sqlQuery = `SELECT url, title, SUM(frecency) frecency, guid, bookmarkGuid,
> +                     last_visit_date / 1000 as lastVisitDate, favicon, mimeType,
> +                     "history" as type
> +                    FROM (SELECT * FROM (

just FYI, this throws away any possibility to use an index, the external ORDER BY will likely just have to do a linear scan and reorder.
I expect this to send warnings in debug builds, so maybe you should add a /* DO NOT WARN comment to the query, provided there's no better way to achieve the same result preserving indices.

::: toolkit/modules/NewTabUtils.jsm:1016
(Diff revision 1)
> +                        frecency,
> +                        last_visit_date,
> +                        moz_places.guid AS guid,
> +                        moz_bookmarks.guid AS bookmarkGuid
> +                      FROM moz_places
> +                      LEFT JOIN moz_favicons

With the new favicons this won't be possible anymore, and we'll bitrot each other. The reason it isn't possible is that each page can have multiple icons at different size, a join won't be able to solve that to a single entry.

the suggested way to get an icon is to use a "page-icon:YOUR_URL" src or the favicons service API. The former is clearly preferred.

::: toolkit/modules/NewTabUtils.jsm:1067
(Diff revision 1)
> +                     b.guid as bookmarkGuid,
> +                     "bookmark" as type,
> +                     f.data as favicon,
> +                     f.mime_type as mimeType
> +                   FROM moz_places p, moz_bookmarks b
> +                   LEFT JOIN moz_favicons f

ditto

::: toolkit/modules/NewTabUtils.jsm:1108
(Diff revision 1)
> +   * @param {Object} aOptions
> +   *          options.ignoreBlocked: do not filter out blocked links
> +   *
> +   * @returns {Promise} Returns a promise with the count of moz_places records
> +   */
> +  getHistorySize: Task.async(function* ActivityStreamProvider_getHistorySize(aOptions = {}) {

what is this used for?
It sounds puzzling to me that we care about exact size of history, and moreover we want to filter that number by an unknown number of blocked urls.
If we'd not filter a count(*) query would be far more performant, but I don't know reasoning and I can't find consumers to check, apart from the test.

This can return hundred thousands entries, and we'd go through all of them just to count. it sounds very inefficient.

::: toolkit/modules/NewTabUtils.jsm:1129
(Diff revision 1)
> +   * @param {Object} aOptions
> +   *          options.ignoreBlocked: do not filter out blocked links
> +   *
> +   * @returns {Promise} Returns a promise with the count of bookmarks
> +   */
> +  getBookmarksSize: Task.async(function* ActivityStreamProvider_getBookmarksSize(aOptions = {}) {

ditto about filtering and inefficient query

::: toolkit/modules/NewTabUtils.jsm:1166
(Diff revision 1)
> +      yield conn.executeCached(aQuery, params, aRow => {
> +        try {
> +          // check if caller wants to handle query raws
> +          if (callback) {
> +            callback(aRow);
> +          }

typo: raws

Also, this option is never used anywhere, so you could avoid the callback and just return an array of rows from the call. That'd simplify the code.
Note that the above will be good for returning a limited number of entries (less than 100) while it's still better to use the callback if a query is expected to return more than those. But if we really need more than 100 entries something is wrong at the architectural level.

::: toolkit/modules/NewTabUtils.jsm:1224
(Diff revision 1)
> +   * @param {String} aUrl
> +   *          The url to bookmark
> +   *
> +   * @returns {Promise} Returns a promise set to an object representing the bookmark
> +   */
> +  addBookmark: function ActivityStreamLinks_addBookmark(aUrl) {

again, this is only used in tests, so I can't evaluate whether it's needed.
Fwiw you could even just invoke 
return PlacesUtils.bookmarks.insert({
  url: aUrl,
  parentGuid: PlacesUtils.bookmarks.unfiledGuid
});

type can be omitted if an url is provided.

So the question is whether this thin wrapper around bookmarks and history (included delete bookmark, delete history and so on) is needed at all, it's not saving valuable code size, it's mostly just a redirect.

::: toolkit/modules/NewTabUtils.jsm:1265
(Diff revision 1)
> +   * @return {Promise} Returns a promise with the array of links as the payload
> +   */
> +  getTopSites: Task.async(function* ActivityStreamProvider_getTopSites(aOptions = {}) {
> +    let sites = yield ActivityStreamProvider.getTopFrecentSites(aOptions);
> +    return sites;
> +  })

wouldn't this suffice?
getTopSites() {
  return ActivityStreamProvider.getTopFrecentSites(aOptions);
}

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:20
(Diff revision 1)
> +  "resource://testing-common/PlacesTestUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
> +  "resource://gre/modules/NetUtil.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",

sounds like a lot of this boilerplate should instead be moved to the head.js file

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:54
(Diff revision 1)
> +    yield PlacesTestUtils.clearHistory();
> +    PlacesTestUtils.clearBookmarks();
> +    let faviconExpiredPromise = new Promise(resolve => {
> +      Services.obs.addObserver(resolve, "places-favicons-expired", false);
> +    });
> +    yield PlacesUtils.favicons.expireAllFavicons();

this API doesn't return a promise. It's quite ugly, I know.

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:374
(Diff revision 1)
> +
> +  for (let test of tests) {
> +    let clone = JSON.parse(JSON.stringify(test));
> +    delete clone[0].mimeType;
> +    clone[0].favicon = `data:foo;base64,${btoa("bar")}`;
> +    let result = provider._faviconBytesToDataURI(test);

you may have to do an actual comparison of favicon contents, depending on the way you decide to handle those.
There's an example here: http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/toolkit/components/places/tests/favicons/test_page-icon_protocol.js#5

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:613
(Diff revision 1)
> +    {uri: NetUtil.newURI("https://example2.com/"), visitDate: timeToday, transition: TRANSITION_TYPED},
> +    {uri: NetUtil.newURI("https://example3.com/"), visitDate: timeEarlier, transition: TRANSITION_TYPED},
> +    {uri: NetUtil.newURI("https://example4.com/"), visitDate: timeEarlier, transition: TRANSITION_TYPED}
> +  ];
> +  yield PlacesTestUtils.addVisits(visits);
> +  yield PlacesUtils.bookmarks.insert({url: "https://example5.com/", parentGuid: "root________", type: PlacesUtils.bookmarks.TYPE_BOOKMARK});

why the root? Nobody should add stuff to the bookmarks root. The API doesn't throw yet, but it should as soon as we define a couple details.
Attachment #8849239 - Flags: review?(mak77)
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review126240

> It sounds a little bit strange to me that we forward all the events to the observer service, without even a prefix.
> Provided there's no better internal channel to communicate with the other consumers, I'd prefer these notifications to have some prefix, "newtab-something".
> The risk here is that other consumers start relying on these notifications thinking they directly come from history, and when we change something here they'll break.
> 
> I'm also not sure what's the reason the consumers couldn't directly observer history? Are we crossing a boundary? Are consumers in a different process? Can't we somehow keep a list of these consumers and directly invoke their observing methods? That'd be far more efficient.
> 
> To sum up, this needs a more global overview and description of the plan.

I like the idea of naming the messages ```newtab-${eventName}```. Let's go with that
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review126240

> just FYI, this throws away any possibility to use an index, the external ORDER BY will likely just have to do a linear scan and reorder.
> I expect this to send warnings in debug builds, so maybe you should add a /* DO NOT WARN comment to the query, provided there's no better way to achieve the same result preserving indices.

I'll add the /* DO NOT WARN */ comment here to avoid warnings in debug builds. There's still a lot of work being done on the test pilot version to better refine this query. 
We're also going to collect some telemetry on how long it takes to execute this query vs the existing about:newtab query and try to improve it from there

> typo: raws
> 
> Also, this option is never used anywhere, so you could avoid the callback and just return an array of rows from the call. That'd simplify the code.
> Note that the above will be good for returning a limited number of entries (less than 100) while it's still better to use the callback if a query is expected to return more than those. But if we really need more than 100 entries something is wrong at the architectural level.

I'll definitely remove the callback bit.
About the performance of this function - I totally agree that this is kind of a brute force way of doing this. I'll note though, that the only function where we pass in columns with the query, is for the top sites, which has a hard limit of 12. So in this case we'll only be iterating over the 12 entries so it should be ok. 

However, in the future (when we want to land 'Highlights') the query will return a lot more entries, so you're right, we will have to rethink how this function performs. For now though, since there are no real consumers of this function, and it was meant to be a helper function to execute the query that returns at max 12 entries, I think it's ok to land this way (with the removal of the callback)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review126240

> With the new favicons this won't be possible anymore, and we'll bitrot each other. The reason it isn't possible is that each page can have multiple icons at different size, a join won't be able to solve that to a single entry.
> 
> the suggested way to get an icon is to use a "page-icon:YOUR_URL" src or the favicons service API. The former is clearly preferred.

Will change this, and the other query to use the favicon service API (getFaviconDataForPage)

> again, this is only used in tests, so I can't evaluate whether it's needed.
> Fwiw you could even just invoke 
> return PlacesUtils.bookmarks.insert({
>   url: aUrl,
>   parentGuid: PlacesUtils.bookmarks.unfiledGuid
> });
> 
> type can be omitted if an url is provided.
> 
> So the question is whether this thin wrapper around bookmarks and history (included delete bookmark, delete history and so on) is needed at all, it's not saving valuable code size, it's mostly just a redirect.

There were 2 ideas behind the wrapper "ActivityStreamLinks":
1. Its a nice clean entry point for the user actions that will eventually call these functions (like adding/removing bookmarks, blocking/unblocking urls etc) that's separate from the place where the actual queries are happening. This style has been helpful in the test pilot version of the addon, when we use A/B experiments that way each branch of the experiment can follow two different code paths in a clean manner.
2. I tried to follow the way NewTabUtils was already handling its links - where there's a "provider" to power what the links need, and then a set of "links" which is separate. That allows me to do something similar to this: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#1418 with the blocked links, but for the "links" that Activity Stream cares about
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8849239 - Flags: review?(mak77)
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review132898

Sorry if it took a few days more than expected, I was trying to complete the icons work, that just landed 2 days ago.

This looks almost ready to land, just a few suggestions here and there. If there's something you prefer to handle in follow-ups I'm fine with that.

::: toolkit/modules/NewTabUtils.jsm:358
(Diff revision 4)
>    /**
>     * Initializes object. Adds a preference observer
>     */
>    init: function GridPrefs_init() {
>      Services.prefs.addObserver(PREF_NEWTAB_ROWS, this, false);
>      Services.prefs.addObserver(PREF_NEWTAB_COLUMNS, this, false);

nit: a possible alternative, when you only need to read and observe prefs, is to use defineLazyPreferenceGetter
off-hand looks like the whole GridPrefs object could be thrown away and replaced with that. You could use its callback to check that the value is meaningful (or set the property to 1) and invoke AllPages.update().
That would also save a small startup cost since you wouldn't need to init() it.

::: toolkit/modules/NewTabUtils.jsm:889
(Diff revision 4)
> +            NetUtil.newURI(link.url),
> +            (iconuri, len, data, mime) => {
> +              // Due to the asynchronous behaviour of inserting a favicon into moz_favicons, the data may
> +              // not be available to us just yet, since we listen on a history entry being inserted.
> +              // As a result, we don't want to throw if the icon uri is not here yet, we just want to resolve
> +              // on an empty favicon. Activity Stream knows how to handle null favicons

This comment may need some reindentation for the 80 chars soft-limit.

::: toolkit/modules/NewTabUtils.jsm:907
(Diff revision 4)
> +          // If something goes wrong - that's ok - just return a null favicon without
> +          // rejecting the entire Promise.all
> +          link.favicon = null;
> +          link.mimeType = null;
> +          return resolve(link);
> +        }

I think you could attach a .catch() handler to the promise and avoid the internal try catch. in the catch you can set link properties and return link.

::: toolkit/modules/NewTabUtils.jsm:986
(Diff revision 4)
> +  bookmarksObsever: {
> +    onItemAdded(id, folderId, index, type) {
> +      if (type === PlacesUtils.bookmarks.TYPE_BOOKMARK) {
> +        ActivityStreamProvider.getBookmark({id}).then(bookmark => {
> +          Services.obs.notifyObservers(null, "newtab-bookmarkAdded", bookmark);
> +        }).catch(err => Cu.reportError(err));

just .catch(Cu.reportError);

::: toolkit/modules/NewTabUtils.jsm:1000
(Diff revision 4)
> +
> +    onItemChanged(id, property, isAnnotation, value, lastModified, type) {
> +      if (type === PlacesUtils.bookmarks.TYPE_BOOKMARK) {
> +        ActivityStreamProvider.getBookmark({id}).then(bookmark => {
> +          Services.obs.notifyObservers(null, "newtab-bookmarkChanged", bookmark);
> +        }).catch(err => Cu.reportError(err));

ditto

::: toolkit/modules/NewTabUtils.jsm:1036
(Diff revision 4)
> +                      SELECT
> +                        rev_host,
> +                        CASE SUBSTR(rev_host, -5)
> +                          WHEN ".www." THEN SUBSTR(rev_host, -4, -999)
> +                          ELSE rev_host
> +                        END AS rev_nowww,

Alternatively you could use
fixup_url(get_unreversed_host(rev_host)) to get the host without www.
It would allow you to avoid fancy string manipulation and it should not be much slower.

::: toolkit/modules/NewTabUtils.jsm:1081
(Diff revision 4)
> +   *          options.id: bookmark ID
> +   */
> +  getBookmark: Task.async(function* ActivityStreamProvider_getBookmark(aOptions = {}) {
> +    let {id} = aOptions;
> +
> +    let sqlQuery = `SELECT p.url, p.title, p.frecency, p.guid,

So, if possible I'd prefer you to use the standard Bookmarks API to fetch a bookmark.
First, in the bookmark notifications you should add the missing arguments, so you will also get the bookmark guid.
Then pass the guid to this function.
With the guid you can use PlacesUtils.bookmarks.fetch(guid) to get back a bookmark object having:
guid, index, dateAdded, lastModified, type, title and url
You can then filter the result.

What will be missing from here:
the page guid: is this critical to have for you or would it be ok to set it to null for bookmarks, what do you use it for? Eventually we could fix fetch() to return a pageGuid property.
the bookmark id: same question, actually you could pass-through the id from the notification, if needed. We are deprecating the use of database ids in favor of guids.
frecency: it would not be a big deal to add it to Bookmarks.jsm::fetch so it gets returned

::: toolkit/modules/NewTabUtils.jsm:1088
(Diff revision 4)
> +                     b.lastModified / 1000 as lastModified,
> +                     b.id as bookmarkId,
> +                     b.title as bookmarkTitle,
> +                     b.guid as bookmarkGuid,
> +                     "bookmark" as type
> +                   FROM moz_places p, moz_bookmarks b

please use an explicit join

::: toolkit/modules/NewTabUtils.jsm:1139
(Diff revision 4)
> +   *
> +   * @returns {Promise} Returns a promise with the count of bookmarks
> +   */
> +  getBookmarksSize: Task.async(function* ActivityStreamProvider_getBookmarksSize() {
> +    let sqlQuery = `SELECT count(*) FROM moz_bookmarks b, moz_places p
> +                    WHERE type = :type AND b.fk = p.id`;

I'm not sure I see why you are joining here, it will be more expensive and you get the same filtering on type.

::: toolkit/modules/tests/xpcshell/head.js:27
(Diff revision 4)
> +const TIME_NOW = (new Date()).getTime();
> +
> +function run_test() {
> +  Services.prefs.setBoolPref(PREF_NEWTAB_ENHANCED, true);
> +  run_next_test();
> +}

just do the setBoolPref call without run_test and run_next_test?

::: toolkit/modules/tests/xpcshell/head.js:43
(Diff revision 4)
> +}
> +
> +// a set up function to prep the activity stream provider
> +function setUpActivityStreamTest() {
> +  return Task.spawn(function*() {
> +    do_get_profile();

you should probably just call do_get_profile() directly in head, since this features requires a profile.

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:322
(Diff revision 4)
> +  for (let test of tests) {
> +    let clone = JSON.parse(JSON.stringify(test));
> +    delete clone[0].mimeType;
> +    clone[0].favicon = `data:foo;base64,${btoa("bar")}`;
> +    let result = provider._faviconBytesToDataURI(test);
> +    do_check_eq(JSON.stringify(clone), JSON.stringify(result), "favicon converted to data uri");

would Assert.deepEqual work here?

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:333
(Diff revision 4)
> +  let provider = NewTabUtils.activityStreamProvider;
> +
> +  // start by passing in a bad uri and check that we get a null favicon back
> +  let links = [{url: "mozilla.com"}];
> +  yield provider._addFavicons(links);
> +  do_check_eq(links[0].favicon, null, "Got a null favicon because we passed in a bad url");

nit: Fwiw, new code should use Assert.equal and Assert.ok, not do_check_ methods

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:342
(Diff revision 4)
> +  links[0].url = "https://mozilla.com";
> +  let base64URL = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAA" +
> +    "AAAA6fptVAAAACklEQVQI12NgAAAAAgAB4iG8MwAAAABJRU5ErkJggg==";
> +
> +  let visit = [
> +    {uri: NetUtil.newURI(links[0].url), visitDate: timeDaysAgo(0), transition: PlacesUtils.history.TRANSITION_TYPED}

it should not be needed anymore to use newURI for addVisits.


As a side note that may be useful to you in the future, PlacesTestUtils.addVisits uses history.insertMany internally, that will only send an onManyFrecenciesChanged notification. IF you need single frecency notifications you will have to use PlacesUtils.history.insert.

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:393
(Diff revision 4)
> +
> +  // Test combined frecency score
> +  links = yield provider.getTopSites();
> +  do_check_eq(links.length, 1, "adding both www. and no-www. yields one link");
> +  do_check_eq(links[0].frecency, 200, "frecency scores are combined");
> +        //

typo-ed comment

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:428
(Diff revision 4)
> +
> +  let provider = NewTabUtils.activityStreamLinks;
> +  let {
> +    TRANSITION_TYPED,
> +    TRANSITION_LINK
> +  } = PlacesUtils.history;

or even
let TRANSITIONS = PlacesUtils.history.TRANSITIONS
and then use TRANSITIONS.TYPED (or a shorter name)

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:441
(Diff revision 4)
> +    // sort by url, frecency 200
> +    {uri: NetUtil.newURI("https://mozilla2.com/1"), visitDate: timeEarlier, transition: TRANSITION_TYPED},
> +    // sort by last visit date, frecency 200
> +    {uri: NetUtil.newURI("https://mozilla3.com/2"), visitDate: timeLater, transition: TRANSITION_TYPED},
> +    // sort by frecency, frecency 10
> +    {uri: NetUtil.newURI("https://mozilla4.com/3"), visitDate: timeLater, transition: TRANSITION_LINK}

fwiw, link is the default, and you could omit it.
Also removing newURI should save some space.
Attachment #8849239 - Flags: review?(mak77)
note, you have some failures on Try, but I can't tell if it's about the previous version or the new one.
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review132898

> nit: a possible alternative, when you only need to read and observe prefs, is to use defineLazyPreferenceGetter
> off-hand looks like the whole GridPrefs object could be thrown away and replaced with that. You could use its callback to check that the value is meaningful (or set the property to 1) and invoke AllPages.update().
> That would also save a small startup cost since you wouldn't need to init() it.

That's a great idea - we can handle this one in a follow-up
(Assignee)

Comment 14

2 years ago
(In reply to Marco Bonardo [::mak] from comment #12)
> note, you have some failures on Try, but I can't tell if it's about the
> previous version or the new one.

The latest try push I've got is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b98d83ec9558f9508eab7ad55986ba107c1bd8f5&group_state=expanded but I'm going to push another once I finish addressing the new review comments
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review132898

> So, if possible I'd prefer you to use the standard Bookmarks API to fetch a bookmark.
> First, in the bookmark notifications you should add the missing arguments, so you will also get the bookmark guid.
> Then pass the guid to this function.
> With the guid you can use PlacesUtils.bookmarks.fetch(guid) to get back a bookmark object having:
> guid, index, dateAdded, lastModified, type, title and url
> You can then filter the result.
> 
> What will be missing from here:
> the page guid: is this critical to have for you or would it be ok to set it to null for bookmarks, what do you use it for? Eventually we could fix fetch() to return a pageGuid property.
> the bookmark id: same question, actually you could pass-through the id from the notification, if needed. We are deprecating the use of database ids in favor of guids.
> frecency: it would not be a big deal to add it to Bookmarks.jsm::fetch so it gets returned

Great idea! We're not using any of the missing fields so it won't be a problem to use the Bookmarks API
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review137276

You're good to go, just a few minor comments.
sorry for the reviews delays, lately my reviews queue grows quickly.

::: toolkit/components/places/tests/PlacesTestUtils.jsm:97
(Diff revision 6)
> +    // If no favicons were provided, we do not want to continue on
> +    if (!faviconURLs) {
> +      throw new Error("No favicon URLs were provided");
> +    }
> +
> +    faviconURLs.forEach((val, key) => {

forEach is slower than for...of (and fwiw the classic for loop is even faster, but also less readable), so if possible please try to avoid .forEach.
That said, this is test code, so it's a nit, but please take that into account for the product code.

::: toolkit/modules/NewTabUtils.jsm:28
(Diff revision 6)
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "BinarySearch",
>    "resource://gre/modules/BinarySearch.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
> +  "resource://gre/modules/NetUtil.jsm");

Let's use Services.io.newURI instead, and avoid importing NetUtil. modules have a cost, when we can avoid them easily we should (I know, we didn't do a great job in the past).

::: toolkit/modules/NewTabUtils.jsm:937
(Diff revision 6)
> +
> +  /*
> +   * Initializes Activity Stream provider - adds a history observer and a
> +   * bookmarks observer.
> +   */
> +  init: function ActivityStreamProvider_init() {

nit: use ES6 init() shorthand (there is no more need to further label each method from when the js debugger was fixed some years ago)

::: toolkit/modules/NewTabUtils.jsm:1077
(Diff revision 6)
> +   *
> +   * @param {String} aGuid
> +   *          A bookmark guid to use as a refrence to fetch the bookmark
> +   */
> +  getBookmark: Task.async(function* ActivityStreamProvider_getBookmark(aGuid) {
> +    let bookmark = yield PlacesUtils.bookmarks.fetch({guid: aGuid});

just .fetch(aGuid) should work, the API accepts a guid OR an object containing a guid (or an url)

::: toolkit/modules/NewTabUtils.jsm:1108
(Diff revision 6)
> +   * Gets Bookmarks count
> +   *
> +   * @returns {Promise} Returns a promise with the count of bookmarks
> +   */
> +  getBookmarksSize: Task.async(function* ActivityStreamProvider_getBookmarksSize() {
> +    let sqlQuery = `SELECT count(*) FROM moz_bookmarks WHERE type = :type`;

not sure if it matters for you, but this will double count tags, since tags (currently) are just copies of the original bookmark.
Excluding tags for now involves joining the table with itself and checking that the grandparent is not the tags root.

::: toolkit/modules/NewTabUtils.jsm:1131
(Diff revision 6)
> +   */
> +  executePlacesQuery: function ActivityStreamProvider_executePlacesQuery(aQuery, aOptions = {}) {
> +    let {columns, params} = aOptions;
> +    let items = [];
> +    let queryError = null;
> +    return Task.spawn(function*() {

This would probably be cleaner as:

executePlacesQuery: Task.async(function* (aQuery, aOptions = {}) {
  ...

::: toolkit/modules/NewTabUtils.jsm:1173
(Diff revision 6)
> +   * Block a url
> +   *
> +   * @param {Object} aLink
> +   *          The link which contains a URL to add to the block list
> +   */
> +  blockURL: function ActivityStreamLinks_blockURL(aLink) {

nit: various new methods could now use ES shorthands.
Attachment #8849239 - Flags: review?(mak77) → review+
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8849239 [details]
Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs

https://reviewboard.mozilla.org/r/122054/#review137276

> not sure if it matters for you, but this will double count tags, since tags (currently) are just copies of the original bookmark.
> Excluding tags for now involves joining the table with itself and checking that the grandparent is not the tags root.

We're aware of the potential inaccuracy of this query, and decided that we don't actually care all that much. Really this is a helper function for tests and was mostly used just to give us a feel of if people are bookmarking a lot or not. So the exact count doesn't really matter to us. If we later decide that we need an exact number for our data collection, we can refine this query to give us a more accurate count.
(In reply to Ursula Sarracini (:ursula) from comment #19)
> was mostly used just to give us a feel of if people are
> bookmarking a lot or not.

We have telemetry about number of bookmarks in the telemetry dashboard (look for the PLACES_ probes), we could add more.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1360316
Comment hidden (mozreview-request)

Comment 23

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6eb5ab801417 -d b2572094f9ff: rebasing 392393:6eb5ab801417 "Bug 1345122 - Update NewTabUtils.jsm for Activity Stream's needs r=mak" (tip)
local [dest] changed browser/components/newtab/PlacesProvider.jsm which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging browser/components/nsBrowserGlue.js
merging toolkit/modules/tests/xpcshell/xpcshell.ini
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)

Comment 25

2 years ago
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b57b968ea784
Update NewTabUtils.jsm for Activity Stream's needs r=mak

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b57b968ea784
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1360854
You need to log in before you can comment on or make changes to this bug.