748562 - Kill AddPageWithDetails, in Sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[:Paolo Amadini]

We're removing AddPageWithDetails in bug 739213.

Comment 1

[:Paolo Amadini]

Attachment: Replace calls to AddPageWithDetails

Extracted from the patch in bug 739213.

Comment 2

[:Paolo Amadini]

Attachment: Replace calls to AddPageWithDetails

This also removes some code that is not required anymore.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 618072 [details] [diff] [review]
Replace calls to AddPageWithDetails

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

Looks good, thanks! r+ with nits.

::: services/sync/tests/unit/test_history_store.js
@@ +91,5 @@
> +      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK
> +    }]
> +  };
> +  PlacesUtils.asyncHistory.updatePlaces(place, {
> +    handleError: function () do_throw("Unexpected error in adding visit."),

Please use full {} function style to avoid tripping up Emacs users. (I know, I know...)

Also, please name all functions:

  handleError: function handleError() {
    do_throw(...);
  },

@@ +92,5 @@
> +    }]
> +  };
> +  PlacesUtils.asyncHistory.updatePlaces(place, {
> +    handleError: function () do_throw("Unexpected error in adding visit."),
> +    handleResult: function () { },

No space between {}, name function.

@@ +93,5 @@
> +  };
> +  PlacesUtils.asyncHistory.updatePlaces(place, {
> +    handleError: function () do_throw("Unexpected error in adding visit."),
> +    handleResult: function () { },
> +    handleCompletion: function () afterAddVisit()

Just 

  handleCompletion: afterAddVisit

@@ +99,2 @@
>  
> +  function afterAddVisit() {

Perhaps 'onVisitAdded'?

::: services/sync/tests/unit/test_places_guid_downgrade.js
@@ +104,5 @@
> +  ];
> +  PlacesUtils.asyncHistory.updatePlaces(places, {
> +    handleError: function () do_throw("Unexpected error in adding visit."),
> +    handleResult: function () { },
> +    handleCompletion: function () afterAddVisit()

Same comments for these three lines.

@@ +201,5 @@
>  
>  function run_test() {
>    setPlacesDatabase("places_v10_from_v11.sqlite");
>  
>    _("Verify initial setup: v11 database is available");

Please move this comment into test_initial_state.

Comment 4

[:Paolo Amadini]

Attachment: Updated patch

Comment 5

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d030e76e9ed3

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d030e76e9ed3