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)

x86
Windows 2000
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: timeless, Assigned: neil)

Details

Attachments

(2 files, 3 obsolete files)

> 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.
Attached patch broken (obsolete) — Splinter Review
Attachment #102168 - Attachment description: patch → broken
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);
Attached patch second attemptSplinter Review
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 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+
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.
Attached patch Whoops, typo :-) (obsolete) — Splinter Review
Another attack would be to use instanceof to save using try/catch.
Attachment #102502 - Attachment is obsolete: true
Comment on attachment 103058 [details] [diff] [review]
Whoops, typo :-)

r=brade
Attachment #103058 - Flags: review+
.
Assignee: timeless → neil
Attachment #103058 - Attachment is obsolete: true
Attachment #103859 - Flags: superreview?(kin)
Attachment #103859 - Flags: review?(brade)
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 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+
Attachment #103859 - Flags: superreview?(kin) → superreview?(alecf)
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+
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
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.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: