Kill AddPageWithDetails

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: Paolo)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete})

Trunk
mozilla15
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
This is used in tests, IE migrator and Safari migrator.
Can be replaced with PlacesUtils.asyncHistory.updatePlaces, the migrators rewrite is ongoing, so what's left to do here is updating tests and removing the API.
(Reporter)

Updated

5 years ago
Blocks: 700250
(Reporter)

Updated

5 years ago
Blocks: 739219
(Assignee)

Updated

5 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 743679
(Assignee)

Comment 1

5 years ago
Created attachment 613299 [details] [diff] [review]
First patch

This replaces addPageWithDetails with updatePlaces in cases where the call is
not nested in a way that requires too much changes in the surrounding code or
test framework.

In other cases, the call to addPageWithDetails is temporarily replaced with
the synchronous addVisit, sometimes followed by setPageTitle.

addPageWithDetails is also marked deprecated in preparation for its removal.
Attachment #613299 - Flags: review?(mak77)
(Reporter)

Comment 2

5 years ago
Comment on attachment 613299 [details] [diff] [review]
First patch

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

nothing major (Apart the suckiness of some of the tests, but those are quite old), though in many cases you don't need to pass an array to updatePlaces, you need a SR, and the Sync part should be also reviewed by a Sync peer.
A try run with a couple retriggers may help confirming this doesn't introduce some subtle random failure.

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +296,5 @@
> +  addPlace(makeURI("http://1hour10minutes.com/"), "1 hour 10 minutes ago", now_uSec - 70*60*1000000);
> +  addPlace(makeURI("http://2hour.com/"), "Less than 2 hours ago", now_uSec - 90*60*1000000);
> +  addPlace(makeURI("http://2hour10minutes.com/"), "2 hours 10 minutes ago", now_uSec - 130*60*1000000);
> +  addPlace(makeURI("http://4hour.com/"), "Less than 4 hours ago", now_uSec - 180*60*1000000);
> +  addPlace(makeURI("http://4hour10minutes.com/"), "4 hours 10 minutesago", now_uSec - 250*60*1000000);

assign 60 * 1000000 to a const kUsecPerMin and use the const, should help readability

@@ +302,5 @@
>    let today = new Date();
>    today.setHours(0);
>    today.setMinutes(0);
>    today.setSeconds(1);
> +  addPlace(makeURI("http://today.com/"), "Today", today.valueOf() * 1000);

is not .valueOf() implicit when you multiply?

@@ +307,4 @@
>    
>    let lastYear = new Date();
>    lastYear.setFullYear(lastYear.getFullYear() - 1);
> +  addPlace(makeURI("http://before-today.com/"), "Before Today", lastYear.valueOf() * 1000);

ditto

