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

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Places
RESOLVED FIXED
8 years ago
6 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)

(Reporter)

Description

8 years ago
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)
(Reporter)

Comment 1

8 years ago
I don't have enough time to do this now
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Created attachment 598336 [details] [diff] [review]
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?
Attachment #598336 - Flags: review?(mak77)

Updated

6 years ago
Assignee: nobody → saneyuki.s.snyk
(Reporter)

Comment 3

6 years ago
(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.
(Reporter)

Comment 4

6 years ago
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.
Created attachment 599097 [details] [diff] [review]
Replace PlacesUIUtils.ptm v2

*Fix wrong points.
*Add parts of tests.
Attachment #598336 - Attachment is obsolete: true
Attachment #599097 - Flags: review?(mak77)
Created attachment 599099 [details] [diff] [review]
Replace PlacesUIUtils.ptm v2.1

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)
(Reporter)

Comment 8

6 years ago
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)
Created attachment 599138 [details] [diff] [review]
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/)
Attachment #599099 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #599138 - Flags: review?(mak77)
(Reporter)

Comment 10

6 years ago
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+
(Reporter)

Comment 11

6 years ago
this may have bitrotted on central, please check.
Created attachment 600666 [details] [diff] [review]
Replace PlacesUIUtils.ptm v3.1

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)
Created attachment 600742 [details] [diff] [review]
Replace PlacesUIUtils.ptm v4

*Remove PlacesUIUtils from test_placesTxn.js.
(add some constant)
Attachment #600666 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 730668
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Created attachment 600863 [details] [diff] [review]
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.
Attachment #600742 - Attachment is obsolete: true
Attachment #600863 - Flags: review?(mak77)
Thank *you*, Ohzeki-san, for your work.
(Reporter)

Comment 17

6 years ago
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+
Created attachment 601273 [details] [diff] [review]
Replace PlacesUIUtils.ptm v5.1

Fix lacking declaration.
Attachment #600863 - Attachment is obsolete: true
Attachment #601273 - Flags: review?(mak77)
(Reporter)

Updated

6 years ago
Attachment #601273 - Flags: review?(mak77) → review+

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/322e727576c8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.