627416 - Port Firefox changes from Jan 20 places merge

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Robert Kaiser]

Today's places repo merge added a few smaller changes on the Firefox side that we'll want to port the browser/ parts of:
http://hg.mozilla.org/mozilla-central/rev/1c1216dcafed (bug 621274)
http://hg.mozilla.org/mozilla-central/rev/9c6b3e840bf2 (bug 620198)
http://hg.mozilla.org/mozilla-central/rev/61546ddb725f (bug 394353)

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Bug 620198 ported in bug 627408 which I just checked in. Two to go (one file each).

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: bug 394353 port [Checkin: comment 6]

Actually bug 394353 is a funny one: AFAICS you cannot test anything that this fixes in Firefox (Minefield) because it only fixes the updating of the tags field (not list) in the dialog for editing a single bookmark, and that dialog is closed automatically when it loses the focus, so you cannot for example switch to the Library and add a tag to that bookmark. You'd probably have to have an add-on (or maybe Sync) modifying the bookmark in the background somehow. The test they added uses the Places API to add a tag while the dialog is open.

All other (more interesting!) cases that would be observable, like updating of the tags list according to the tags field upon leaving the latter, have not been fixed. :-(

Fortunately for us (from a testing POV), our "New Bookmark" dialog (Bookmarks/File Bookmark) is still a normal one, so you can switch to the Bookmarks Manager and add a tag to the bookmark you're currently editing (which is already created when the dialog opens and deleted if the dialog is left using Cancel). Without the patch, the tags field on the dialog is not updated.

Comment 3

[[e-mail]]

(In reply to comment #2)
> Actually bug 394353 is a funny one: AFAICS you cannot test anything that this
> fixes in Firefox (Minefield) because it only fixes the updating of the tags
> field (not list) in the dialog for editing a single bookmark

I seem to have found another way to trigger the bug: simply drag a bookmark to a tag folder. Without the patch, the tag does not appear in the panel.

Comment 4

[[e-mail]]

Comment on attachment 505956 [details] [diff] [review]
bug 394353 port [Checkin: comment 6]

I asked mak and he may have overlooked the fact that editing a tag generates a item changed notification which may provide a superior solution, in which case I'll expect a separate bug to port his patch in due course.

Comment 5

[[e-mail]]

(In reply to comment #2)
> Fortunately for us (from a testing POV), our "New Bookmark" dialog
> (Bookmarks/File Bookmark) is still a normal one, so you can switch to the
> Bookmarks Manager and add a tag to the bookmark you're currently editing (which
> is already created when the dialog opens and deleted if the dialog is left
> using Cancel). Without the patch, the tags field on the dialog is not updated.

Side note: leaving a bookmark dialog open is not a good idea. All changes to bookmarks are lost if you cancel it.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 505956 [details] [diff] [review]
bug 394353 port [Checkin: comment 6]

http://hg.mozilla.org/comm-central/rev/2e8f93800bce

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

One to go (bug 621274, FeedWriter.js), so leaving bug open.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Port bug 621274 [Checked in: See comment 11]

Untested. (Sorry.)

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Just copy bug 597995 [Checked in: Comment 14]

Simply make it identical to FF file again.
(Untested.)

Comment 10

[[e-mail]]

Comment on attachment 508206 [details] [diff] [review]
(Bv1) Port bug 621274
[Checked in: See comment 11]

>+    let self = this;
Bah, this content sandbox is annoying. I wish I could find someone who could verify that we can just ditch it :-(

>+      function (aURI, aDataLen, aData, aMimeType) {
Nit: no space between function and (

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 508206 [details] [diff] [review]
(Bv1) Port bug 621274
[Checked in: See comment 11]

http://hg.mozilla.org/comm-central/rev/c4f4707e77cf
Bv1, with comment 10 suggestion(s).

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> >+    let self = this;
> Bah, this content sandbox is annoying. I wish I could find someone who could
> verify that we can just ditch it :-(

(File a bug...)

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 508207 [details] [diff] [review]
(Cv1) Just copy bug 597995
[Checked in: Comment 14]

Are we failing any tests [from this file?] from not porting this change? and I think this should be tested [at least single-file, locally] before landing.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 508207 [details] [diff] [review]
(Cv1) Just copy bug 597995
[Checked in: Comment 14]

http://hg.mozilla.org/comm-central/rev/092a73c5d936


(In reply to comment #13)
> Are we failing any tests [from this file?] from not porting this change? and I
> think this should be tested [at least single-file, locally] before landing.

No reported failure without this patch.
This test still succeeds with this patch on my local Windows 2000.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> Comment on attachment 505956 [details] [diff] [review]
> bug 394353 port [Checkin: comment 6]
> 
> http://hg.mozilla.org/comm-central/rev/2e8f93800bce

This changeset ported their v1.0 patch :-)

But, a few days later, they landed a part 2 patch :-|
Reopening, to complete porting that bug here too:
Jens, can you do it? Thanks.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #15)
> But, a few days later, they landed a part 2 patch :-|

I know.

> Jens, can you do it?

No. Of course I could, but I still have 26 Sync follow-ups in front of me, and since I doubt anyone beside the reviewers will help me there I need to prioritize in order to keep some level of sanity.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Just copy bug 394353 2nd patch, Resync' 3 nits [Checked in: Comment 19]

This "fully" resync' with Firefox file:
remaining differences are Ci/Cr/etc and 2 blank lines.
Untested.

Comment 18

[[e-mail]]

Comment on attachment 510243 [details] [diff] [review]
(Dv1) Just copy bug 394353 2nd patch, Resync' 3 nits
[Checked in: Comment 19]

This patch looks strangely familiar ;-)

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 510243 [details] [diff] [review]
(Dv1) Just copy bug 394353 2nd patch, Resync' 3 nits
[Checked in: Comment 19]

http://hg.mozilla.org/comm-central/rev/5bb7cde86a93