Closed Bug 570788 Opened 10 years ago Closed 9 years ago

Incorporate improvements from SeaMonkey places review into Firefox code

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b5

People

(Reporter: kairo, Assigned: kairo)

References

Details

Attachments

(5 files, 2 obsolete files)

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.
This patch addresses the first round of Neil's comments over in bug 498596.
Attachment #449922 - Flags: review?
Attachment #449922 - Flags: review? → review?(mak77)
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)
Attachment #449922 - Flags: review?(mak77) → review+
(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.
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.
(In reply to comment #3)
k
Attachment #449922 - Attachment description: round 1 of Neil's comments → round 1 of Neil's comments (checked in)
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
Attached patch round 2 of Neil's comments (obsolete) — Splinter Review
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.
Attachment #463848 - Flags: review?
Attachment #463848 - Flags: review? → review?(mak77)
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...
Attachment #463850 - Flags: review?(mak77)
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.
Attachment #463864 - Flags: review?(mak77)
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.
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
Attachment #463850 - Flags: review?(mak77) → review+
Attachment #463864 - Flags: review?(mak77) → review+
(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.
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.
Attachment #463850 - Flags: approval2.0?
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.
Attachment #463864 - Flags: approval2.0?
Attachment #463850 - Flags: approval2.0? → approval2.0+
Attachment #463864 - Flags: approval2.0? → approval2.0+
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
Attachment #463850 - Attachment description: round 3 - my own and IanN's nits (PUIU, PC, sbUtils) → round 3 - my own and IanN's nits (PUIU, PC, sbUtils) (check3ed in)
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?
Attachment #463864 - Attachment description: round 4 - my own and IanN's nits (tree) → round 4 - my own and IanN's nits (tree) (checked in)
Attachment #463850 - Attachment description: round 3 - my own and IanN's nits (PUIU, PC, sbUtils) (check3ed in) → round 3 - my own and IanN's nits (PUIU, PC, sbUtils) (checked in)
(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.
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.
Attachment #463848 - Attachment is obsolete: true
Attachment #466640 - Flags: review?(mak77)
Attachment #463848 - Flags: review?(mak77)
Here's another nit found by Neil: no element with the ID contentTitle does exist, but we have style for it.
Attachment #468165 - Flags: review?
Attachment #468165 - Flags: review? → review?(mak77)
Attachment #468165 - Flags: review?(mak77)
Attachment #468165 - Flags: review+
Attachment #468165 - Flags: approval2.0+
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.
Attachment #466640 - Flags: review?(mak77) → review-
Gavin already said in the last comment that the rest would be good, so requesting his review and approval at the same time.
Attachment #466640 - Attachment is obsolete: true
Attachment #469692 - Flags: review?(gavin.sharp)
Attachment #469692 - Flags: approval2.0?
Attachment #469692 - Flags: review?(gavin.sharp)
Attachment #469692 - Flags: review+
Attachment #469692 - Flags: approval2.0?
Attachment #469692 - Flags: approval2.0+
Comment on attachment 468165 [details] [diff] [review]
round 5 - CSS nit [checked-in]

Pushed as http://hg.mozilla.org/mozilla-central/rev/e1256c19216c
Attachment #468165 - Attachment description: round 5 - CSS nit → round 5 - CSS nit [checked-in]
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
Attachment #469692 - Attachment description: round 2 of Neil's comments, without browser.xul change → round 2 of Neil's comments, without browser.xul change [checked-in]
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b5
You need to log in before you can comment on or make changes to this bug.