Closed Bug 180814 Opened 22 years ago Closed 22 years ago

nsIControllers::InsertControllerAt() method should start numbering controller IDs with 1, not 0

Categories

(Core :: DOM: Editor, defect)

All
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.3alpha

People

(Reporter: cmanske, Assigned: cmanske)

Details

Attachments

(1 file)

nsIControllers::AppendController() was fixed recently so the ID assigned to 
each new controller should start with 1. InsertControllerAt() should do the 
same. This is so we can trust that "0" for a cached controller ID means we haven't 
created a controller yet.
Attached patch Fix v1Splinter Review
This includes fixes to controller code in editor that relies on the controller 

ID.
Comment on attachment 106725 [details] [diff] [review]
Fix v1

This fix was originally included in another bug and brade commented:
I don't understand this block:
+  // Don't need to do this if already done
+  if (gComposerWindowControllerID)
+    return;
Why isn't there a similar optimization for gComposerJSCommandControllerID?

cmanske: Once we create a controller and get its ID, it must be non-zero so we
can use the ID to test for creation. There is the same optimization for
gComposerJSCommandControllerID, the line:
+  if (gComposerJSCommandControllerID)

tests if that controller was already created and uses getControllerById instead
of creating a new controller.
Attachment #106725 - Flags: superreview?(sfraser)
Attachment #106725 - Flags: review?(brade)
Status: NEW → ASSIGNED
Keywords: patch, review
Whiteboard: [FIX IN HAND]
Target Milestone: --- → mozilla1.3alpha
Comment on attachment 106725 [details] [diff] [review]
Fix v1

+  if (gComposerWindowControllerID)

Would prefer 

+  if (gComposerWindowControllerID > 0)
Attachment #106725 - Flags: superreview?(sfraser) → superreview+
Comment on attachment 106725 [details] [diff] [review]
Fix v1

probably this should be covered elsewhere, but I find it confusing to have such
similarly named variables:
  gComposerJSCommandControllerID
  gComposerWindowControllerID

Is there a comment that explains what each is for?

r=brade
Attachment #106725 - Flags: review?(brade) → review+
checked into 1.3a trunk.
(I'll take care of controller ID comments elsewhere)
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: