743692 - Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey.

(Untested) I hope i got it right...

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Serge Gautherie (:sgautherie) from comment #1)
> (Untested) I hope i got it right...

Err, so who exactly should take a decision whether *this patch*, as is unreviewed and untested, should be allowed to land for Aurora/Beta?

If you want to make sure this bug gets fixed for Aurora/Beta, we have tracking flags for that (and you know it). Otherwise, I think one should only request branch approval for things that have been both reviewed (or which are trivial enough to expect passing review with only some nits, which is hardly the case here) and tested (at least so far that no obvious regressions occur, and that the patch indeed fixes the problem).

Additionally, whenever one requests branch approval now, Bugzilla should take care of adding an approval template comment. If that didn't happen in your case, I'd like to know what you did. Otherwise, please fill it in in the future instead of removing it [1]. I think it's important (read: very helpful) for those making the choice whether to grant approval or not. I don't know whether it's actually considered mandatory now, but I'm pretty sure we always required at least some risk assessment (mind you, landing something late in the Beta cycle can imply a broken release, which we certainly want to avoid, right?).

[1] Not all fields/lines are equally important there; I'm mainly concerned about the risks.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey [Split into patches B,C,D,E]

Av1, with a typo fix.

Comment 4

[[e-mail]]

Comment on attachment 613478 [details] [diff] [review]
(Av1a) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey
[Split into patches B,C,D,E]

I can't review this because the var/let and PlacesUtils changes make it impossible to tell whether there are any real changes.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits) [Checked in: See comment 11+12]

(Untested) I assume/hope the s/callback/batch/ changes are not required yet.

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 614028 [details] [diff] [review]
(Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits)
[Checked in: See comment 11+12]

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

::: suite/common/places/tests/unit/test_browserGlue_smartBookmarks.js
@@ +258,5 @@
>  
> +  // Initialize SuiteGlue, but remove its listener to places-init-complete.
> +  let bg = Cc["@mozilla.org/suite/suiteglue;1"].getService(Ci.nsISuiteGlue);
> +  // Initialize Places.
> +  PlacesUtils.history;

