Last Comment Bug 627416 - Port Firefox changes from Jan 20 places merge
: Port Firefox changes from Jan 20 places merge
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
Depends on: 394353 597995 621274 627408
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-20 10:24 PST by Robert Kaiser
Modified: 2011-02-07 03:59 PST (History)
3 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
bug 394353 port [Checkin: comment 6] (1.76 KB, patch)
2011-01-21 14:22 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review
(Bv1) Port bug 621274 [Checked in: See comment 11] (7.00 KB, patch)
2011-01-30 00:04 PST, Serge Gautherie (:sgautherie)
neil: review+
Details | Diff | Splinter Review
(Cv1) Just copy bug 597995 [Checked in: Comment 14] (3.24 KB, patch)
2011-01-30 00:16 PST, Serge Gautherie (:sgautherie)
bugspam.Callek: review+
Details | Diff | Splinter Review
(Dv1) Just copy bug 394353 2nd patch, Resync' 3 nits [Checked in: Comment 19] (9.79 KB, patch)
2011-02-07 03:07 PST, Serge Gautherie (:sgautherie)
neil: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2011-01-20 10:24:11 PST
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) 2011-01-20 14:58:44 PST
Bug 620198 ported in bug 627408 which I just checked in. Two to go (one file each).
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-01-21 14:22:33 PST
Created attachment 505956 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-01-21 15:41:03 PST
(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 neil@parkwaycc.co.uk 2011-01-21 15:46:33 PST
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 neil@parkwaycc.co.uk 2011-01-21 15:52:09 PST
(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) 2011-01-22 03:40:12 PST
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) 2011-01-22 03:44:50 PST
One to go (bug 621274, FeedWriter.js), so leaving bug open.
Comment 8 Serge Gautherie (:sgautherie) 2011-01-30 00:04:48 PST
Created attachment 508206 [details] [diff] [review]
(Bv1) Port bug 621274
[Checked in: See comment 11]

Untested. (Sorry.)
Comment 9 Serge Gautherie (:sgautherie) 2011-01-30 00:16:00 PST
Created attachment 508207 [details] [diff] [review]
(Cv1) Just copy bug 597995
[Checked in: Comment 14]

Simply make it identical to FF file again.
(Untested.)
Comment 10 neil@parkwaycc.co.uk 2011-02-01 08:33:33 PST
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) 2011-02-01 09:55:46 PST
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) 2011-02-01 10:15:00 PST
(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) (Away until Aug 29) 2011-02-01 19:18:31 PST
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) 2011-02-01 21:52:34 PST
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) 2011-02-06 03:07:28 PST
(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) 2011-02-06 04:02:25 PST
(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) 2011-02-07 03:07:15 PST
Created attachment 510243 [details] [diff] [review]
(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 neil@parkwaycc.co.uk 2011-02-07 03:16:19 PST
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) 2011-02-07 03:58:37 PST
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

Note You need to log in before you can comment on or make changes to this bug.