575955 - Replace internal usage of old transactions shim, add a new toolkit test

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Replace all internal references to PlacesUIUtils.ptm but those in test_placesTXN.js rename this test to point out it is testing the shim, add a new toolkit test (cleaned up copy of test_placesTxn.js)

Comment 1

[Marco Bonardo [:mak]]

I don't have enough time to do this now

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm

Replace internal references to PlacesUIUtils.ptm on this patch.
I used "var" statement as possible if it is used in a function.

And I don't make patches for test.
I have some questions:
*Rename test_placesTXN.js & clean up it? (replace references to PlacesUIUtils.ptm in it?)
*No change test_txnGUIDs.js?
*Change /browser/components/places/tests/browser/browser_457473_no_copy_guid.js & /browser/components/places/tests/browser/browser_425884.js
 (Replace internal references to PlacesUIUtils.ptm in them?)

What should I do?

Comment 3

[Marco Bonardo [:mak]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #2)
> *Rename test_placesTXN.js & clean up it? (replace references to
> PlacesUIUtils.ptm in it?)

The best thing to do would be to completely rewrite it, since it is a bit too much serialized (should be split into subtests). Though this is a lot of work to make it proper, since we don't want to lose some of the coverage from it. So as a first step may even be fine to move the test to toolkit/components/places/tests/unit/ and just replace the calls. Further cleanup and separation may be done in a separate bug.

> *No change test_txnGUIDs.js?

same as for test_placesTxn, even if this test will disappear soon, but it's ok to move it and replace calls for now.

> *Change
> /browser/components/places/tests/browser/browser_457473_no_copy_guid.js &
> /browser/components/places/tests/browser/browser_425884.js
>  (Replace internal references to PlacesUIUtils.ptm in them?)

yes, any call to PlacesUIUtils.ptm found by mxr should go away.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 598336 [details] [diff] [review]
Replace PlacesUIUtils.ptm

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

I'd like to take a second look once these are addressed.

Btw I'd really want to thank you for all these patches, it's great!

::: browser/base/content/browser-places.js
@@ +331,5 @@
>        var parent = aParent != undefined ?
>                     aParent : PlacesUtils.unfiledBookmarksFolderId;
>        var descAnno = { name: PlacesUIUtils.DESCRIPTION_ANNO, value: description };
> +      var txn = new PlacesCreateBookmarkTransaction(uri, parent, -1,
> +                                                    title, null, [descAnno]);

DEFAULT_INDEX

::: browser/components/places/content/bookmarkProperties.js
@@ +614,3 @@
>  
> +    return new PlacesAggregatedTransaction(this._getDialogTitle(),
> +                                           transactions);

just pass [createTxn] without making a temp var

@@ +625,5 @@
>      for (var i = 0; i < this._URIs.length; ++i) {
>        var uri = this._URIs[i];
>        var title = this._getURITitleFromHistory(uri);
> +      var createTxn = new PlacesCreateBookmarkTransaction(uri, -1, -1, title);
> +      transactions.push(createTxn);

use DEFAULT_INDEX here as well

::: browser/components/places/content/controller.js
@@ +857,5 @@
>      NS_ASSERT(transactions instanceof Array, "Must pass a transactions array");
>      if (!removedFolders)
>        removedFolders = [];
>  
> +    var txn;

use let locally please

::: browser/components/places/content/editBookmarkOverlay.js
@@ +454,5 @@
>            tagsToAdd.push(tags[i]);
>        }
>  
> +      if (tagsToRemove.length > 0) {
> +        let untagTxn =  new PlacesUntagURITransaction(this._uri, tagsToRemove);

remove double spacing after the =

@@ +465,3 @@
>  
>        if (txns.length > 0) {
> +        var aggregate = new PlacesAggregatedTransaction("Update tags", txns);

please use let here. generally we try to use let in new or changed code, unless all the function or module is using var. this code is a bit a mixture though as soon as we touch it we try to stick to let.

@@ +534,5 @@
>        }
>  
>        if (txns.length > 0) {
> +        var aggregate = new PlacesAggregatedTransaction("Update tags",
> +                                                        txns);

ditto, use let here

@@ +540,4 @@
>  
>          this._allTags = tags;
>          this._tags = [];
>          for (i = 0; i < this._uris.length; i++)

sigh, while here could you please fix this making it "for (let i = 0"

there is another wrong "for (i=0" in the same file, bonus points if you also fix that :)

@@ +555,5 @@
>      if (this._itemId == -1)
>        return;
>  
>      var namePicker = this._element("namePicker")
> +    var txns = [], txn;

I think you could define txn at first use, don't see a reason to declare it here

@@ +572,4 @@
>      }
>  
> +    var aggregate = new PlacesAggregatedTransaction("Edit Item Title", txns);
> +    PlacesUtils.transactionManager.doTransaction(aggregate);

honestly this aggregate seems useless, to me looks like we only have one PlacesEditItemTitleTransaction transaction here, in the else if branch, so we should just doTransaction that one.

@@ +738,5 @@
>  
>      // Move the item
>      var container = this._getFolderIdFromMenuList();
>      if (PlacesUtils.bookmarks.getFolderIdForItem(this._itemId) != container) {
> +      var txn = new PlacesMoveItemTransaction(this._itemId, container, -1);

replace 1 with PlacesUtils.bookmarks.DEFAULT_INDEX

@@ +780,5 @@
>    },
>  
>    _markFolderAsRecentlyUsed:
>    function EIO__markFolderAsRecentlyUsed(aFolderId) {
> +    var txns = [], annoTxn;

as well as here you may just use let annoTxn locally instead of defining it here globally

::: browser/components/places/content/moveBookmarks.js
@@ +68,5 @@
>          continue;
>  
> +      let txn = new PlacesMoveItemTransaction(this._nodes[i].itemId,
> +                                              selectedFolderID,
> +                                              -1)

missing semicolon
also please replace -1 with PlacesUtils.bookmarks.DEFAULT_INDEX

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #3)
> (In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #2)
> > *Rename test_placesTXN.js & clean up it? (replace references to
> > PlacesUIUtils.ptm in it?)
> 
> The best thing to do would be to completely rewrite it, since it is a bit
> too much serialized (should be split into subtests). Though this is a lot of
> work to make it proper, since we don't want to lose some of the coverage
> from it. So as a first step may even be fine to move the test to
> toolkit/components/places/tests/unit/ and just replace the calls. Further
> cleanup and separation may be done in a separate bug.

I understand that splitting into subtests in test_placesTXN.js is the other bug.

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm v2

*Fix wrong points.
*Add parts of tests.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm v2.1

Sorry, I didn't add auther & email address.
This patch fixes:
*Fix wrong points.
*Add parts of tests.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 599099 [details] [diff] [review]
Replace PlacesUIUtils.ptm v2.1

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

Thank you for the update, it looks fine.

Suggestion for future, when moving files please use hg mv command, otherwise you destroy all blame information on the file contents. It also makes harder doing reviews, since changed lines are not hilighted.

When adding or removing xpcshell tests you also have to edit the xpcshell.ini file in the same folder, to add or remove the test from it. Otherwise this will fail compilation (that is what prevents the r+ here)

::: browser/components/places/content/bookmarkProperties.js
@@ +602,5 @@
>      //XXX TODO: this should be in a transaction!
>      if (this._charSet)
>        PlacesUtils.history.setCharsetForURI(this._uri, this._charSet);
>  
> +    var createTxn = new PlacesCreateBookmarkTransaction(this._uri,

nit: please use let here

@@ +626,5 @@
>        var uri = this._URIs[i];
>        var title = this._getURITitleFromHistory(uri);
> +      var createTxn = new PlacesCreateBookmarkTransaction(uri, -1, 
> +                                                          DEFAULT_INDEX,
> +                                                          title);

please don't define an additional const for a single use, even if you go over 80 chars in limited cases it's fine.

::: browser/components/places/content/controller.js
@@ +1545,4 @@
>            insertionPoint.orientation == Ci.nsITreeView.DROP_ON) {
>          let uri = NetUtil.newURI(unwrapped.uri);
>          let tagItemId = insertionPoint.itemId;
> +        let tagTxn = new PlacesTagURITransaction(uri,[tagItemId]);

missing whitespace after the comma

::: browser/components/places/content/editBookmarkOverlay.js
@@ +526,4 @@
>          }
>        }
>        if (tagsToRemove.length > 0) {
> +        for (var i = 0; i < this._uris.length; i++) {

nit: use let here

@@ +796,2 @@
>  
> +    var aggregate = new PlacesAggregatedTransaction("Update last used folders", txns);

nit: use let

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm v3

*Fix wrong points.

I tried "hg export qtip" to make pacth, so it may lost the meta data of "hg mv".
(This patch is picked from .hg/patches/)

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 599138 [details] [diff] [review]
Replace PlacesUIUtils.ptm v3

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

looks good, please just get a try run to ensure the tests are happy with the changes and it will be fine.
Thank you!

Comment 11

[Marco Bonardo [:mak]]

this may have bitrotted on central, please check.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm v3.1

Rebased Patch.

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 600666 [details] [diff] [review]
Replace PlacesUIUtils.ptm v3.1

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

Sorry, this patch fails to test for some error. I must fix them.

Comment 14

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm v4

*Remove PlacesUIUtils from test_placesTxn.js.
(add some constant)

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm v5

Change from patch v3:
*Rebase on newest mozilla-central.
*Remove PlacesUIUtils from test_placesTxn.js.
(add some constant)
*Fix to fail tests.

I confirmed that this patch passed tests.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=3094a199a238
(Thanks, Nakano-san!)
The above page has some orange but they are not related to changed parts of this patch.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank *you*, Ohzeki-san, for your work.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 600863 [details] [diff] [review]
Replace PlacesUIUtils.ptm v5

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

r=me with these addressed

Thank you for doing these cleanups, we hardly have time to go through them, even if they may have a nice code size impact (once old code is removed), so it's really appreciated.

::: browser/components/places/content/controller.js
@@ +885,5 @@
>          // must only remove the query node.
>          var tag = node.title;
>          var URIs = PlacesUtils.tagging.getURIsForTag(tag);
> +        for (var j = 0; j < URIs.length; j++) {
> +          txn = new PlacesUntagURITransaction(URIs[j], [tag]);

this one is missing declaration, fwiw you can use let, regardless we are moving toward using it wherever possible.

@@ +915,4 @@
>            // to skip nodes that are children of an already removed folder.
>            removedFolders.push(node);
>          }
> +        txn = new PlacesRemoveItemTransaction(node.itemId);

as well as this one is lacking declaration

Comment 18

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Replace PlacesUIUtils.ptm v5.1

Fix lacking declaration.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/322e727576c8

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/322e727576c8