Last Comment Bug 709589 - Some engine manager cleanup found by SeaMonkey reviews
: Some engine manager cleanup found by SeaMonkey reviews
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Robert Kaiser
:
: Florian Quèze [:florian] [:flo]
Mentors:
Depends on: 719990
Blocks: 728508
  Show dependency treegraph
 
Reported: 2011-12-11 08:13 PST by Robert Kaiser
Modified: 2012-02-24 03:51 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
services, v1 (10.39 KB, patch)
2011-12-11 09:27 PST, Robert Kaiser
gavin.sharp: review+
Details | Diff | Splinter Review
engine.name, v1 (2.07 KB, patch)
2011-12-11 09:29 PST, Robert Kaiser
gavin.sharp: review+
Details | Diff | Splinter Review
observer, v1 (1.02 KB, patch)
2011-12-11 09:34 PST, Robert Kaiser
gavin.sharp: review+
Details | Diff | Splinter Review
_cloneEngine, v1 (1.04 KB, patch)
2011-12-11 09:39 PST, Robert Kaiser
gavin.sharp: review+
Details | Diff | Splinter Review
onSelect, v1 (2.33 KB, patch)
2011-12-11 09:55 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
onSelect, v1 (2.31 KB, patch)
2011-12-11 09:56 PST, Robert Kaiser
gavin.sharp: review+
Details | Diff | Splinter Review
nits, v1 (8.61 KB, patch)
2011-12-11 10:02 PST, Robert Kaiser
gavin.sharp: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2011-12-11 08:13:15 PST
Splitting off the engine manager parts from bug 643172.
Comment 1 Robert Kaiser 2011-12-11 09:27:03 PST
Created attachment 580760 [details] [diff] [review]
services, v1

Here's the first and probably largest part - Switching engine manager to using Services.jsm, and as it's closely connected, it also removes redundancy in removing the observer, putting it in a common destroy/onunload function, obsoleting the onCancel one.
Comment 2 Robert Kaiser 2011-12-11 09:29:22 PST
Created attachment 580762 [details] [diff] [review]
engine.name, v1

This patch fixes the use of engine.name outside the loop where it's declared with a let. This is a generic bug in current engine manager, as there it currently must end up being undefined, the way I'm reading the code (and Neil agreed with that POV in the SeaMonkey reviews).
Comment 3 Robert Kaiser 2011-12-11 09:34:43 PST
Created attachment 580764 [details] [diff] [review]
observer, v1

Here's a patch for the observer - only "engine-changed" needs to .invalidate() as .rowCountChanged() does the invalidate/update to the tree display anyhow (in an optimized way to only do it for the affected rows), calling .invalidate() there will just be more work with the same effect. This ends up being a slight perf fix.
(Also, the "Not relevant" cases also can now exit the function normally, no need to early return there).
Comment 4 Robert Kaiser 2011-12-11 09:39:16 PST
Created attachment 580765 [details] [diff] [review]
_cloneEngine, v1

This patch makes _cloneEngine easier to read and debug (and also makes the object init to be an object and not an array).
When I read this function the first time, I had difficulty wrapping my head around what this intermediate variable was, in the patched version it's clear that it's a cloned object. Also, the function name now correlates with the method name it has in EngineStore.
Comment 5 Robert Kaiser 2011-12-11 09:55:17 PST
Created attachment 580766 [details] [diff] [review]
onSelect, v1

This patch makes onSelect better readable, also avoiding another intermittent variable and using the nice fact that if we only have one engine left, it's both the first and last one in the list.
Comment 6 Robert Kaiser 2011-12-11 09:56:47 PST
Created attachment 580767 [details] [diff] [review]
onSelect, v1

Actually, this is the correct patch for this (the original comment is correct).
Comment 7 Robert Kaiser 2011-12-11 10:02:27 PST
Created attachment 580769 [details] [diff] [review]
nits, v1

OK, here's the last patch in this series, consisting of a number of nits - whitespace fixes, superfluous brackets, missing semicolons, superfluous "!important" markers, unneeded spacers, a better understandable button ID - and using the fact that we can use .currentIndex instead of re-calculating the position of the just-selected item another time.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-12 13:14:32 PST
Comment on attachment 580762 [details] [diff] [review]
engine.name, v1

This fixes the engine duplicate case, but doesn't properly handle the bookmark duplication case (dupName will be ""), so it's necessary but not sufficient. I guess maybe we need to have two different error messages ("used by 'Someengine'" vs. "used by a bookmark.").
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-12 13:17:09 PST
Before landing, please make sure that all of these changes together don't break the engine manager with manual testing, if you haven't already.

Thanks for doing this!
Comment 10 Robert Kaiser 2012-01-19 04:54:56 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Before landing, please make sure that all of these changes together don't
> break the engine manager with manual testing, if you haven't already.

I had done this before and did it again (including adding/resorting/keywording/removing engines) before pushing. I also pushed to try to be extra sure (and found something it doesn't like in the bug 643172 patches, still need to investigate that, but the ones here are fine).

> Thanks for doing this!

I just hope it will still be helpful, given that work to move the engine management into add-on manager has been picked up again (which is good in its own respect). ;-)


http://hg.mozilla.org/integration/mozilla-inbound/rev/84becc09d904
http://hg.mozilla.org/integration/mozilla-inbound/rev/79cb693afd54
http://hg.mozilla.org/integration/mozilla-inbound/rev/3374fe9737a9
http://hg.mozilla.org/integration/mozilla-inbound/rev/c8ed809f6a9d
http://hg.mozilla.org/integration/mozilla-inbound/rev/622f1d1e7c52
Comment 12 Robert Kaiser 2012-01-19 11:05:00 PST
Let's leave the bug open as one patch didn't get the reviews to land right now. I'll look into that one as soon as I have some time.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-17 21:37:20 PST
Comment on attachment 580762 [details] [diff] [review]
engine.name, v1

Reuben pointed out in bug 728508 that I misunderstood this code - there already are two different strings, and the alert prompt uses the right one depending on whether eduplicate is true. So this patch is fine!
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-17 21:37:43 PST
(sorry about that)
Comment 15 Robert Kaiser 2012-02-23 06:42:14 PST
OK, pushed this one as well: https://hg.mozilla.org/integration/mozilla-inbound/rev/f21351397aa7
Comment 16 Richard Newman [:rnewman] 2012-02-23 18:52:52 PST
https://hg.mozilla.org/mozilla-central/rev/f21351397aa7

Robert, please close this if everything has landed.
Comment 17 Robert Kaiser 2012-02-24 03:51:44 PST
Yes, this one is fixed now (bug 643172 isn't, though).

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