@@ +311,5 @@
>    
> +  PlacesUtils.asyncHistory.updatePlaces(places, {
> +    handleError: function () ok(false, "Unexpected error in adding visit."),
> +    handleResult: function () { },
> +    handleCompletion: function () {

actually, for sanity may be nice to check that handleResult has been called at least once.

@@ +312,5 @@
> +  PlacesUtils.asyncHistory.updatePlaces(places, {
> +    handleError: function () ok(false, "Unexpected error in adding visit."),
> +    handleResult: function () { },
> +    handleCompletion: function () {
> +      confirmSetupHistory();

Otherwise, if we properly count the number of handleResult calls, we may even kill this additional check.

::: browser/base/content/test/newtab/browser_newtab_bug722273.js
@@ +43,5 @@
> +  }
> +  PlacesUtils.asyncHistory.updatePlaces(places, {
> +    handleError: function () do_throw("Unexpected error in adding visit."),
> +    handleResult: function () { },
> +    handleCompletion: function () TestRunner.next()

put 59 in a const, and check handleResult is called that many times.

::: browser/components/privatebrowsing/test/unit/do_test_placesTitleNoUpdate.js
@@ +58,5 @@
> +        visits: [{
> +          visitDate: Date.now() * 1000,
> +          transitionType: Ci.nsINavHistoryService.TRANSITION_LINK
> +        }]
> +      }],

just for readability, make a separate places obj (doesn't need to be an array, fwiw, if it's just a single place)

@@ +75,1 @@
>      do_check_eq(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_1);

may we rather check this in handleResult based on the place we get back?

@@ +83,5 @@
> +        visits: [{
> +          visitDate: Date.now() * 2000,
> +          transitionType: Ci.nsINavHistoryService.TRANSITION_LINK
> +        }]
> +      }],

ditto

@@ +103,1 @@
>      do_check_eq(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_1);

as well as this one.

::: browser/components/privatebrowsing/test/unit/do_test_removeDataFromDomain.js
@@ +90,5 @@
>    check_visited(aURI, false);
> +  let history = Cc["@mozilla.org/browser/nav-history-service;1"]
> +                .getService(Ci.nsINavHistoryService);
> +  history.addVisit(aURI, Date.now() * 1000, null,
> +                   Ci.nsINavHistoryService.TRANSITION_LINK, false, 0);

use PlacesUtils.history (if not available already just import it)

::: services/sync/tests/unit/test_history_store.js
@@ +85,2 @@
>  
> +  PlacesUtils.asyncHistory.updatePlaces(

for the changes to Sync tests you want an additional review from rnewman... I suggest splitting these to a separate patch.

@@ +90,5 @@
> +      visits: [{
> +        visitDate: TIMESTAMP1,
> +        transitionType: Ci.nsINavHistoryService.TRANSITION_LINK
> +      }]
> +    }],

doesn't need to be an array

::: services/sync/tests/unit/test_places_guid_downgrade.js
@@ +101,5 @@
> +          visitDate: Date.now() * 1000,
> +          transitionType: Ci.nsINavHistoryService.TRANSITION_LINK
> +        }]
> +      }
> +    ],

nit: could compress a bit the indentation and define a separate var for readability

@@ +116,5 @@
> +    let fxid = PlacesUtils.bookmarks.insertBookmark(
> +      PlacesUtils.bookmarks.toolbarFolder,
> +      uri,
> +      PlacesUtils.bookmarks.DEFAULT_INDEX,
> +      "Mozilla");

not sure what this Hack is meant to do, if (as I suppose) it was meant to do a Flush when we had temp tables to read the guid from the disk table, it can likely die.  Richard may know more.

::: toolkit/components/places/nsIBrowserHistory.idl
@@ +60,2 @@
>       */
> +    [deprecated]

needs SR

::: toolkit/components/places/tests/bookmarks/test_savedsearches.js
@@ +70,5 @@
>  
> +// a search term that matches a default bookmark
> +var searchTerm = "about";
> +
> +var testRoot;

man, this test sucks...

::: toolkit/components/places/tests/queries/head_queries.js
@@ +112,5 @@
> +            PlacesUtils.history.addVisit(
> +                                   uri(qdata.uri),
> +                                   qdata.lastVisit,
> +                                   null,
> +                                   Ci.nsINavHistoryService.TRANSITION_LINK,

I don't mind if you go a bit over the 80 chars, provided you don't use fancy indentation :)
Btw notice that head_common.js has shortcuts for transitions, so you can just use TRANSITION_LINK in any Places xpcshell test.

::: toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js
@@ +69,5 @@
> +    var query = PlacesUtils.history.getNewQuery();
> +    // if we don't set a tag folder, RESULTS_AS_TAG_CONTENTS will return all
> +    // tagged URIs
> +    var result = PlacesUtils.history.executeQuery(query, options);
> +    var root = result.root;

coalesce to root = PlacesUtils.history.executeQuery(query, options).root; since you don't need result.

btw, since your practically rewriting this test, please convert it to use let
Attachment #613299 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Depends on: 748562
(Assignee)

Comment 3

5 years ago
Created attachment 618083 [details] [diff] [review]
Updated patch, excluding Sync parts
Attachment #613299 - Attachment is obsolete: true
Attachment #618083 - Flags: feedback?(mak77)
(Reporter)

Comment 4

5 years ago
Comment on attachment 618083 [details] [diff] [review]
Updated patch, excluding Sync parts

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

