732027 - Port |Bug 575955 - Replace internal usage of old transactions shim, add a new toolkit test| to SeaMonkey

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P4)

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: First attempt at patch

Patch is not complete yet

Comment 2

[Frank Wein [:mcsmurf]]

Attachment: Replace all uses of old transactions shim (excluding tests for now)

Comment 3

[Frank Wein [:mcsmurf]]

Comment on attachment 602395 [details] [diff] [review]
Replace all uses of old transactions shim (excluding tests for now)

I removed onFeedLocationFieldBlur and onSiteLocationFieldBlur because those changes are no longer supported. Also see Bug 732404, we would need to port that fix then to our GUI, too.

Comment 4

[Frank Wein [:mcsmurf]]

Serge: Do you know who could review this patch?

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to Frank Wein [:mcsmurf] from comment #4)
> Serge: Do you know who could review this patch?

According to brand new
http://www.seamonkey-project.org/dev/project-areas
that should be "IanN, Mnyromyr" or "Neil".

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (AAv1) Replace internal usage of old transactions shim, SeaMonkey tests [Checked in: Comment 7 & 13]

Trivial patch copy.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 612534 [details] [diff] [review]
(AAv1) Replace internal usage of old transactions shim, SeaMonkey tests
[Checked in: Comment 7 & 13]

http://hg.mozilla.org/comm-central/rev/9be92ae9a206

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (ABv1) Port |Bug 732404 - Remove dead handlers from editBookmarkOverlay.xul| to SeaMonkey [Checked in: Comment 12 & 13]

(In reply to Frank Wein [:mcsmurf] from comment #3)
> see Bug 732404, we would need to port that fix then to our GUI, too.

http://mxr.mozilla.org/comm-central/search?string=LocationFieldBlur&case=on


[Approval Request Comment]
Risk to taking this patch (and alternatives if risky):
No risk, "dead" (Toolkit) code removal.

Comment 9

[Serge Gautherie (:sgautherie)]

Frank, what about your patch?

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 612534 [details] [diff] [review]
(AAv1) Replace internal usage of old transactions shim, SeaMonkey tests
[Checked in: Comment 7 & 13]

Test Only a=me

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 614366 [details] [diff] [review]
(ABv1) Port |Bug 732404 - Remove dead handlers from editBookmarkOverlay.xul| to SeaMonkey
[Checked in: Comment 12 & 13]

a=me per neil over IRC :-)

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 614366 [details] [diff] [review]
(ABv1) Port |Bug 732404 - Remove dead handlers from editBookmarkOverlay.xul| to SeaMonkey
[Checked in: Comment 12 & 13]

http://hg.mozilla.org/comm-central/rev/043d160cdd96

Comment 13

[Justin Wood (:Callek)]

http://hg.mozilla.org/releases/comm-aurora/rev/304952ef9937
http://hg.mozilla.org/releases/comm-aurora/rev/614748f71131

Is this fixed now?

Comment 14

[Frank Wein [:mcsmurf]]

Attachment: Replace all uses of old transactions shim [Checked in: See comment 16 & 30 & 35]

Neil: This patch is not high priority, so take your time :)

Comment 15

[[e-mail]]

Comment on attachment 621339 [details] [diff] [review]
Replace all uses of old transactions shim
[Checked in: See comment 16 & 30 & 35]

Seems to work.

Comment 16

[Frank Wein [:mcsmurf]]

Checked in: https://hg.mozilla.org/comm-central/rev/fb6446c0d84f

I found a bug on check-in, fixed this in a seperate check-in: https://hg.mozilla.org/comm-central/rev/c8fff6ccf460 I declared (and initialized) one variable twice, I removed the wrong variable

Comment 17

[Serge Gautherie (:sgautherie)]

V.Fixed, per MXR search.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 621339 [details] [diff] [review]
Replace all uses of old transactions shim
[Checked in: See comment 16 & 30 & 35]

[Approval Request Comment]
Low risk, has tests.

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> Comment on attachment 621339 [details] [diff] [review]
> Replace all uses of old transactions shim
> 
> [Approval Request Comment]
> Low risk, has tests.

Please hold off on these. AFAICS part of this code change actually regressed something and we certainly don't want to regress branches.

STR:
1. Visit a page that has a favicon.
2. Open File Bookmark dialog (Ctrl+D).

Result:

Error: ReferenceError: Ci is not defined
Source File: chrome://communicator/content/bookmarks/bm-props.js
Line: 595

