Add Activity Stream related fields to places

RESOLVED FIXED in Firefox 56

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nanj, Assigned: nanj)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Activity Stream currently requires following metadata fields for the MVP,

* A rich icon, used in Top Sites. 128px by 128px for retina, 64px by 64px for non-retina screens
* A color analysis of the icon/favicon, indicating most appropriate/primary color
* A description of the site, used for Highlight cards
* A preview image URL, i.e. the URL of representative image of the page, used for Highlight cards

Fortunately, the first two fields (the rich icon and its color) are already covered by bug-977177 (https://bugzilla.mozilla.org/show_bug.cgi?id=977177). So we only need to add the "description" and "preview image".

Since the new fields are both of the type "TEXT", and won't take space as much as favicon data URIs. We are looking to add those new columns to the "moz_places" table, other than adding a new table for them.

In addition, like other tables in Places, we will need to implement the services to allow users to get/set those metadata fields.

Notes:

[1]. Both "description" and "preview iamge" are just "dumb" data fileds, and won't be used as the query keys, thus no need to attach the index to them.
[2]. No other expiration policy needed, the default one is fine.
[3]. A-S will populate those two fields once it parses the page. Also, it could populate the favicon and color fields if needed. 
[4]. Need to confirm with the Sync team and the Android team to figure out the side effect of this change.
(Assignee)

Updated

2 years ago
Assignee: nobody → najiang
(Assignee)

Updated

2 years ago
Depends on: 977177

Comment 1

2 years ago
(In reply to Nan Jiang [:nanj] from comment #0)
> In addition, like other tables in Places, we will need to implement the
> services to allow users to get/set those metadata fields.
Would it be better for places to be adding these values instead of exposing an API? In particular thinking about the existing `title` column and a pretty closely related `description`, we could use a single query to insert/update both instead of having places create a page with title and later waiting for page creation before adding the description.

In terms of other metadata that might require more async processing, I'm not sure if that favors exposing an API and potentially also having a separate table?
(Assignee)

Comment 2

2 years ago
(In reply to Ed Lee :Mardak from comment #1)

> Would it be better for places to be adding these values instead of exposing
> an API? In particular thinking about the existing `title` column and a
> pretty closely related `description`, we could use a single query to
> insert/update both instead of having places create a page with title and
> later waiting for page creation before adding the description.

Yes, we definitely want to do that if all the metadata is available upon places inserts. But that means we have to hook up the metadata parsing to the existing page parsing, which should be feasible for 'description', but unsure for those fields like 'preview image'. Another user case for API is for those ad hoc metadata persistences, for instance, the user bookmarks a history link.

> > In terms of other metadata that might require more async processing, I'm not
> sure if that favors exposing an API and potentially also having a separate
> table?

I think having a separate table is a bit waste right now, given that there are only two new fields needed. Though I could be convinced otherwise if that'll significantly simplify this schema migration.
(Assignee)

Comment 3

2 years ago
Just double checked with Android folks, this change won't affect them as they're using a different Places implementation. Eventually they may apply the same schema change to Activity Stream for Android.
Just to annotate some points to take into account from yesterday's discussion with Nan:

1. fields should be limited, as far as possible. We are removing large (like blobs) stuff from the db because the locationbar still needs linear scans, the smaller the db the better is. I'm fine with storing somehow-limited text fields. Indices on large text fields are out of the question (I also removed the index from url because it was too large). Based on the discussion I think we are fine here.

2. Private Browsing. In PB mode Places doesn't store anything about history (we still store bookmarks and their info though), so if you expect to store something during PB we should evaluate what to do. The most problematic case is users in permanent PB mode (disabled history). Nan said for now there's no plan to store anything in PB mode, so we should be fine.

3. Expiration currently is db size based (http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/toolkit/components/places/nsPlacesExpiration.js#77 actually being changed to 60MiB after bug 977177). The reason to not use a time threshold (n-months) is again the fact the location bar needs to quickly make linear scans of moz_places, and not all the users browse the same way, heavy browsing users would end up with a too large db to be efficient. You should make a guess of the average increase your new fields are bringing (or do some real-world testing) and increase that size limit, or we'd start expiring history to make space for the new data. Alternatively, we need to find a better strategy for expiration.
as a point of reference, a 60MiB DB contains about 100k places with a normally sized bookmarks set (There are some ways to reduce the size in bug 977149 and it could happen in the future, but no resources to do that now).
(Assignee)

Comment 6

2 years ago
Just a quick update that I've been working on this bug and will be pushing some patches to try/reviewboard shortly.

Here is a quick breakdown of this work,

* [Done] Add a new database migration
* [Done] Update the existing APIs for this change
         * `updatePlaces/getPlacesInfo` in "mozIAsyncHistory.idl"
         * `insert/insertMany` in "History.jsm"
* [WIP]  Add some helper APIs for the new fields, e.g. `SetDescriptionForPage` and `SetPreviewImageForPage`
* [WIP]  Tune the data expiration for Places

Hey mak, would you be able to review these patches for me by any chance? Before wrapping up all the above items and sending the whole patch to review, I'd like to hear from you first :)
Flags: needinfo?(mak77)
(In reply to Nan Jiang [:nanj] from comment #0)
> [4]. Need to confirm with the Sync team and the Android team to figure out
> the side effect of this change.

I haven't seen the implications for Sync discussed here. Even if you don't want the data synced (which would be surprising), there's still the reality that Sync itself may change existing records, so the implications of losing this data must be considered.
(Assignee)

Comment 8

2 years ago
(In reply to Mark Hammond [:markh] from comment #7)
> I haven't seen the implications for Sync discussed here. Even if you don't
> want the data synced (which would be surprising), there's still the reality
> that Sync itself may change existing records, so the implications of losing
> this data must be considered.

Good catch! Thanks for pointing this out, markh!

Having Sync to make those new fields available across multiple devices is critical for Activity Stream. Actually, A-S already benefits a lot from Sync to generate the most recent highlights for the users.

IIRC, Sync uses `mozIAsyncHistory.updatePlaces` to load those synced records into Places. I've update that function in my patch to populate the "description" and "preview_image_url" if they're presented. Otherwise, it'll leave the existing values alone. As a side note, A-S will be responsible for parsing the visited page, extracting those two fields if any, and saving them to Places.

In order to make these new fields syncable, I assume we need some other changes on the Sync side, correct? We'd like to hear more details from you, markh. Thanks in advance!
(In reply to Nan Jiang [:nanj] from comment #8)
> Having Sync to make those new fields available across multiple devices is
> critical for Activity Stream. Actually, A-S already benefits a lot from Sync
> to generate the most recent highlights for the users.

We had a bit of a chat with the AS team in Hawaii - the tl;dr of this is that "it's tricky".

> IIRC, Sync uses `mozIAsyncHistory.updatePlaces` to load those synced records
> into Places. I've update that function in my patch to populate the
> "description" and "preview_image_url" if they're presented. Otherwise, it'll
> leave the existing values alone. As a side note, A-S will be responsible for
> parsing the visited page, extracting those two fields if any, and saving
> them to Places.
> 
> In order to make these new fields syncable, I assume we need some other
> changes on the Sync side, correct? We'd like to hear more details from you,
> markh. Thanks in advance!

Sync currently moves a subset of data around between devices - there are specific fields we look at for history and bookmarks. While adding new fields is relatively easy at face value, it quickly gets tricky:

* Sync may move data between (say) 2 x desktop version 55 and 1 x desktop version 54. Version 55 may sync a new field to the server, but if version 54 then writes the same record back, that new field will be lost on the server. Thus, only 1 of the 55 desktops may have the new field (and any new 55 devices added to the account may not get these new fields).

* Sync may move data between mobile and desktop, which has the same problem as above - if the mobile platforms aren't aware of these fields, we end up in the same situation - new fields are lost.

I wish sync was architected differently, but it is what it is. To do this properly probably means a coordinated effort between desktop and the 2 mobile platforms, over a number of cycles, so a new "schema" settles down and round-trips correctly.
(Assignee)

Comment 10

2 years ago
(In reply to Mark Hammond [:markh] from comment #9)

> I wish sync was architected differently, but it is what it is. To do this
> properly probably means a coordinated effort between desktop and the 2
> mobile platforms, over a number of cycles, so a new "schema" settles down
> and round-trips correctly.

Agreed! A cross team effort is needed to make this right. I've already talked to some Android/iOS folks about this schema change, since they are all using the separate Places schema, it gets trickier to sync this to all platforms. Let's schedule a meeting with all the stakeholders to cover more details so that every team could stay on the same page.

Thanks again markh for your inputs!
(In reply to Nan Jiang [:nanj] from comment #6)
> * [Done] Update the existing APIs for this change
>          * `updatePlaces/getPlacesInfo` in "mozIAsyncHistory.idl"
>          * `insert/insertMany` in "History.jsm"

How are these expected to work, both of these APIs are intended for insertions that don't happen on visit, but I'd expect the most common case for Firefox is to insert new visits from the docshell, that doesn't use these code paths.

> * [WIP]  Add some helper APIs for the new fields, e.g.
> `SetDescriptionForPage` and `SetPreviewImageForPage`

Where are these implemented? History.jsm?

> * [WIP]  Tune the data expiration for Places

What's the plan?
Flags: needinfo?(mak77)
(Assignee)

Comment 12

2 years ago
(In reply to Marco Bonardo [::mak] from comment #11)

> How are these expected to work, both of these APIs are intended for
> insertions that don't happen on visit, but I'd expect the most common case
> for Firefox is to insert new visits from the docshell, that doesn't use
> these code paths.

Updating these APIs along with this schema change is mainly due to:

* According to http://searchfox.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#1319, looks like `mozIAsyncHistory.getPlacesInfo` is the preferred API to fetch the places info (title, description, and preview image url).

* Since these two fields are also being used by Activity Stream on Firefox Andriod/iOS, we're looking to leverage Sync to sync those fields across multiple platforms, that's why we've also updated `mozIAsyncHistory.updatePlaces` here. As markh mentioned earlier, API update is only one part of the whole story, it needs some extra work from other teams as well, we'll schedule a meeting to discuss this in more details soon.

So the plan for the desktop AS is that we'll be attaching a frame script to each visited page (except the private browsing), by which these two fields get extracted. Upon the successful of each extraction, it persists the results by calling those async APIs `SetDescriptionForPage` and `SetPreviewImageForPage`. So it won't be talking to the docshell, nor to the API `mozIAsyncHistory.updatePlaces`.

> Where are these implemented? History.jsm?

Actually, We'd like to hear your thoughts on this one. We'd like to implement those APIs in an async fashion same as those in `mozIAsyncHistory.idl`. As you mentioned above, that idl is perhaps not the appropriate home for those new APIs. Also, the `history.cpp` is already a bloated file with over 3K loc.

Do you have any suggestions on this? Adding those APIs to `History.jsm` seems fine, but shall we just add a new idl and a new C++ source file to implement those functions?

> What's the plan?

Firstly, we plan to estimate the average increase of this change bringing to Places, then update the current expiry policy as taking those estimates into account. Fortunately, we've already collected some metrics on that in the Test Pilot version, however, it might be trickier than expected, and some followup tweaks are still needed once we know more about the storage impact on Places.
(In reply to Nan Jiang [:nanj] from comment #12)
> * According to
> http://searchfox.org/mozilla-central/source/toolkit/components/places/
> nsINavHistoryService.idl#1319, looks like `mozIAsyncHistory.getPlacesInfo`
> is the preferred API to fetch the places info (title, description, and
> preview image url).

This is going to change with bug 1350377 that will introduce PlacesUtils.history.fetch and likely remove getPlacesInfo. The good thing is that then you could work on js rather than cpp.
Do you need anything in particular from getPlacesInfo API that would not be served by fetch (see the discussion in the bug)?

> * Since these two fields are also being used by Activity Stream on Firefox
> Andriod/iOS, we're looking to leverage Sync to sync those fields across
> multiple platforms, that's why we've also updated
> `mozIAsyncHistory.updatePlaces` here. As markh mentioned earlier, API update
> is only one part of the whole story, it needs some extra work from other
> teams as well, we'll schedule a meeting to discuss this in more details soon.

Ok, so that needs Sync discussions first.

> > Where are these implemented? History.jsm?
> 
> Actually, We'd like to hear your thoughts on this one. We'd like to
> implement those APIs in an async fashion same as those in
> `mozIAsyncHistory.idl`. As you mentioned above, that idl is perhaps not the
> appropriate home for those new APIs. Also, the `history.cpp` is already a
> bloated file with over 3K loc.

History.cpp is mostly a cpp backend for things that need to do multiple operations off the main thread, updatePlaces does multiple inserts and fetches and thus it was the perfect fit. But it's also wrapped by insert and insertMany for js consumers sugar-y APIs.
In general, if an API doesn't need to do crazy amount of work off the main thread it's fine to directly implement it in History.jsm

> Do you have any suggestions on this? Adding those APIs to `History.jsm`
> seems fine, but shall we just add a new idl and a new C++ source file to
> implement those functions?

Unless you think something in cpp may need to access those APIs, you don't need to add idl or such things. I should update the code documentation to better explain that APIs are spread across nsINavHistoryService, History.jsm and mozIAsyncHistory.
In general js consumers won't care since everything is exposed through PlacesUtils.history.*
Using History.jsm sounds good so far.
(Assignee)

Comment 14

2 years ago
(In reply to Marco Bonardo [::mak] from comment #13)

> This is going to change with bug 1350377 that will introduce
> PlacesUtils.history.fetch and likely remove getPlacesInfo. The good thing is
> that then you could work on js rather than cpp.
> Do you need anything in particular from getPlacesInfo API that would not be
> served by fetch (see the discussion in the bug)?

Cool, good to know that `history.fetch` is now implemented! I think it's good enough for us and therefore `getPlacesInfo` is no longer needed. 

> Ok, so that needs Sync discussions first.

Nod, we'll arrange a meeting for this shortly.

> History.cpp is mostly a cpp backend for things that need to do multiple
> operations off the main thread, updatePlaces does multiple inserts and
> fetches and thus it was the perfect fit. But it's also wrapped by insert and
> insertMany for js consumers sugar-y APIs.
> In general, if an API doesn't need to do crazy amount of work off the main
> thread it's fine to directly implement it in History.jsm

You've absolutely found our pain point here! :)

So we intentionally avoid doing *ANY* Places queries on the main thread for following two reasons:
* The lesson that we learned from the AS Test Pilot version, those expensive queries do jank the browser.
* Some Firefox/Gecko folks have strongly recommended us to migrate all the IO ops (e.g. Places queries) to the worker thread in order to keep the main thread responsive.

Hence, we've decided to wrap *ALL* the Places queries (select&update) into various async APIs and execute them on the worker thread. This appears to be an overkill given that dispatching a query to the worker thread as well as converting xpcom values from js to cpp could be more expensive than simply running it on the main thread. The only argument I can image is that making worker thread busier is perhaps better than doing so on the main thread.

Also another question, is it true that hacking cpp is the only way to access to Places worker thread? I couldn't find any other reference of doing so from js.
(In reply to Nan Jiang [:nanj] from comment #14)
> So we intentionally avoid doing *ANY* Places queries on the main thread for
> following two reasons:

Wait, I think there's some misunderstanding here.
History.jsm doesn't run queries on the main thread, as well as History.cpp. Both are fine from that point of view.
What I was saying is that implementing stuff in History.cpp is needed when, apart from querying, you also need to do other additional work off the main thread. Then for how things are done today, your only choice would be cpp, because Sqlite is not exposed to javascript workers.

> Also another question, is it true that hacking cpp is the only way to access
> to Places worker thread? I couldn't find any other reference of doing so
> from js.

Yes, if you want to run custom code (not just queries!) off the main thread, you need to use cpp in Places, that's what we do in History.cpp or FaviconHelpers.cpp. But if you just need to run a query off the main-thread, that can be done from js without any problem, and that's what we do everywhere (but in some legacy APIs we are working on removing).
Sorry, I figured today I had some other questions regarding the preview image field. The url it's going to contain points to remote content, not local, right?
Just because it came to my mind we have cases where a places.sqlite is moved to a different profile, and local contents would be detached from their pointers then. But if it's a url to remote content there should be no problem.
Priority: -- → P2
(Assignee)

Comment 17

2 years ago
(In reply to Marco Bonardo [::mak] from comment #16)
> Sorry, I figured today I had some other questions regarding the preview
> image field. The url it's going to contain points to remote content, not
> local, right?

Yes, those are just remote URLs extracted directly from the page.
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
Hey mak, just a quick update that I've pushed following two patches to MozReview.

1. Add two columns `description` and `preview_image_url` to `moz_places`.
2. Add an API `History.setPageMetaInfo` as the setter, and update `History.fetch` as the getter.

While I am still working on the data expiry tuning one, we could get something to review in advance. 

Also, I've left out the commit which contains the changes about `mozIAsyncHistory.updatePlaces`, `History.insert`, and `History.insertMany` etc. I think we will revisit that once we have a clear view of cross-device support with Sync for this schema change.

Please let me know if you have any questions.
Nan, since we are close to the feature-freeze time for 55, and this patch is honestly scary for its expiration consequences, how bad would it be to delay landing to 56?

I'd also like to discuss the size limits, do we have evidence that 4096 chars brings a tangible benefit over, for example, 2k?
Can we make the preview image url respect places.history.maxUrlLength (default if doesn't exist is 2k)? We could expose Database::MaxUrlLength from nsINavHistoryService clearly.
Flags: needinfo?(najiang)
(Assignee)

Comment 22

2 years ago
(In reply to Marco Bonardo [::mak] from comment #21)
> Nan, since we are close to the feature-freeze time for 55, and this patch is
> honestly scary for its expiration consequences, how bad would it be to delay
> landing to 56?

Sure! As A-S is targeting 57, I think landing to 56 for this patch should be fine.

> I'd also like to discuss the size limits, do we have evidence that 4096
> chars brings a tangible benefit over, for example, 2k?
> Can we make the preview image url respect places.history.maxUrlLength
> (default if doesn't exist is 2k)? We could expose Database::MaxUrlLength
> from nsINavHistoryService clearly.

That's a fair point, choosing 4k for `description` is more or less a lazy copy from `MAX_TITLE_LENGTH`. Our telemetry suggests that the average size for `description` (en-US only) is approximately 200 characters, and 100 characters for `preview_image_url`, respectively. Taking L10N into account, I'd say 512 bytes for `description` should be fine.

As for `preview_image_url`, you're absolutely right, we should've applied MaxUrlLength on it. I forgot to do that, cause I was using `nsURI` which has an implicit size limiting in its constructor in my original patch.

Will fix them up soon.

Thanks for the feedback, mak!
Flags: needinfo?(najiang)
(In reply to Nan Jiang [:nanj] from comment #22)
> That's a fair point, choosing 4k for `description` is more or less a lazy
> copy from `MAX_TITLE_LENGTH`.

Oh wow, that's old code, we should also fix that, since I don't really want to store those long titles.
All of URI_LENGTH_MAX, TITLE_LENGTH_MAX and DB_TITLE_LENGTH_MAX look totally outdated and should be exposed from the history service for more general consumption.
Database::MaxUrlLength() is the only sane things around :(
We should make these more coherent and better exposed.

> Our telemetry suggests that the average size
> for `description` (en-US only) is approximately 200 characters, and 100
> characters for `preview_image_url`, respectively. Taking L10N into account,
> I'd say 512 bytes for `description` should be fine.

I'd also be fine with 1024 I guess, also for title we should use that limit.
(Assignee)

Comment 24

2 years ago
Oops, so I was looking at some deprecated code again? Nice catch! I figured that a constant defined in "nsNavHistory.h" and being used by multiple modules (e.g. "SetPageTitle") should be the canonical one. Haha, a poor assumption. 

Indeed, we need to clean it up to avoid further confusions ;)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 years ago
Hey all, here is a list of notes I kept from the "Activity Stream + Sync" meeting:

* We've agreed that these two fields won't be enabled for Sync in Firefox 57
* Let's make sure these new fields won't be included in the Sync payload
* Let's confirm that these fields won't get overwritten by Sync

Although we've decided not to enable Sync in the first iteration, we still believe that Sync could help A-S to improve the user experience in the long run. Further discussion and investigation are needed if we want to enable Sync for this Places change:

* Storage impact
  * for Sync server
  * for clients (desktop/mobile)
  * syncing all (particularly for those old history entries) seems unnecessary if A-S only needs the most recent ones

Please feel free to add your notes if I am missing anything here. Thank y'all!

Comment 27

2 years ago
mozreview-review
Comment on attachment 8871279 [details]
Bug 1352502 - Part 1. Add `description` and `preview_image_url` to Places.

https://reviewboard.mozilla.org/r/142776/#review156228

::: toolkit/components/places/tests/migration/test_current_from_v38.js:22
(Diff revision 1)
> +
> +  // Manually update these two fields for a places record.
> +  yield db.execute(`
> +    UPDATE moz_places
> +    SET description = :description, preview_image_url = :previewImageURL
> +    WHERE id = 1`, { description: "Page description",

I'd prefer if you'd PlacesTestUtils.addVisit an actual entry, and update that one selecting by url_hash and url.

Otherwise, it would probably justb enough to SELECT the 2 new columns and if the query succeeds everything should be fine, we don't have to test that Sqlite works, that's Sqlite team job :)
Attachment #8871279 - Flags: review?(mak77) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8871280 [details]
Bug 1352502 - Part 2. Add API to update the page meta info for Places.

https://reviewboard.mozilla.org/r/142778/#review156270

I'm suggesting an alternative API, if you think something's wrong there, please let me know.
My main point is that I'd like to reduce the number of structures to remember around.

::: toolkit/components/places/History.jsm:634
(Diff revision 2)
> +   *      If `pageMetaInfo` has neither `description` nor `previewImageURL`.
> +   * @throws (Error)
> +   *      If the length of `pageMetaInfo.previewImageURL` is greater than
> +   *      DB_PREVIEW_IMAGE_URL_MAX.
> +   */
> +  setPageMetaInfo(guidOrURI, pageMetaInfo) {

I'd like to go a bit more towards APIs simplification. You may disagree and that'd be fine, we can discuss about this :)

I'd suggest to throw away the pageMetaInfo object, let's just use PageInfo. Make this just update(), and let it accept a PageInfo. The scope is so that most of the new APIs have similar names (so you know there's PlacesUtils.bookmarks.update,  PlacesUtils.history.update...)

We should be clear that at this time we'll only support updating description and previewImageURL, while other fields in the object will be ignored.
if both guid and url are set in the pageinfo object, guid will win.

the "includePageMeta" option will just become "includeMeta"

validatePageMetaInfo should just be merged into validatePageInfo (before the !validateVisits check).

I think we should also move the !info || !typeof == object directly inside validatePageInfo, and remove from insert() as well.

::: toolkit/components/places/History.jsm:717
(Diff revision 2)
> + */
> +function validatePageMetaInfo(pageMetaInfo) {
> +  let info = {
> +  };
> +
> +  if (typeof pageMetaInfo.description === "string" || pageMetaInfo.description === null) {

I suppose you want to do something to handle empty string vs null. You could either convert that here, or handle that in the query itself using a NULLIF(description, "")

The validator should probably also slice on DB_DESCRIPTION_LENGTH_MAX.

::: toolkit/components/places/History.jsm:724
(Diff revision 2)
> +  } else if (pageMetaInfo.description !== undefined) {
> +    throw new TypeError(`description property of pageMetaInfo object: ${pageMetaInfo.description} must be a string if provided`);
> +  }
> +
> +  if (pageMetaInfo.previewImageURL) {
> +    info.previewImageURL = PlacesUtils.normalizeToURLOrGUID(pageMetaInfo.previewImageURL);

normalizeToURLOrGUID is not strict enough, if I pass a guid here I'll just break this.
a good validato example may be BOOKMARK_VALIDATORS.url:
http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/toolkit/components/places/PlacesUtils.jsm#240

(yes, we should share more code between the various validators, I'm sorry).

::: toolkit/components/places/History.jsm:1392
(Diff revision 2)
> + * via PlacesUtils.validatePageMetaInfo.
> + *
> + * @param pageMetaInfo: (PageMetaInfo)
> + * @return (info)
> + */
> +function convertForSetPageMetaInfo(pageMetaInfo) {

This should not be needed, the validator should already either discard input or transform it.

::: toolkit/components/places/History.jsm:1440
(Diff revision 2)
> +    updateFragments.push("preview_image_url = :previewImageURL");
> +  }
> +  let query = `UPDATE moz_places
> +               SET ${updateFragments.join(", ")}
> +               ${whereClauseFragment}`;
> +  await db.execute(query, info);

you may want to extract the strictly needed properties from the info object, just in case.
Attachment #8871280 - Flags: review?(mak77)
(Assignee)

Comment 29

2 years ago
mozreview-review-reply
Comment on attachment 8871280 [details]
Bug 1352502 - Part 2. Add API to update the page meta info for Places.

https://reviewboard.mozilla.org/r/142778/#review156270

> I'd like to go a bit more towards APIs simplification. You may disagree and that'd be fine, we can discuss about this :)
> 
> I'd suggest to throw away the pageMetaInfo object, let's just use PageInfo. Make this just update(), and let it accept a PageInfo. The scope is so that most of the new APIs have similar names (so you know there's PlacesUtils.bookmarks.update,  PlacesUtils.history.update...)
> 
> We should be clear that at this time we'll only support updating description and previewImageURL, while other fields in the object will be ignored.
> if both guid and url are set in the pageinfo object, guid will win.
> 
> the "includePageMeta" option will just become "includeMeta"
> 
> validatePageMetaInfo should just be merged into validatePageInfo (before the !validateVisits check).
> 
> I think we should also move the !info || !typeof == object directly inside validatePageInfo, and remove from insert() as well.

Makes sense. I like this idea to keep it simple and extensible. Will change the implementation as such.

> I suppose you want to do something to handle empty string vs null. You could either convert that here, or handle that in the query itself using a NULLIF(description, "")
> 
> The validator should probably also slice on DB_DESCRIPTION_LENGTH_MAX.

Nice to know that there is a built-in "NULLIF" to handle this.

Yes, merging "validation" and "conversion" will solve it.

> you may want to extract the strictly needed properties from the info object, just in case.

`convertForSetPageMetaInfo` is responsible for creating the `info` object, and handling the extraction here. Since we're going to merge it into validator, I'll make some changes here if needed.
Comment hidden (mozreview-request)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8871280 [details]
Bug 1352502 - Part 2. Add API to update the page meta info for Places.

https://reviewboard.mozilla.org/r/142778/#review157140

Please check issues on mozreview, there's one open issue also for the other changeset, I'd suggest to mark them as fixed once you uplift a new version fixing them, so it's easier to track remaining work.

::: toolkit/components/places/History.jsm:31
(Diff revision 3)
> + * - description: (string)
> + *     The description of the page, if any.
> + * - previewImageURL: (URL)
> + *     or (nsIURI)
> + *     or (string)
> + *     The preview image URL of the page, if any.

This is great, there's one missing bit though, now that the pageInfo oject may include this, insert and insertMany should be able to also insert this information in one step (with tests) if it's passed-in.

If you don't want to do this here, I'd also be fine with a follow-up provided we fix it shortly after.

::: toolkit/components/places/History.jsm:577
(Diff revision 3)
>  
> -  /**
> +   /**
> +   * Update information for a page.
> +   *
> +   * Currently, it supports updating the description and the preview image URL
> +   * for a page, any other fields will be ingored.

typo ingored

::: toolkit/components/places/PlacesUtils.jsm:1030
(Diff revision 3)
> +      } else if (typeof(previewImageURL) === "string" && previewImageURL.length <= DB_URL_LENGTH_MAX) {
> +        info.previewImageURL = new URL(previewImageURL);
> +      } else if (previewImageURL instanceof Ci.nsIURI && previewImageURL.spec.length <= DB_URL_LENGTH_MAX) {
> +        info.previewImageURL = new URL(previewImageURL.spec);
> +      } else if (previewImageURL instanceof URL && previewImageURL.href.length <= DB_URL_LENGTH_MAX) {
> +        info.previewImageURL = previewImageURL;

You can probably unify the string and URL paths (you CAN invoke URL() on an URL, it's just a no-op).
Attachment #8871280 - Flags: review?(mak77)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1375874

Comment 33

2 years ago
mozreview-review
Comment on attachment 8871280 [details]
Bug 1352502 - Part 2. Add API to update the page meta info for Places.

https://reviewboard.mozilla.org/r/142778/#review157232

::: toolkit/components/places/PlacesUtils.jsm:213
(Diff revision 4)
>  
>    return JSON.stringify(data);
>  }
>  
>  // Imposed to limit database size.
>  const DB_URL_LENGTH_MAX = 65536;

please file a bug to fix this, I can eventually look into that in the next weeks.

::: toolkit/components/places/PlacesUtils.jsm:1028
(Diff revision 4)
> +      if (previewImageURL === null) {
> +        info.previewImageURL = null;
> +      } else if ((typeof(previewImageURL) === "string" && previewImageURL.length <= DB_URL_LENGTH_MAX) ||
> +                 (previewImageURL instanceof URL && previewImageURL.href.length <= DB_URL_LENGTH_MAX)) {
> +        info.previewImageURL = new URL(previewImageURL);
> +      } else if (previewImageURL instanceof Ci.nsIURI && previewImageURL.spec.length <= DB_URL_LENGTH_MAX) {

ok, what I was thinking (pseudocode) is:
if (null)
else if (nsIURI)
else {
  new URL(value);
  ...
}

The new URL call should basically already do the checks and throw for us.

::: toolkit/components/places/tests/history/test_fetch.js:82
(Diff revision 4)
>    }
>  });
>  
> +add_task(async function test_fetch_page_meta_info() {
> +  await PlacesTestUtils.clearHistory();
> +  await PlacesUtils.bookmarks.eraseEverything();

you're not touching bookmarks, so this should not be needed (maybe the other tests are also doing this wrong, dunno, regardless it's not useful here)
Attachment #8871280 - Flags: review?(mak77) → review+
please have a green-ish Try Run of this, then it will be fine!
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8871280 [details]
Bug 1352502 - Part 2. Add API to update the page meta info for Places.

https://reviewboard.mozilla.org/r/142778/#review157232

> ok, what I was thinking (pseudocode) is:
> if (null)
> else if (nsIURI)
> else {
>   new URL(value);
>   ...
> }
> 
> The new URL call should basically already do the checks and throw for us.

Hmm, looks like `new URL(value)` uses a different length limit than "DB_URL_LENGTH_MAX". I couldn't find the exact limit in the documentation, but I managed to create a new URL object with 1 MB in Firefox.

I think we'll just stick to DB_URL_LENGTH_MAX here.
(Assignee)

Comment 36

2 years ago
mozreview-review
Comment on attachment 8871280 [details]
Bug 1352502 - Part 2. Add API to update the page meta info for Places.

https://reviewboard.mozilla.org/r/142778/#review157206

::: toolkit/components/places/History.jsm:31
(Diff revision 3)
> + * - description: (string)
> + *     The description of the page, if any.
> + * - previewImageURL: (URL)
> + *     or (nsIURI)
> + *     or (string)
> + *     The preview image URL of the page, if any.

Agreed, both insert and insertMany should be able to handle those new columns in PageInfo.

In fact, I've already made a patch for `mozIAsyncHistory.updatePlaces` as well as `insert/insertMany`, didn't include it in this review though since the original API design was different.

So yes, I'd like to do this in a follow-up bug if possible, shouldn't take too much effert.

::: toolkit/components/places/PlacesUtils.jsm:1030
(Diff revision 3)
> +      } else if (typeof(previewImageURL) === "string" && previewImageURL.length <= DB_URL_LENGTH_MAX) {
> +        info.previewImageURL = new URL(previewImageURL);
> +      } else if (previewImageURL instanceof Ci.nsIURI && previewImageURL.spec.length <= DB_URL_LENGTH_MAX) {
> +        info.previewImageURL = new URL(previewImageURL.spec);
> +      } else if (previewImageURL instanceof URL && previewImageURL.href.length <= DB_URL_LENGTH_MAX) {
> +        info.previewImageURL = previewImageURL;

Gotcha!
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 38

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3d588be9bd87
Part 1. Add `description` and `preview_image_url` to Places. r=mak
https://hg.mozilla.org/integration/autoland/rev/309b8e7ad7f0
Part 2. Add API to update the page meta info for Places. r=mak
Keywords: checkin-needed

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d588be9bd87
https://hg.mozilla.org/mozilla-central/rev/309b8e7ad7f0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

2 years ago
Blocks: 1397789
You need to log in before you can comment on or make changes to this bug.