|PlacesUtils.history;| is probably redundant in this patch (as |var hs| isn't removed yet), but I assume it doesn't hurt.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #6)
> |PlacesUtils.history;| is probably redundant in this patch (as |var hs|
> isn't removed yet), but I assume it doesn't hurt.

Or maybe both are needed: anyway.

Comment 8

[[e-mail]]

Comment on attachment 614028 [details] [diff] [review]
(Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits)
[Checked in: See comment 11+12]

> function rebuildSmartBookmarks() {
>-  var consoleListener = {
>+  let consoleListener = {
Drop all the var/let changes, they're not useful.

>-    print("\nTEST " + (++testIndex) + ": " + test.description);
>+    print("\nTEST: " + test.description);
Why this change?

>+  let bg = Cc["@mozilla.org/suite/suiteglue;1"].getService(Ci.nsISuiteGlue);
>+  // Initialize Places.
>+  PlacesUtils.history;
>+  // Usually places init would async notify to glue, but we want to avoid
>+  // randomness here, thus we fire the notification synchronously.
>   bg.QueryInterface(Ci.nsIObserver).observe(null, "places-init-complete", null);
Could getService(Ci.nsIObserver) perhaps?

>-    } catch(ex) { /* no version set, new profile */ }
>+    } catch(ex) {}
Why this change?

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #8)

(It seems we two just hit the same debate quite often :-/)

> Drop all the var/let changes, they're not useful.

Can't we just copy Firefox code, to reduce (maintenance and execution) diffs?
It's just test code (and these nits are unrelated to what is tested).
And, wrt the application code, 'let' is already used in that (even both) file.

NB: I understand it may add work for you as a reviewer, but I believe it's better to get it done once than hit the diffs over and over again :-|

> >-    print("\nTEST " + (++testIndex) + ": " + test.description);
> >+    print("\nTEST: " + test.description);
> Why this change?

I assume they decided/noticed 'test.description' is enough, and (varying) numbers are not very helpful anyway.
At least, I would agree with that.

> Could getService(Ci.nsIObserver) perhaps?

Yes: to be sync'ed in a next part patch.
(You wanted to have more readable/reviewable parts... (I can understand that.))

> >-    } catch(ex) { /* no version set, new profile */ }
> >+    } catch(ex) {}
> Why this change?

I guess they thought this comment was somewhat redundant with the one before the 'try' block. (Or maybe that the 'new profile' assumption was not always right. Anyway.)
At least, I would agree with that.

(In short, I do think this patch is fine as it is.)

Comment 10

[[e-mail]]

Comment on attachment 614028 [details] [diff] [review]
(Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits)
[Checked in: See comment 11+12]

OK, so these are just syncing our test with the old version of their test?

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 614028 [details] [diff] [review]
(Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits)
[Checked in: See comment 11+12]

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


(In reply to [e-mail] from comment #10)
> these are just syncing our test with the old version of their test?

I'm not sure what you mean ... This bug ports bug 493557 (only) and may not fully resync' these files with current Firefox files: additional resync' will be for other bugs.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 614028 [details] [diff] [review]
(Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits)
[Checked in: See comment 11+12]

http://hg.mozilla.org/comm-central/rev/5344f7011330
"Cv1 ... typo fixes for patch Bv1"

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 2 (Var renames, ...) [Checked in: Comment 16+26]

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 3 (Logic changes) [Checked in: See comment 24]

(Ah, patches D and E are untested.)


(In reply to Serge Gautherie (:sgautherie) from comment #11)
> (In reply to [e-mail] from comment #10)
> > these are just syncing our test with the old version of their test?
> 
> I'm not sure what you mean ... This bug ports bug 493557 (only) and may not
> fully resync' these files with current Firefox files: additional resync'
> will be for other bugs.

Ftr, this just makes the tests "identical".

Comment 15

[[e-mail]]

Comment on attachment 615225 [details] [diff] [review]
(Ev1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 3 (Logic changes)
[Checked in: See comment 24]

>+            let smartBookmark = smartBookmarks[queryId];
Unused variable. r=me with that removed.

>+          // Remove old version of the smart bookmark if it exists, since it
>+          // will be replaced in place.
>+          if (smartBookmark.itemId)
>+            PlacesUtils.bookmarks.removeItem(smartBookmark.itemId);
And, it's now pretty easy to see what the actual fix is!
(Although, it's possible that -w might have made it even easier still.)

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 615224 [details] [diff] [review]
(Dv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 2 (Var renames, ...)
[Checked in: Comment 16+26]

http://hg.mozilla.org/comm-central/rev/3fdc5f308109

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1-FF) nsBrowserGlue.js: fix some nits

(In reply to [e-mail] from comment #15)
> >+            let smartBookmark = smartBookmarks[queryId];
> Unused variable. r=me with that removed.

Right, I forgot about that one: let's make use of it rather.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #16)
> http://hg.mozilla.org/comm-central/rev/3fdc5f308109

Though I didn't expect this changeset to have any effect, it caused
{
TEST-UNEXPECTED-FAIL | e:/builds/slave/test/build/xpcshell/tests/suite/common/places/tests/unit/test_421483.js | 0 != 0 - See following stack:
[...]
JS frame :: e:/builds/slave/test/build/xpcshell/tests/suite/common/places/tests/unit/test_421483.js :: run_test :: line 82
}

(up-to-date) Test code is
{
78   // TEST 2: create smart bookmarks
79   Services.prefs.setIntPref("browser.places.smartBookmarksVersion", 0);
80   gluesvc.ensurePlacesDefaultQueriesInitialized();
81   smartBookmarkItemIds = annosvc.getItemsWithAnnotation(SMART_BOOKMARKS_ANNO);
82   do_check_neq(smartBookmarkItemIds.length, 0);
83   // check that pref has been bumped up
84   do_check_true(Services.prefs.getIntPref("browser.places.smartBookmarksVersion") > 0);
}

My guess would be the s/Services.io.newURI/NetUtil.newURI/g,
but we'll see after patch E has landed...

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #18)

> Though I didn't expect this changeset to have any effect, it caused

It also caused:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/places/tests/browser_library_infoBox.js | Correctly selected recently bookmarked node. - Got SeaMonkey and Mozilla, expected Recently Bookmarked
[...]
}

> but we'll see after patch E has landed...

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 615540 [details] [diff] [review]
(Fv1-FF) nsBrowserGlue.js: fix some nits

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

please avoid style changes where you are not actually changing code

::: browser/components/nsBrowserGlue.js
@@ +1371,1 @@
>        return;

no reason to unbrace ifs, if you want to stick to the coding stule all ifs should be braced, I don't care that much regarding that, but this is actually changing blame for no valid reason.

@@ +1425,5 @@
>              let smartBookmark = smartBookmarks[queryId];
> +            smartBookmark.itemId = itemId;
> +            smartBookmark.parent = PlacesUtils.bookmarks.getFolderIdForItem(itemId);
> +            smartBookmark.position = PlacesUtils.bookmarks.getItemIndex(itemId);
> +          } else {

nsBrowserGlue doesn't use cuddled else, so please don't change this.
I know the coding style is for them, but that also depends on the surrounding code.

@@ +1451,1 @@
>              PlacesUtils.bookmarks.removeItem(smartBookmark.itemId);

don't unbrace

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Fv2-FF) nsBrowserGlue.js: fix 2 nits [Checked in: Comment 23]

Fv1-FF, with comment 20 suggestion(s).

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 616394 [details] [diff] [review]
(Fv2-FF) nsBrowserGlue.js: fix 2 nits
[Checked in: Comment 23]

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

yes, this is stricter on the issue :)

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 616394 [details] [diff] [review]
(Fv2-FF) nsBrowserGlue.js: fix 2 nits
[Checked in: Comment 23]

https://hg.mozilla.org/mozilla-central/rev/c55ce76c8dac

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 615225 [details] [diff] [review]
(Ev1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 3 (Logic changes)
[Checked in: See comment 24]

http://hg.mozilla.org/comm-central/rev/d04fc59c787f
Ev1, sync'ed with patch Fv2-FF.

Comment 25

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 615225 [details] [diff] [review]
(Ev1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 3 (Logic changes)
[Checked in: See comment 24]

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

::: suite/common/src/nsSuiteGlue.js
@@ +914,5 @@
>  
> +        let smartBookmarks = {
> +          MostVisited: {
> +            title: bundle.GetStringFromName("mostVisitedTitle"),
> +            uri: NetUtil.newURI("place:redirectsMode=" +

This (and the other two occurrences) broke us since unlike nsBrowserGlue.js, we don't have NetUtil in this context (file). You can even see that if you look at attachment 492628 [details] [diff] [review] (from the bug you ported, beginning of context).

Error: NetUtil is not defined
Source File: resource:///components/nsSuiteGlue.js
Line: 919

Comment 26

[Jens Hatlak (:InvisibleSmiley)]

Pushed bustage fix with rs=Ratty over IRC:
http://hg.mozilla.org/comm-central/rev/fef14da47d5e

Aurora base had already been tagged and merged, but I got an OK from Standard8 on IRC for landing despite the closed tree:
http://hg.mozilla.org/releases/comm-aurora/rev/3b8d54c2d8ec

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> My guess would be the s/Services.io.newURI/NetUtil.newURI/g,

I (had) made just that change in
[Mozilla/5.0 (Windows NT 5.0; rv:12.0a1) Gecko/20120126 Firefox/12.0a1 SeaMonkey/2.9a1] (custom debug build for Windows 2000)
and both xpcshell and browser-chrome tests still pass.
Unless I did something wrong, I'm rather clueless.


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #25)

> Error: NetUtil is not defined
> Source File: resource:///components/nsSuiteGlue.js
> Line: 919

I wonder how to reproduce this error (with my older/custom 2.9/2.10 builds)?


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #26)
> http://hg.mozilla.org/comm-central/rev/fef14da47d5e
> http://hg.mozilla.org/releases/comm-aurora/rev/3b8d54c2d8ec

Thanks for looking into this! Hope it will actually fix that regression.

Comment 28

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Serge Gautherie (:sgautherie) from comment #27)
> (In reply to Jens Hatlak (:InvisibleSmiley) from comment #25)
> 
> > Error: NetUtil is not defined
> > Source File: resource:///components/nsSuiteGlue.js
> > Line: 919
> 
> I wonder how to reproduce this error (with my older/custom 2.9/2.10 builds)?

No idea about older builds, but for trunk (now Aurora) prior to my fix (e.g. latest nightly as of now) you could do the following:
1. create fresh profile - see that no special Places folders are present in the BM
2. go to about:config and reset browser.places.smartBookmarksVersion
3. restart
4. open Error Console

> (In reply to Jens Hatlak (:InvisibleSmiley) from comment #26)
> > http://hg.mozilla.org/comm-central/rev/fef14da47d5e
> > http://hg.mozilla.org/releases/comm-aurora/rev/3b8d54c2d8ec
> 
> Thanks for looking into this! Hope it will actually fix that regression.

It will (I checked before checking in, just forgot to add that info to my comment). 

[I didn't check the tests, so in theory the tests could be regressed by my bustage fix, but I was really more concerned with fixing actual application code ASAP.]

Comment 29

[Serge Gautherie (:sgautherie)]

Was not ready to land for SeaMonkey 2.9 :-/

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #27)
> Hope it will actually fix that regression.

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1335307535.1335313861.7058.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/04/24 15:45:35
s: sea-win32-02

Fixed browser_library_infoBox.js (failure) regression :-)

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1335320201.1335325922.29426.gz
WINNT 5.2 comm-central-trunk debug test xpcshell on 2012/04/24 19:16:41
s: cb-seamonkey-win32-01

Fixed test_421483.js (failure) and test_browserGlue_corrupt.js (timeout) regressions. :-)

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 614028 [details] [diff] [review]
(Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits)
[Checked in: See comment 11+12]

[Approval Request Comment]
No risk (= has test).

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 615224 [details] [diff] [review]
(Dv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 2 (Var renames, ...)
[Checked in: Comment 16+26]

[Approval Request Comment]
No risk (= has test).

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 615225 [details] [diff] [review]
(Ev1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 3 (Logic changes)
[Checked in: See comment 24]

[Approval Request Comment]
No risk (= has test).

Comment 34

[Justin Wood (:Callek)]

Comment on attachment 614028 [details] [diff] [review]
(Bv1) Port |Bug 493557 - "Recent Tags" and "Recently Bookmarked" are flipped when smart bookmarks are updated| to SeaMonkey, part 1 (Use PlacesUtils.jsm, Nits)
[Checked in: See comment 11+12]

This has a non-trivial (to a quick skim) hunk that won't apply. So I'm marking a-, if we can get this fixed up for beta and a quick review from a peer I'm ok with taking it within the week.

Comment 35

[Serge Gautherie (:sgautherie)]

(In reply to Justin Wood (:Callek) from comment #34)
> I'm ok with taking it within the week.

I'll just give up on this bug.