Please fix. [Actually I wonder how this could slip through. We don't use those shortcuts in SM code.]

[I think for any future changes to bookmarks-related code we (the patch author and/or the reviewer) MUST test AT LEAST the Bookmarks Manager and File Bookmark dialog. As we can see here, automatic tests cannot be trusted.]

Comment 20

[[e-mail]]

(In reply to Jens Hatlak from comment #19)
> Actually I wonder how this could slip through.
Oops :-[

I probably only tested creating a bookmark from the manager, which doesn't use any of the affected code paths. Sorry about that.

Comment 21

[[e-mail]]

Attachment: Fix "Ci." bustage [Checked in: See comment 26 & 29 & 35]

Like this?

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 625435 [details] [diff] [review]
Fix "Ci." bustage
[Checked in: See comment 26 & 29 & 35]

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

This fixes the regression, thanks. If the other patch is supposed to land on branches, this one (final checkin) must go to the same package.

::: suite/common/bookmarks/bm-props.js
@@ +588,5 @@
>     * various fields and opening arguments of the dialog.
>     */
>    _getCreateNewBookmarkTransaction:
>    function BPP__getCreateNewBookmarkTransaction(aContainer, aIndex) {
> +    var nsIAnnotationService = Components.interfaces.nsIAnnotationService;

(likewise elsewhere)

Any reason not to use "const" instead of "var"? I don't see how this value could ever change.

[I'll leave it to you to decide whether to have it on function level or file-global. The latter possibly needs checking for redeclaration.]

Comment 23

[Frank Wein [:mcsmurf]]

Jens: I tested this patch and filing a bookmark with the File Bookmark dialog worked. But a few hours ago I also found this issue. Not sure why this issue (Ci not working) is sometimes happening and sometimes not.

Comment 24

[Frank Wein [:mcsmurf]]

Addition: With "this patch" I mean "my patch"

Comment 25

[Frank Wein [:mcsmurf]]

Actually I see now what code path is needed to reproduce this. Maybe we should just Ci like any other Mozilla app to avoid such issues (actually I suggested this already a few years ago).

Comment 26

[[e-mail]]

Comment on attachment 625435 [details] [diff] [review]
Fix "Ci." bustage
[Checked in: See comment 26 & 29 & 35]

Pushed changeset 4cf3ad339257 with const nsIAnnotationService to comm-central.

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #26)
> Pushed changeset 4cf3ad339257 with const nsIAnnotationService to
> comm-central.

[Sorry for not catching the two EIO__getLastUsedAnnotationObject occurrences during review; we could have done these here in one go. r=me for a follow-up if you like.]

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 625435 [details] [diff] [review]
Fix "Ci." bustage
[Checked in: See comment 26 & 29 & 35]

[Approval Request Comment]
To go along Frank's patch.

Comment 29

[Philip Chee]

Pushed:
http://hg.mozilla.org/releases/comm-beta/rev/132d65201b8d

Comment 30

[Philip Chee]

Pushed:
http://hg.mozilla.org/releases/comm-beta/rev/2f1ac5a53126
http://hg.mozilla.org/releases/comm-beta/rev/ff26dd4c1b8f

Comment 31

[Serge Gautherie (:sgautherie)]

V.Fixed, as it was before the bustage fix.

***

http://tbpl.drapostles.org/?tree=SeaMonkey-Beta&rev=cd00c74eef12

seamonkey2.10: verified.

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #29)
> http://hg.mozilla.org/releases/comm-beta/rev/132d65201b8d

It looks like you pushed the patch rather than the changeset :-/
Yet that will do as is :-|

Comment 33

[Philip Chee]

> It looks like you pushed the patch rather than the changeset :-/
> Yet that will do as is :-|
Sorry I have no idea what you mean. I didn't push anything.

Comment 34

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #33)
> > It looks like you pushed the patch rather than the changeset :-/
> > Yet that will do as is :-|
> Sorry I have no idea what you mean. I didn't push anything.

Right: "you" was Callek in this case...

Comment 35

[Philip Chee]

Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/998f1a8f9f20
http://hg.mozilla.org/releases/comm-aurora/rev/15a59c54156b
http://hg.mozilla.org/releases/comm-aurora/rev/b4bc87ee306f

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 625435 [details] [diff] [review]
Fix "Ci." bustage
[Checked in: See comment 26 & 29 & 35]

NB: Pushed to Alpha as it was pushed to Beta :-|