Last Comment Bug 739213 - Kill AddPageWithDetails
: Kill AddPageWithDetails
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Paolo Amadini
:
Mentors:
Depends on: 710259 710895 743679 748562
Blocks: 739219 700250
  Show dependency treegraph
 
Reported: 2012-03-26 07:03 PDT by Marco Bonardo [::mak] (Away 6-20 Aug)
Modified: 2012-05-15 08:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First patch (52.32 KB, patch)
2012-04-09 08:29 PDT, :Paolo Amadini
mak77: review+
Details | Diff | Splinter Review
Updated patch, excluding Sync parts (56.44 KB, patch)
2012-04-24 15:51 PDT, :Paolo Amadini
mak77: review+
Details | Diff | Splinter Review
Remove the function (58.30 KB, patch)
2012-04-27 06:32 PDT, :Paolo Amadini
gavin.sharp: superreview+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-26 07:03:26 PDT
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.
Comment 1 :Paolo Amadini 2012-04-09 08:29:17 PDT
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.
Comment 2 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-12 07:16:23 PDT
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
Comment 3 :Paolo Amadini 2012-04-24 15:51:37 PDT
Created attachment 618083 [details] [diff] [review]
Updated patch, excluding Sync parts
Comment 4 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-26 09:19:33 PDT
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
Comment 5 :Paolo Amadini 2012-04-27 06:32:44 PDT
Created attachment 619010 [details] [diff] [review]
Remove the function

Removing addPageWithDetails entirely as per comment 4.
Comment 6 :Paolo Amadini 2012-04-27 06:36:37 PDT
(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.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-27 08:06:30 PDT
(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.
Comment 8 :Paolo Amadini 2012-04-27 08:58:52 PDT
(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.
Comment 9 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-27 09:01:42 PDT
well probably not the same reelase cycle, addVisit is trickier to remove, but I agree we should only suggest updatePlaces.
Comment 10 :Paolo Amadini 2012-05-14 11:46:38 PDT
Unbitrotted and pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b199744d601
Comment 11 Ed Morley [:emorley] 2012-05-15 06:52:01 PDT
https://hg.mozilla.org/mozilla-central/rev/4b199744d601
Comment 12 Eric Shepherd [:sheppy] 2012-05-15 08:09:08 PDT
Docs updated:

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

Mentioned on Firefox 15 for developers.

Note You need to log in before you can comment on or make changes to this bug.