Clean up nsIEditorSpellCheck.idl

NEW
Unassigned

Status

()

13 years ago
9 years ago

People

(Reporter: timeless, Unassigned)

Tracking

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

13 years ago
Hi brett. this bug is not optional as you did not get an editor sign off and no sr nor editor peer should ever have granted any mark for your changes.

deadline is 1.9a. if you can't arrange for this to be done, i'll find an editor owner to agree to revert your changes.

In short, you faield to get a module owner/peer review for editor on your changes.

The editor interface changes are unacceptable and the superreviewer should have blocked all of them.
 
At the very least things should have proper idl case.

converting a property (inlinespellchecker) into a function with a maybe init is uncool. instead add a new property that indicates whether accessing the property will create it as needed.
Er... timeless, did you read the mail I sent about this?

1) Brett DID NOT CHANGE THE CASE OF ANY IDL METHODS OR PROPERTIES IN EDITOR.
2) The CanSpellCheck method that was added was uppercase simply because every
   single other method in that interface is.  If you want that interface cleaned
   up, that's not really brett's job.
3) Your proposal for the inline spell checker property doesn't handle the case
   we need to handle here -- getting the existing spellchecker without creating
   it if it hasn't been created yet.

Maybe I'm missing something, but what are the exact review comments on https://bugzilla.mozilla.org/attachment.cgi?id=201876 from you as an editor peer?  Please quote the parts of that patch that you're commenting on?

Comment 2

13 years ago
I haven't heard of any specific regressions other than the title of this bug, and I'll certainly fix anything that is broken. Timeless, could you please be more specific?

> 1) Brett DID NOT CHANGE THE CASE OF ANY IDL METHODS OR PROPERTIES IN EDITOR.

I did change one: inlineSpellChecker. I thought I had the proper signoffs. All my new IDL has the proper formatting (as far as I know) as bz pointed out.

WRT the lazy creation, some code assumes that it will always autocreate. I added some code that wants no auto create because creation can be very expensive. It seems dumb to say "SetAutoCreate(false); GetSpellchecker(blah); SetAutoCreate(true);" It also seems dumb to add a new attribute MaybeGetInlineSpellchecker. Is there some consensus that this should work differently?
(Reporter)

Comment 3

13 years ago
he added nsIEditorSpellCheck.CanSpellCheck

Comment 5

13 years ago
Heh, the first checkin of the idl file (with leading uppercase and methods rather than attributes) is mine, back in 1999.


Severity: blocker → normal
Keywords: regression
OS: Windows XP → All
Hardware: PC → All
Summary: Undo idl regressions to editor module → Clean up nsIEditorSpellCheck.idl

Comment 6

13 years ago
I'm closing this bug in lieu of specific regressions or suggestions. If you find any actual problems or regressions with this feature, feel free to assign them to me.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → WONTFIX

Comment 7

13 years ago
This should not be closed.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Updated

13 years ago
Assignee: brettw → mozeditor
Status: REOPENED → NEW
QA Contact: timeless
(Reporter)

Comment 8

13 years ago
so, bz and i actually came up w/ an approach a couple of days ago.
QA Contact: timeless
QA Contact: timeless → editor
Assignee: mozeditor → nobody
You need to log in before you can comment on or make changes to this bug.