Robert Kaiser Assignee
	Description • 15 years ago

The reviews on the SeaMonkey place bookmarks bug 498596 are bringing up some improvements we should port back to the Firefox side as well. I'll try to bring up patches here as we go on that other bug, so that this review profits both sides and makes porting of further work easier.

	Robert Kaiser Assignee
	Comment 1 • 15 years ago

This patch addresses the first round of Neil's comments over in bug 498596.

	Shawn Wilsher :sdwilsh
	Comment 2 • 15 years ago

Comment on attachment 449922 [details] [diff] [review]
round 1 of Neil's comments (checked in)

>+++ b/browser/components/places/src/PlacesUIUtils.jsm
>-XPCOMUtils.defineLazyGetter(this, "Services", function() {
>-  Cu.import("resource://gre/modules/Services.jsm");
>-  return Services;
>-});
>+Cu.import("resource://gre/modules/Services.jsm");
I think we still want to do this import lazily.  Is there a reason why you don't think we should do this?

>   _confirmOpenInTabs: function PUIU__confirmOpenInTabs(numTabsToOpen) {
>-    let pref = Services.prefs;
>-    let prompt = Services.prompt;
Not really sure of the benefit of this change.  We did this originally so things were not so verbose...

r=sdwilsh, but I want answers to the above questions.  (mak, feel free to chime in here too)

	Robert Kaiser Assignee
	Comment 3 • 15 years ago

(In reply to comment #2)
> (From update of attachment 449922 [details] [diff] [review])
> >+++ b/browser/components/places/src/PlacesUIUtils.jsm
> >-XPCOMUtils.defineLazyGetter(this, "Services", function() {
> >-  Cu.import("resource://gre/modules/Services.jsm");
> >-  return Services;
> >-});
> >+Cu.import("resource://gre/modules/Services.jsm");
> I think we still want to do this import lazily.  Is there a reason why you
> don't think we should do this?

Services.jsm is all-lazy internally, so there should not be a reason to call it lazily. Also, it actually should already be loaded in any case, esp. as nsBrowserGlue.js loads it as well. Defining a lazy getter is probably not just more complex but possibly even slower than just directly "loading" (actually referencing) it here.

> >   _confirmOpenInTabs: function PUIU__confirmOpenInTabs(numTabsToOpen) {
> >-    let pref = Services.prefs;
> >-    let prompt = Services.prompt;
> Not really sure of the benefit of this change.  We did this originally so
> things were not so verbose...

It doesn't change verbosity that much, but possibly even saves us loading the prompt service if it's not instantiated yet and not really needed here. The whole purpose of Services.jsm - except avoiding to instantiate services multiple times - is to only load them lazily and making code easier to understand, and IMHO (and Neil seems to think the same way) putting up yet another var and using that instead makes it harder to understand as you need to dereference the var in your brain, while directly using Services.* is uniform in all places where it's used. Just my 2c on that.

	Marco Bonardo [:mak]
	Comment 4 • 15 years ago

Services.jsm at that point is very likely already included (utilityOverlay), so I think Neil is correct saying there is no reason to have it even lazier in PU.

+    // We reach the XUL document here as it's the parent node of the root element

just kill the comment, not really useful.

other parts are fine imo.

	Shawn Wilsher :sdwilsh
	Comment 5 • 15 years ago

(In reply to comment #3)
k

	Robert Kaiser Assignee
	Comment 6 • 15 years ago

Comment on attachment 449922 [details] [diff] [review]
round 1 of Neil's comments (checked in)

Pushed this "round 1" patch as http://hg.mozilla.org/mozilla-central/rev/b85d67f6361f

	Robert Kaiser Assignee
	Comment 7 • 14 years ago

Here's another patch with round 2 of Neil's comments, including bug 498596 comment #49, bug 580660 comment #6 as well as bug 580662 comment #9 and following.

	Robert Kaiser Assignee
	Comment 8 • 14 years ago

I'll keep this to patches that don't grow too large. ;-)

Here's one with Ian's nits on PlacesUIUtils, controller, and sidebarUtils, including my own nit/whitespace fixes I had done in my work on the SeaMonkey patches. A lot of this is removal of trailing whitespace, someone seems to have an editor that likes creating that...

	Robert Kaiser Assignee
	Comment 9 • 14 years ago

Here's the hopefully last round from this one, with my own and Ian's nits/comments on the tree code, see bug 580656 comment #14.
Ian nicely found out that we had a few places where we can inline variables we only are using once, and that we can use isSorted() in some places instead of repeating the direct check every time.

	Marco Bonardo [:mak]
	Comment 10 • 14 years ago

Comment on attachment 463848 [details] [diff] [review]
round 2 of Neil's comments

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>   _saveSession: false,
>-  _isIdleObserver: false,
>-  _isPlacesInitObserver: false,
>-  _isPlacesLockedObserver: false,
>-  _isPlacesShutdownObserver: false,
>+  _hasIdleObserver: false,
>+  _hasPlacesInitObserver: false,
>+  _hasPlacesLockedObserver: false,
>+  _hasPlacesShutdownObserver: false,
>   _isPlacesDatabaseLocked: false,

I'm not sure these changes are much valuable, both are true, the service is an observer by itself since the observer is "this". Sounds like polluting blame, but if you have specific reasons to believe is is incorrect provide them please.

other changes are fine, but waiting for answer above.

	Marco Bonardo [:mak]
	Comment 11 • 14 years ago

Comment on attachment 463850 [details] [diff] [review]
round 3 - my own and IanN's nits (PUIU, PC, sbUtils) (checked in)


>     function makeXferable(types) {
>-      var xferable = 
>+      var xferable =
>           Cc["@mozilla.org/widget/transferable;1"].
>           createInstance(Ci.nsITransferable);

this doesn't look like needing a new line, is CC["..."] going over 80 chars if you put it above? if you can plase move Cc to tha same line as var xferable

>   /**
>    * Allows opening of javascript/data URI only if the given node is
>    * bookmarked (see bug 224521).
>    * @param aURINode
>    *        a URI node
>+   * @param aWindow
>+   *        a window on which a potential an error alert is shown on.

"a potential an error" looks wrong

	Robert Kaiser Assignee
	Comment 12 • 14 years ago

(In reply to comment #10)
> I'm not sure these changes are much valuable, both are true, the service is an
> observer by itself since the observer is "this". Sounds like polluting blame,
> but if you have specific reasons to believe is is incorrect provide them
> please.

Well, Neil demanded them on the SeaMonkey side, and it's easier to port back and forth when it's done on both sides - that's my core reason for doing this here.
That said, I see a logical difference between if we _have_ an observer for something and if the places DB _is_ locked, and IMHO it makes sense to have the var names between those differ. And, actually, the var value matters to us in terms of _having_ an observer added for that and needing to remove it at some point.
Still, while I understand his logic, my primary motivation to get this by you is to ease my future porting of changes to this, subtle var naming differences are easy to get wrong in those jobs.

	Robert Kaiser Assignee
	Comment 13 • 14 years ago

Comment on attachment 463850 [details] [diff] [review]
round 3 - my own and IanN's nits (PUIU, PC, sbUtils) (checked in)

Asking for approval - low risk, mostly whitespace and minor nits fixes to sync up with reviewqed code on the SeaMonkey side.

	Robert Kaiser Assignee
	Comment 14 • 14 years ago

Comment on attachment 463864 [details] [diff] [review]
round 4 - my own and IanN's nits (tree) (checked in)

Asking for approval - low risk, nit fixes / minor code simplifications to sync up with reviewed code on the SeaMonkey side.

	Robert Kaiser Assignee
	Comment 15 • 14 years ago

Comment on attachment 463850 [details] [diff] [review]
round 3 - my own and IanN's nits (PUIU, PC, sbUtils) (checked in)

rund 3 pushed as http://hg.mozilla.org/mozilla-central/rev/bc371fcc9c6a

	Robert Kaiser Assignee
	Comment 16 • 14 years ago

Comment on attachment 463864 [details] [diff] [review]
round 4 - my own and IanN's nits (tree) (checked in)

round 4 pushed as http://hg.mozilla.org/mozilla-central/rev/c047cfdc5227

Marco, where are we going wrt those glue variables in the round 2 patch?

	Marco Bonardo [:mak]
	Comment 17 • 14 years ago

(In reply to comment #16)
> Marco, where are we going wrt those glue variables in the round 2 patch?

Imho, for now nsSuiteGlue should use nsBrowserGlue names, even if I see you don't like them.

	Robert Kaiser Assignee
	Comment 18 • 14 years ago

Here's round 2 without the glue var changes.

I filed bug 588027 separately on the library cleanups Neil commented on, as that probably will need ui-reviews.

	Robert Kaiser Assignee
	Comment 19 • 14 years ago

Here's another nit found by Neil: no element with the ID contentTitle does exist, but we have style for it.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 20 • 14 years ago

Comment on attachment 466640 [details] [diff] [review]
round 2 of Neil's comments, without glue var changes

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>-                           onpopupshowing="document.getElementById('PlacesToolbar')
>-                                                   ._placesView._onChevronPopupShowing(event);">
>+                           onpopupshowing="this.parentNode.parentNode
>+                                               ._placesView._onChevronPopupShowing(event);">

This change doesn't really seem beneficial, but I think it was bitrotten by http://hg.mozilla.org/mozilla-central/rev/8280bf7b49aa anyways. Otherwise this patch is fine.

	Robert Kaiser Assignee
	Comment 21 • 14 years ago

Gavin already said in the last comment that the rest would be good, so requesting his review and approval at the same time.

	Justin Wood (:Callek)
	Comment 22 • 14 years ago

Comment on attachment 468165 [details] [diff] [review]
round 5 - CSS nit [checked-in]

Pushed as http://hg.mozilla.org/mozilla-central/rev/e1256c19216c

	Justin Wood (:Callek)
	Comment 23 • 14 years ago

Comment on attachment 469692 [details] [diff] [review]
round 2 of Neil's comments, without browser.xul change [checked-in]

Pushed as http://hg.mozilla.org/mozilla-central/rev/9bc2b06e2793

	Robert Kaiser Assignee
	Comment 24 • 14 years ago

Thanks to gavin reviewing fast and Callek pushing as a ride-along to some other checkins, this is now fixed!

All other followups are in their own bugs.