Will need SR once comments are addressed.

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +19,5 @@
> +  setupFormHistory();
> +  setupHistory(afterSetupHistory);
> +}
> +
> +function afterSetupHistory() {

nit: onHistoryReady may be clearer, globally using onXReady than afterXDone would be better.

::: browser/base/content/test/newtab/browser_newtab_bug722273.js
@@ +32,5 @@
>    let uri = makeURI(URL);
> +  let places = [];
> +  for (let i = 59; i > 0; i--) {
> +    places.push({
> +      uri: uri,

if the uri is the same, you should rather add a single place object and generate its visits in a loop, that's how updatePlaces is supposed to be used (to avoid adding multiple times the same page).

::: browser/components/privatebrowsing/test/unit/do_test_removeDataFromDomain.js
@@ +362,5 @@
>    pb.removeDataFromDomain("mozilla.org");
>    check_visited(TEST_URI, true);
>  
>    // Clear history since we left something there from this test.
> +  PlacesUtils.bhistory.removeAllPages();

I hope no test after this one uses history, cause would likely generate a random failure...

::: toolkit/components/places/nsIBrowserHistory.idl
@@ +62,3 @@
>      void addPageWithDetails(in nsIURI aURI,
>                              in wstring aTitle,
>                              in long long aLastVisited);

This actually hurts us from a Snappy point of view (blocking other snappy work), so I'd prefer if we'd directly remove it, please send a mail to Jorge to notify add-ons using it about the removal and the alternative.

I can blog post about the removal of AddPageWithDetails and AddVisit too, the latter may take more time but it's ok to start with a heads-up.

::: toolkit/components/places/tests/bookmarks/test_savedsearches.js
@@ +68,5 @@
>  // get bookmarks root id
>  var root = bmsvc.bookmarksMenuFolder;
>  
> +// a search term that matches a default bookmark
> +var searchTerm = "about";

looks like a const
Attachment #618083 - Flags: feedback?(mak77) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 619010 [details] [diff] [review]
Remove the function

Removing addPageWithDetails entirely as per comment 4.
Attachment #618083 - Attachment is obsolete: true
Attachment #619010 - Flags: superreview?(gavin.sharp)
(Assignee)

Comment 6

5 years ago
(In reply to Marco Bonardo [:mak] from comment #4)
> This actually hurts us from a Snappy point of view (blocking other snappy
> work), so I'd prefer if we'd directly remove it, please send a mail to Jorge
> to notify add-ons using it about the removal and the alternative.

Jorge, can you take the appropriate actions, if needed, to notify add-on
developers that the addPageWithDetails API will be removed in Firefox 15?

The suggested alternative is the updatePlaces asyncronous API, though for the
moment there is a temporary alternative using addVisit and setPageTitle, if making
the calling code asynchronous is difficult.
(In reply to Paolo Amadini [:paolo] from comment #6)
> though for the moment there is a temporary alternative using addVisit and
> setPageTitle, if making the calling code asynchronous is difficult.

How long do we expect that solution to be present? Seems dangerous to even mention it as an alternative if we're actively trying to get rid of those synchronous APIs.
Attachment #619010 - Flags: superreview?(gavin.sharp) → superreview+
(Assignee)

Comment 8

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> (In reply to Paolo Amadini [:paolo] from comment #6)
> > though for the moment there is a temporary alternative using addVisit and
> > setPageTitle, if making the calling code asynchronous is difficult.
> 
> How long do we expect that solution to be present? Seems dangerous to even
> mention it as an alternative if we're actively trying to get rid of those
> synchronous APIs.

Right, I didn't think that, even if it requires some time, it is probably going
away in the same release cycle, so no need to mention it.
(Reporter)

Comment 9

5 years ago
well probably not the same reelase cycle, addVisit is trickier to remove, but I agree we should only suggest updatePlaces.
Keywords: addon-compat
(Assignee)

Comment 10

5 years ago
Unbitrotted and pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b199744d601
Keywords: dev-doc-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/4b199744d601
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Docs updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIBrowserHistory

Mentioned on Firefox 15 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.