Remove nsIDownloadHistory

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: mak, Assigned: standard8)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments)

Blocks: 1387446
(Assignee)

Updated

10 months ago
Depends on: 1419704
(Assignee)

Updated

10 months ago
Depends on: 1473533
(Assignee)

Updated

10 months ago
Depends on: 1473607
(Assignee)

Updated

10 months ago
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: P3 → P1
(Assignee)

Updated

9 months ago
Depends on: 1474637
(Assignee)

Updated

9 months ago
Depends on: 1474638
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Blocks: 1479053
(Reporter)

Comment 4

9 months ago
mozreview-review
Comment on attachment 8995546 [details]
Bug 1468980 - Add an annotations parameter to PlacesUtils.history.update to retrieve any annotations.

https://reviewboard.mozilla.org/r/259950/#review267232

::: toolkit/components/places/History.jsm:641
(Diff revision 1)
>     *
> +   *      If a property `annotations` is provided, the annotations will be
> +   *      updated. Note that:
> +   *      1). It should be a Map containing key/value pairs to be updated.
> +   *      2). If the value is falsy, the annotation will be removed.
> +   *      3). If the value is non-falsy, the annotation will be added or updated.

Maybe worth a few words about which value types are supported?

::: toolkit/components/places/History.jsm:659
(Diff revision 1)
>     * @throws (Error)
>     *      If the length of `pageInfo.previewImageURL` is greater than
>     *      DB_URL_LENGTH_MAX defined in PlacesUtils.jsm.
>     */
>    update(pageInfo) {
> -    let info = PlacesUtils.validatePageInfo(pageInfo, false);
> +    let info = PlacesUtils.validatePageInfo(pageInfo, false, true);

it looks like validatePageInfo could automatically detect the existence of an annotations property and validate it if it exists.

It made more sense for visits, because visits is mandatory in normal validations.

::: toolkit/components/places/History.jsm:1481
(Diff revision 1)
> +      let annosToRemove = [];
> +      let annosToUpdate = [];
> +
> +      for (let anno of pageInfo.annotations) {
> +        anno[1] ? annosToUpdate.push(anno[0]) : annosToRemove.push(anno[0]);
> +      }

this could be out of the transaction. Transactions must contain as less code as possible.

::: toolkit/components/places/History.jsm:1494
(Diff revision 1)
> +        for (let anno of annosToUpdate) {
> +          let content = pageInfo.annotations.get(anno);
> +          // TODO: We only really need to save the type whilst we still support
> +          // accessing page annotations via the annotation service.
> +          let type = typeof content == "string" ? Ci.nsIAnnotationService.TYPE_STRING :
> +            Ci.nsIAnnotationService.TYPE_INT64;

Do we validate this? Since Sqlite can store really anything in a column, it looks like considering everything INT64 may end up storing unwanted things.

I think the validator could actually do something here, maybe discard anyting that is not Boolean, Number or string. Trying to set an array or an object as annotation should be a no-no.

::: toolkit/components/places/History.jsm:1497
(Diff revision 1)
> +          // accessing page annotations via the annotation service.
> +          let type = typeof content == "string" ? Ci.nsIAnnotationService.TYPE_STRING :
> +            Ci.nsIAnnotationService.TYPE_INT64;
> +          let date = PlacesUtils.toPRTime(new Date());
> +
> +          await db.execute(`

please add a comment that this will replace the id everytime, and that's not a problem until we don't need to join on it.

::: toolkit/components/places/History.jsm:1509
(Diff revision 1)
> +                    :last_modified)
> +          `, {
> +            ...baseParams,
> +            anno_name: anno,
> +            content,
> +            expiration: PlacesUtils.history.EXPIRE_NEVER,

Is this a typo? shoulnd't it be PlacesUtils.annotations.EXPIRE_NEVER? Is this missing a test? (surely once the old API goes away this can be not set)

::: toolkit/components/places/History.jsm:1513
(Diff revision 1)
> +            content,
> +            expiration: PlacesUtils.history.EXPIRE_NEVER,
> +            type,
> +            // The date fields are unused, so we just set them both to the latest.
> +            date_added: date,
> +            last_modified: date

nit: comma dangle

::: toolkit/components/places/History.jsm:1529
(Diff revision 1)
> +          WHERE place_id = (SELECT id FROM moz_places WHERE ${whereClauseFragment})
> +          AND anno_attribute_id =
> +            (SELECT id FROM moz_anno_attributes WHERE name = :anno_name)
> +        `, { ...baseParams,
> +          anno_name: anno,
> +        });

could you please oneline this object?

::: toolkit/components/places/PlacesUtils.jsm:1096
(Diff revision 1)
> +
> +      if (pageInfo.annotations.size == 0) {
> +        throw new TypeError("there must be at least one annotation");
> +      }
> +
> +      info.annotations = pageInfo.annotations;

As I said before, we should do some checks about keys and values, at least check keys are strings and values are boolean, number or strings. (we can also accept null as value)

::: toolkit/components/places/nsPlacesTables.h:51
(Diff revision 1)
>      ", PRIMARY KEY (place_id, input)" \
>    ")" \
>  )
>  
> +// Note: dateAdded and lastModified should be considered deprecated but are
> +// kept to ease backwards compatibility.

well, also flags, expiration and type are deprecated.
Attachment #8995546 - Flags: review?(mak77)
(Reporter)

Comment 5

9 months ago
mozreview-review
Comment on attachment 8995547 [details]
Bug 1468980 - Change downloads code to save downloads via the new async history API.

https://reviewboard.mozilla.org/r/259952/#review267242

::: browser/components/downloads/DownloadsCommon.jsm:738
(Diff revision 1)
>  
>      this.oldDownloadStates.set(download,
>                                 DownloadsCommon.stateOfDownload(download));
>    },
>  
> -  onDownloadChanged(download) {
> +  async onDownloadChanged(download) {

I'd prefer to not make this async directly, because onDownloadChanged is not really async, no consumer handles rejections.

I think I'd rather start a new async function inside it and .catch internally. Otherwise we should just not await for it and .catch the updateMetaData call; it doesn't look critical, unless we use the event in tests to check for the metadata?

::: toolkit/components/downloads/DownloadCore.jsm:1717
(Diff revision 1)
>     * started, to add it to the browsing history.  This method has no effect if
>     * the download is private.
>     */
> -  addToHistory() {
> -    if (this.download.source.isPrivate) {
> -      return;
> +  async addToHistory() {
> +    if (AppConstants.MOZ_PLACES) {
> +      await DownloadHistory.addDownloadToHistory(this.download);

Should this .catch? It's clear now addToHistory could reject, and since we await it somewhere, it could end up interrupting the download process.
While history is important, it's not critical to a download and should not interrupt the process.
Thus, I'd probably just catch and reportError here

::: toolkit/components/downloads/DownloadHistory.jsm:96
(Diff revision 1)
> +    if (download.source.isPrivate ||
> +        !PlacesUtils.history.canAddURI(PlacesUtils.toURI(download.source.url))) {
> +      return;
> +    }
> +
> +    let targetUri = NetUtil.newURI(new FileUtils.File(

directly use Services.io.newFileURI?

::: toolkit/components/downloads/DownloadHistory.jsm:111
(Diff revision 1)
> +      }]
> +    });
> +    await PlacesUtils.history.update({
> +      annotations: new Map([["downloads/destinationFileURI", targetUri.spec]]),
> +      guid: pageInfo.guid,
> +      url: pageInfo.url,

why are both guid and url necessary? one should be enough

::: toolkit/components/downloads/DownloadHistory.jsm:505
(Diff revision 1)
> +   * Updates the download history item when the meta data or destination file
> +   * changes.
> +   *
> +   * @param {String} sourceUrl The sourceUrl which was updated.
> +   */
> +  updateForDataChange(sourceUrl) {

data is a bit generic since this is a download, let's keep Metadata in the name
Attachment #8995547 - Flags: review?(mak77)
(Reporter)

Comment 6

9 months ago
mozreview-review
Comment on attachment 8995548 [details]
Bug 1468980 - Remove nsIDownloadHistory.idl.

https://reviewboard.mozilla.org/r/259954/#review267244

::: toolkit/components/places/History.cpp
(Diff revision 1)
> -
> -      nsAutoString destinationFileName;
> -      rv = destinationFile->GetLeafName(destinationFileName);
> -      NS_ENSURE_SUCCESS(rv, rv);
> -
> -      rv = mHistory->SetURITitle(source, destinationFileName);

interesting, is the new code doing this? Doesn't look like... We should check if this is breaking some ui.
Attachment #8995548 - Flags: review?(mak77) → review+
(Assignee)

Comment 7

9 months ago
mozreview-review-reply
Comment on attachment 8995547 [details]
Bug 1468980 - Change downloads code to save downloads via the new async history API.

https://reviewboard.mozilla.org/r/259952/#review267242

> why are both guid and url necessary? one should be enough

Filed bug 1479445
(Assignee)

Comment 8

9 months ago
mozreview-review-reply
Comment on attachment 8995548 [details]
Bug 1468980 - Remove nsIDownloadHistory.idl.

https://reviewboard.mozilla.org/r/259954/#review267244

> interesting, is the new code doing this? Doesn't look like... We should check if this is breaking some ui.

Good catch, I've added this functionality in the second patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 12

9 months ago
mozreview-review
Comment on attachment 8995546 [details]
Bug 1468980 - Add an annotations parameter to PlacesUtils.history.update to retrieve any annotations.

https://reviewboard.mozilla.org/r/259950/#review267494
Attachment #8995546 - Flags: review?(mak77) → review+
(Reporter)

Comment 13

9 months ago
mozreview-review
Comment on attachment 8995547 [details]
Bug 1468980 - Change downloads code to save downloads via the new async history API.

https://reviewboard.mozilla.org/r/259952/#review267496
Attachment #8995547 - Flags: review?(mak77) → review+

Comment 14

9 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c7eadf5e1ce
Add an annotations parameter to PlacesUtils.history.update to retrieve any annotations. r=mak
https://hg.mozilla.org/integration/autoland/rev/e48757a556f8
Change downloads code to save downloads via the new async history API. r=mak
https://hg.mozilla.org/integration/autoland/rev/bc2f6a85b93f
Remove nsIDownloadHistory.idl. r=mak

Comment 15

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3c7eadf5e1ce
https://hg.mozilla.org/mozilla-central/rev/e48757a556f8
https://hg.mozilla.org/mozilla-central/rev/bc2f6a85b93f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1490848
You need to log in before you can comment on or make changes to this bug.