Closed Bug 732027 Opened 8 years ago Closed 8 years ago

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

Categories

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

Tracking

(seamonkey2.10 verified, seamonkey2.11 fixed, seamonkey2.12 fixed)

VERIFIED FIXED
seamonkey2.12
Tracking Status
seamonkey2.10 --- verified
seamonkey2.11 --- fixed
seamonkey2.12 --- fixed

People

(Reporter: sgautherie, Assigned: mcsmurf)

References

()

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
Attached patch First attempt at patch (obsolete) — Splinter Review
Patch is not complete yet
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #602198 - Attachment is obsolete: true
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.
Serge: Do you know who could review this patch?
(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".
Target Milestone: seamonkey2.10 → seamonkey2.11
Attachment #612534 - Flags: approval-comm-aurora?
Blocks: 742706
Attachment #612534 - Flags: review?(neil)
Attachment #612534 - Flags: review?(neil) → review+
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
Attachment #612534 - Attachment description: (AAv1) Replace internal usage of old transactions shim, SeaMonkey tests → (AAv1) Replace internal usage of old transactions shim, SeaMonkey tests [Checked in: Comment 7]
Attachment #612534 - Flags: review?(iann_bugzilla)
Depends on: 732404
(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.
Attachment #614366 - Flags: review?(neil)
Attachment #614366 - Flags: approval-comm-aurora?
Frank, what about your patch?
Attachment #614366 - Flags: review?(neil) → review+
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
Attachment #612534 - Flags: approval-comm-aurora? → approval-comm-aurora+
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 :-)
Attachment #614366 - Flags: approval-comm-aurora? → approval-comm-aurora+
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
Attachment #614366 - Attachment description: (ABv1) Port |Bug 732404 - Remove dead handlers from editBookmarkOverlay.xul| to SeaMonkey → (ABv1) Port |Bug 732404 - Remove dead handlers from editBookmarkOverlay.xul| to SeaMonkey [Checked in: Comment 12]
Keywords: checkin-needed
Whiteboard: [c-n: 9be92ae9a206 and 043d160cdd96 to c-a]
Attachment #612534 - Attachment description: (AAv1) Replace internal usage of old transactions shim, SeaMonkey tests [Checked in: Comment 7] → (AAv1) Replace internal usage of old transactions shim, SeaMonkey tests [Checked in: Comment 7 & 13]
Attachment #614366 - Attachment description: (ABv1) Port |Bug 732404 - Remove dead handlers from editBookmarkOverlay.xul| to SeaMonkey [Checked in: Comment 12] → (ABv1) Port |Bug 732404 - Remove dead handlers from editBookmarkOverlay.xul| to SeaMonkey [Checked in: Comment 12 & 13]
Neil: This patch is not high priority, so take your time :)
Attachment #602395 - Attachment is obsolete: true
Attachment #621339 - Flags: review?(neil)
Flags: in-testsuite+
Target Milestone: seamonkey2.11 → seamonkey2.12
Comment on attachment 621339 [details] [diff] [review]
Replace all uses of old transactions shim
[Checked in: See comment 16 & 30 & 35]

Seems to work.
Attachment #621339 - Flags: review?(neil) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 755758
V.Fixed, per MXR search.
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.
Attachment #621339 - Flags: approval-comm-beta?
Attachment #621339 - Flags: approval-comm-aurora?
(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.]
No longer blocks: 742706
No longer depends on: 755758, 575955, 730849, 732404
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(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 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.]
Attachment #625435 - Flags: review?(jh) → review+
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.
Addition: With "this patch" I mean "my patch"
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 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.
(In reply to neil@parkwaycc.co.uk 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.]
Attachment #621339 - Attachment description: Replace all uses of old transactions shim → Replace all uses of old transactions shim [Checked in: See comment 16]
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.
Attachment #625435 - Attachment description: Ci. → Fix "Ci." bustage [Checked in: See comment 26]
Attachment #625435 - Flags: approval-comm-beta?
Attachment #625435 - Flags: approval-comm-aurora?
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Attachment #621339 - Flags: approval-comm-beta?
Attachment #621339 - Flags: approval-comm-beta+
Attachment #621339 - Flags: approval-comm-aurora?
Attachment #621339 - Flags: approval-comm-aurora+
Attachment #625435 - Flags: approval-comm-beta?
Attachment #625435 - Flags: approval-comm-beta+
Attachment #625435 - Flags: approval-comm-aurora?
Attachment #625435 - Flags: approval-comm-aurora+
Keywords: checkin-needed
Whiteboard: [c-n: fb6446c0d84f + c8fff6ccf460 + 4cf3ad339257 to c-a and c-b]
Whiteboard: [c-n: fb6446c0d84f + c8fff6ccf460 + 4cf3ad339257 to c-a and c-b] → [c-n: fb6446c0d84f + c8fff6ccf460 + 4cf3ad339257 to c-a]
V.Fixed, as it was before the bustage fix.

***

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

seamonkey2.10: verified.
Status: RESOLVED → VERIFIED
Attachment #625435 - Attachment description: Fix "Ci." bustage [Checked in: See comment 26] → Fix "Ci." bustage [Checked in: See comment 26 & 29]
Attachment #621339 - Attachment description: Replace all uses of old transactions shim [Checked in: See comment 16] → Replace all uses of old transactions shim [Checked in: See comment 16 & 30]
(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 :-|
> 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.
(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...
Depends on: 754405
Attachment #621339 - Attachment description: Replace all uses of old transactions shim [Checked in: See comment 16 & 30] → Replace all uses of old transactions shim [Checked in: See comment 16 & 30 & 35]
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 :-|
Attachment #625435 - Attachment description: Fix "Ci." bustage [Checked in: See comment 26 & 29] → Fix "Ci." bustage [Checked in: See comment 26 & 29 & 35]
You need to log in before you can comment on or make changes to this bug.