Closed
Bug 173258
Opened 22 years ago
Closed 22 years ago
controller.QueryInterface(Components.interfaces.nsICommandController) should be in goUpdateComposerMenuItems instead of goUpdateCommandState
Categories
(SeaMonkey :: Composer, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: timeless, Assigned: neil)
Details
Attachments
(2 files, 3 obsolete files)
2.36 KB,
patch
|
Brade
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
7.95 KB,
patch
|
cmanske
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
> Exception ``[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "JS frame :: chrome://editor/content/ComposerCommands.js :: goUpdateCommandState :: line 377" data: no]'' thrown from function goUpdateCommandState(command=string:"cmd_ul") in <chrome://editor/content/ComposerCommands.js> line 377. > Stopped for thrown exception. > #0: function goUpdateCommandState(command=string:"cmd_ul") in <chrome://editor/content/ComposerCommands.js> line 377 <brade> timeless: your build is from yesterday? > yes > basically i'm complaining about what appears to be a performance hotspot > and a really bad way to write some code > 375 try > 376 { > 377 cmdController = controller.QueryInterface(Components.interfaces.nsICommandController); > 378 } catch(e) {} > 379 if (!cmdController) return; > 1. return should be alone in catch <brade> I agree with #1 > 2. it appears that this function is called for a whole bunch of things sequentially. if the caller could do the QI test instead and only once, that'd save a whole lot of exceptions. <brade> 2. who is the caller? > line 446 > according to venkman stack,. > 436 cmanske 1.9 function goUpdateComposerMenuItems(commandset) > 437 { > 438 // dump("Updating commands for " + commandset.id + "\n"); > 439 > 440 for (var i = 0; i < commandset.childNodes.length; i++) > 441 { > 442 cmanske 1.154 var commandID = commandset.childNodes[i].id; > 443 cmanske 1.9 if (commandID) > 444 { > 445 brade 1.142 goUpdateCommand(commandID); // enable or disable > 446 goUpdateCommandState(commandID); > 447 cmanske 1.9 } > 448 } > 449 } <brade> timeless: ok, I agree with #2 in theory > it looks like you can store the info at line 439, and add if (thingPassedQITest) before 446. given how long this loop is, it would save a lot.
Attachment #102168 -
Attachment description: patch → broken
Assignee | ||
Comment 2•22 years ago
|
||
If there aren't many command controllers around, you could just cache the last found command controller, something like this: if (gCommandController != controller) { try { gCommandController = controller.QueryInterface(Components.interfaces.nsICommandController); } catch (e) { return; } } gCommandController.getCommandStateWithParams(command, params);
i've seen up to 5 controllers (+ placeholder), 3 return ok from the QI. I'd much prefer to be able to key by objects since that makes it O(1) and this approach is O(n). I haven't counted the number of consumers for each controller, i suppose I could. I did compare using venkman's profiler and this really does seem to be a win. right now the array lives outside the function to save on second uses but doesn't have a 'g' prefix. if someone from editor feels strongly about paying to rebuild it (move inside), or style (keep but rename), i'm flexible.
Attachment #102168 -
Attachment is obsolete: true
Comment 4•22 years ago
|
||
Comment on attachment 102207 [details] [diff] [review] second attempt r=cmanske, r=brade
Attachment #102207 -
Flags: review+
Comment on attachment 102207 [details] [diff] [review] second attempt sr=kin@netscape.com ==== Put a return in after |menuControllers|, also can you rename it |gMenuControllers| so it's obvious that it persists? +var menuControllers = [{o:null}]; function goUpdateComposerMenuItems(commandset) ==== Also, add a comment to the code that does the lookup/caching that explains what you're doing to make it more obvious to others.
Attachment #102207 -
Flags: superreview+
Assignee | ||
Comment 6•22 years ago
|
||
By only updating the state of those commands that have state I can be reasonably sure that the QI will always succeed, although in this example I have cached the result of the QI for extra speed.
Assignee | ||
Comment 7•22 years ago
|
||
Another attack would be to use instanceof to save using try/catch.
Attachment #102502 -
Attachment is obsolete: true
Comment 8•22 years ago
|
||
Comment on attachment 103058 [details] [diff] [review] Whoops, typo :-) r=brade
Attachment #103058 -
Flags: review+
Assignee | ||
Comment 10•22 years ago
|
||
Attachment #103058 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #103859 -
Flags: superreview?(kin)
Attachment #103859 -
Flags: review?(brade)
Assignee | ||
Comment 11•22 years ago
|
||
Comment on attachment 103859 [details] [diff] [review] Better patch using instanceof brade says to ask cmanske
Attachment #103859 -
Flags: review?(brade) → review?(cmanske)
cmanske : you should really review this patch ; editor is MUCH slower since the recent big landings. It's noticeably slower even on a fast PC. Very annoying.
Comment 13•22 years ago
|
||
Comment on attachment 103859 [details] [diff] [review] Better patch using instanceof glazman: Yes, I agree that this patch does help performance. I'm seeing one problem: the paragraph style menulist on the toolbar isn't initialized. I've investigated that and now I don't think it's related to this work.
Attachment #103859 -
Flags: review?(cmanske) → review+
Updated•22 years ago
|
Attachment #103859 -
Flags: superreview?(kin) → superreview?(alecf)
Comment 14•22 years ago
|
||
Comment on attachment 103859 [details] [diff] [review] Better patch using instanceof try/catch is often faster than any random manual check..but usually only by a little bit.. sr=alecf
Attachment #103859 -
Flags: superreview?(alecf) → superreview+
Comment 15•22 years ago
|
||
checked in; note that the patch on this bug has bitrotted so I checked in a different version that neil sent me privately.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 16•22 years ago
|
||
For the record, the bitrot was caused by the enabling of cmd_fontSize. I removed the lines that would have removed cmd_fontSize thus reenabling it.
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•