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

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: mak, Assigned: tetsuharu)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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)
I don't have enough time to do this now
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Posted patch Replace PlacesUIUtils.ptm (obsolete) — Splinter Review
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?
Attachment #598336 - Flags: review?(mak77)
Assignee: nobody → saneyuki.s.snyk
(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 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
Attachment #598336 - Flags: review?(mak77)
(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.
Posted patch Replace PlacesUIUtils.ptm v2 (obsolete) — Splinter Review
*Fix wrong points.
*Add parts of tests.
Attachment #598336 - Attachment is obsolete: true
Attachment #599097 - Flags: review?(mak77)
Sorry, I didn't add auther & email address.
This patch fixes:
*Fix wrong points.
*Add parts of tests.
Attachment #599097 - Attachment is obsolete: true
Attachment #599099 - Flags: review?(mak77)
Attachment #599097 - Flags: review?(mak77)
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
Attachment #599099 - Flags: review?(mak77)
Posted patch Replace PlacesUIUtils.ptm v3 (obsolete) — Splinter Review
*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/)
Attachment #599099 - Attachment is obsolete: true
Attachment #599138 - Flags: review?(mak77)
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!
Attachment #599138 - Flags: review?(mak77) → review+
this may have bitrotted on central, please check.
Rebased Patch.
Attachment #599138 - Attachment is obsolete: true
Attachment #600666 - Flags: review?(mak77)
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.
Attachment #600666 - Flags: review?(mak77)
Posted patch Replace PlacesUIUtils.ptm v4 (obsolete) — Splinter Review
*Remove PlacesUIUtils from test_placesTxn.js.
(add some constant)
Attachment #600666 - Attachment is obsolete: true
Blocks: 730668
Status: NEW → ASSIGNED
Posted patch Replace PlacesUIUtils.ptm v5 (obsolete) — Splinter Review
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.
Attachment #600742 - Attachment is obsolete: true
Attachment #600863 - Flags: review?(mak77)
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
Attachment #600863 - Flags: review?(mak77) → review+
Fix lacking declaration.
Attachment #600863 - Attachment is obsolete: true
Attachment #601273 - Flags: review?(mak77)
Attachment #601273 - Flags: review?(mak77) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/322e727576c8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
No longer blocks: 732027
You need to log in before you can comment on or make changes to this bug.