Comment 3

As a google user I'm getting hit hard by this bug. I have three accounts with google: My GMail, my hosted domain mail, and my adsense. I log into my GMail at this URL: https://gmail.google.com/gmail I log into my hosted domain mail at this URL: https://mail.google.com/hosted/example.com/ And I log into my Adsense here: https://www.google.com/adsense/ Camino is not capable of remembering more than one of those passwords at once. Worse, every time I log into one of them, it asks me if I want to update the keychain entry with the "new values", or keep the old values. So not only does it not remember all 3 of my passwords, it annoyingly pesters me as to which login I want it to remember. Just remember all of them! The thing that really gets me is that all of these logins are not just at different URLs, they're also different domains (gmail.google.com vs mail.google.com vs. www.google.com), and Camino still screws it up. Camino crams all of these into "www.google.com" in keychain. I read through all the comments in this bug and I can kind of understand the problem, but if Safari can do it properly I don't see why camino can't. Please fix this!

Pascal

Comment 46

•

19 years ago

I really appreciate Camino as it is my default browser, but guys, this really annoying bug/lack has been there for 4 years (sic!!!); please don't tell me you postponed it to 1.2!!! As a website developer I have tons of websites at 127.0.0.1; you can imagine what a pain it is to always have to type the user/pass combo. Especially when you know every other browser supports this...

Stuart Morgan

Comment 47

•

19 years ago

Please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html before commenting in this (or any other) bug. The current behavior and the reasons it is undesirable are well understood. Unless you are working on this bug or have new information to add, commenting here is not helpful.

Smokey Ardisson (offline for a while; not following bugs - do not email)

Updated

•

19 years ago

Assignee: mikepinkerton → nobody

QA Contact: chrispetersen → os.integration

Summary: Keychain can not save multiple passwords for the same host, different urls → Keychain cannot save multiple passwords for the same host, different urls

C Foss

Comment 48

•

19 years ago

Is there a chance this bug can be escalated in terms of priority and/or severity. This is a major annoyance for those people who come across it and its 4+ years(!) old. I think most people here would agree this is more important to get working than a lot of the more recent convenience type features. If you agree make sure you vote this up.

Stuart Morgan

Comment 49

•

19 years ago

Let's try this again: read comment 47 before commenting in this bug. As the person doing precursor work to this bug and the person most likely to implement this, I can guarantee that wasting my (and other developers') time by making pointless comments in this bug will absolutely not cause this bug to be fixed any sooner.

Stuart Morgan

Updated

•

19 years ago

No longer blocks: 202337

David Rock

•

19 years ago

I'm back for round two! I'm going to take a crack at this feature although I was a bit off the mark when I thought I could do it over a weekend. I should have a patch very soon but before that I want to give an overview of how it's going to work. Some background on how Firefox does password form fill, it uses 4 components: * nsPasswordManager.cpp - for password storage and password auto fill * XUL popup - widget that displays popup below text inputs * nsAutoCompleteController.cpp - handles autocompletion tasks like search * nsFormFillController.cpp - handles UI, events, and coordination between above components The tragedy is that Camino can't use any of this. The password code uses a non-Keychain encrypted file. The other pieces require compiling in a lot of extra XUL code since they are in mozilla/toolkit. So I rewrote the above in Cocoa and stripped out a lot of code that we don't need. Some highlights of the Camino Form Fill solution: * Adds a Cocoa class file FormFillController.mm - Combines Firefox's AutoCompleteController and FormFillController - Uses autocomplete functions from mozilla/xpfe like Camino's AutoCompleteTextField instead of the one from mozilla/toolkit like Firefox - By default, integrated with KeychainService but can easily be extended to also handle generic form history completion if someone writes that. - Does complete-as-you-type like Safari, which is more user-friendly than Firefox * Adds a Cocoa class file FormFillPopup.mm - UI Widget to hold form fill options in a popup menu below the text input - Each BrowserWrapper window has its own popup (like it has its own ToolTip window) * Adds functionality to KeychainService.mm - Allows multiple usernames for a single domain - Handles searching of keychain usernames for autocomplete matching - Remembers the default (last used) password for a site like Safari does. Questions, comments, proclamations, and sarcasm are welcome. More to come soon!

Daryl Kulak

•

19 years ago

Attached file First patch w/additional file (obsolete) — Details

oops, I forgot to add a file (BrowserWrapper.h) to the patch

Attachment #257939 - Attachment is obsolete: true

Bryan Atwood

Assignee

Comment 57

•

•

19 years ago

(In reply to comment #52) > - Uses autocomplete functions from mozilla/xpfe like Camino's > AutoCompleteTextField instead of the one from mozilla/toolkit like Firefox Is there something critical/compelling for using xpfe autocomplete stuff (or are you just re-using stuff we're already using elsewhere in Camino instead of adding new deps on that code), since at the end of the day our goal is to stop using Mozilla autocomplete code (see bug 340611)? Obviously if xpfe stuff is the only (sane) way to do it now short of completely fixing bug 340611, adding more links to the xpfe stuff is OK, but I wanted to make sure you were aware of that other bug....

Bryan Atwood

Assignee

Comment 64

•

19 years ago

> Is there something critical/compelling for using xpfe autocomplete stuff (or > are you just re-using stuff we're already using elsewhere in Camino instead of > adding new deps on that code) No reason to use xpfe, just wanted to keep it the same as the address bar code. I agree we need to change it as bug 340611 states. We can update the address bar and form fill once we have a cocoa version.

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 65

•

19 years ago

It's likely out-of-scope for here, but we should think about things like bug 376668 (and file a follow-up once we actually have support for multiple accounts), because the Safari/Firefox way of "autofilling" with multiple accounts is utterly undiscoverable in my experience.

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 66

•

18 years ago

Setting priority per 1.6 roadmap.

Priority: -- → P3

Bryan Atwood

Assignee

Comment 67

•

18 years ago

OK, I'm uploading the patch for this and the four additional files (FormFillController.h/mm, FormFillPopup.h/mm) that should probably go in camino/src/formfill. Stuart, thanks for agreeing to review the Keychain code. I have a lot of questions for you :) There are a few things that still need to be worked out: 1. If you scroll the window when the username popup list is open, the page scrolls but the popup doesn't. In Firefox, the formfill popup listens for "rollup" events, but these don't exist in Camino. So we can just have the popup window listen for Cocoa scroll notifications, or implement the "rollup" events farther up the chain like the ToolTip close events. 2. Alignment of the popup window could be better. Thoughts? 3. The Keychain code was modified to set the comment to 'default' like Safari to choose the default user/pass for the page. 4. Do we still need kCaminoKeychainCreatorCode? 5. Just a thought, but maybe KeychainBrowserListener should be modified so that it doesn't implement CHBrowserListener methods and just directly listens for the events that it needs. If so, we can combine it with the new class KeychainAutoFillListener.

Bryan Atwood

Assignee

Comment 68

•

18 years ago

Attached patch First patch for review (obsolete) — Details — Splinter Review

Attachment #257942 - Attachment is obsolete: true

Attachment #277352 - Flags: review?(stuart.morgan)

Bryan Atwood

Assignee

Comment 69

•

18 years ago

•

18 years ago

Comment on attachment 286943 [details] [diff] [review] Patch for #72,73 that adds missing files This is looking great! My comments are mostly nits at this point; there are a few architectural notes but I think they are all fairly restricted to small areas. + * The Original Code is mozilla.org code. We've been using "Camino code" for new Camino files. +#import "FormFillPopup.h" +#import "AutoCompleteUtils.h" +#import "KeychainService.h" + +#include "nsIDOMHTMLInputElement.h" Do these really all need to be included in the header, as opposed to just forward class declarations? +@interface FormFillController : NSObject <AutoCompleteListener> +{ + NSMutableArray* mBrowserViews; + NSMutableArray* mPopups; + + KeychainAutoCompleteSession* mKeychainSession; + AutoCompleteResults* mResults; + FormFillListener* mListener; + + FormFillPopup* mFocusedPopup; + CHBrowserView* mFocusedView; + nsIDOMHTMLInputElement* mFocusedInput; These all need strong/weak annotation. +static FormFillController *sInstance = nil; + ++ (FormFillController*)instance +{ + return sInstance ? sInstance : sInstance = [[self alloc] init]; +} Why is this a singleton tracking a bunch of browsers, rather than having an instance per browser? It doesn't appear to be controlling a single-access resource. +// listeners //////////////////////////// Use #pragma mark for grouping methods. + if (status == kAutoCompleteMatchFound) { + // Make sure results array has only NSString. + // Items must be NSString objects. + resultsValid = true; + NSEnumerator* itemsEnumerator = [[results items] objectEnumerator]; + id item; + while ((item = [itemsEnumerator nextObject])) { + if (![item isKindOfClass:[NSString class]]) + resultsValid = false; + } + } There are tabs in this code. More importantly though, what's the expected case where our own autocomplete implementation would give us the wrong type of results? We should be able to trust the basic contract of objects we interact with, and any username autocompletion system should presumably be giving us only usernames. +- (BOOL)isPopupOpen +{ + return (mFocusedPopup) ? [mFocusedPopup isPopupOpen] : FALSE; +} BOOL values are YES and NO; there are several instances in the patch that need to be fixed. + // Set the lower left corner of the input element as the upper left corner + // of the popup and the width of the element as the width of the popup + NSPoint point = NSMakePoint(inputRect.x, inputRect.y + inputRect.height); + [mFocusedPopup openPopup:mFocusedView orig:point width:inputRect.width]; What if the input is right at the bottom of the screen? I'd suggest passing in the entire rect and letting the popup class decide how to best position itself accordingly (you don't necessarily need to handle all positioning cases now, a TODO is fine; I'd just like an API that puts the logic in the right place and has all the information we'll need). +- (void)selectRowBy:(int)aRows shiftRowSelectionBy: ? + // Send an autocomplete DOM event any time auto fill is done + // For example, a corresponding password element could be filled Remove the second line; no need to comment on what the implementation of a different method might want to do. + if (mUsernameFillEnabled) + [mKeychainSession onStartLookup:searchString + previousResults:mResults + listener:self]; Use {} for multi-line statements in an |if| + if ([mFocusedPopup rowCount] > 1 && mCompleteResult) { + [self openPopup]; + } + else + [self closePopup]; You can borrow the braces from here ;) +// XXX Put comment here +- (void)doAutoComplete Sounds like good advice! I'm not a fan of "do" as a method name prefix, since it doesn't really add anything. How about autoCompleteFieldText (to make it clear it's talking about the text itself, not the popup)? + // Select the default if it's available + // Otherwise, select the first username in the list + int defaultIndex = [mResults defaultIndex]; + if (defaultIndex < 0) + defaultIndex = 0; If there's no specific default, should the autocomplete itself set the default to 0 rather than -1 so all the clients don't have to do the same fixup? + // Auto-fill the input like Safari where untyped letters are selected This is the right behavior regardless of what Safari is doing (and may do in the future); let's not define the behavior in terms of something we don't control. +// Called whenever the form element is filled. It calls a DOM event s/calls/sends/ ? + if (!input) + return; + + nsAutoString type; + input->GetType(type); + + PRBool isReadOnly = PR_FALSE; + input->GetReadOnly(&isReadOnly); You may as well do early return at each of these steps if the answer isn't what we are looking for. + nsAutoString autocomplete; + input->GetAttribute(NS_LITERAL_STRING("autocomplete"), autocomplete); + if (type.LowerCaseEqualsLiteral("text") && !isReadOnly && + !autocomplete.LowerCaseEqualsLiteral("off")) { We have chosen to support wallet.crypto.autocompleteoverride, so that needs to be honored here and in the form check. +// FormFillDataSource +// +// Manages the data returned by the FormFillController +// +@interface FormFillDataSource : NSObject +{ + NSArray* mItems; +} +- (int)rowCount; +- (id)resultForRow:(int)aRow columnIdentifier:(NSString *)aColumnIdentifier; + +- (void)setItems:(NSArray*)items; +- (NSArray*)items; Since the FormFillPopup ends up implementing the datasource methods anyway, and it's a very simple source, it's probably best to just make FormFillPopup implement the necessary methods directly as a combination model/controller. +// Manages the display of hte popup window, receiving data from the FormFillController +// and storing it in a FormFillDataSource. s/hte/the/. Also, don't give implementation details like the storage layer in the overview comment, just purpose. + NSWindow* mPopupWin; + NSTableView* mTableView; + + FormFillDataSource* mDataSource; Needs strong/weak; >+ if ((self = [super init])) >+ { Same line. There are a number of these in the patch that all need correcting >+ // construct and configure the popup window >+ mPopupWin = [[NSWindow alloc] initWithContentRect:NSMakeRect(0,0,0,0) >+ styleMask:NSBorderlessWindowMask >+ backing:NSBackingStoreBuffered >+ defer: NO]; s/: NO/:NO/ >+ [mPopupWin setReleasedWhenClosed:NO]; >+ [mPopupWin setHasShadow:YES]; >+ [mPopupWin setAlphaValue:0.9]; >+ >+ // construct and configure the view >... Would it be worthwhile to make a nib for this instead? (I'm fine with it either way) >+ // XXX check shift here, still not aligned sometimes >+ // shift popup to the right to account for text box border >+ point.x += 2.0; Do you mean the page's text field? If so, this needs to be figured out by the caller and accounted for there, because it will depend on the styling of the page. >+ // XXX check width change here, still not aligned sometimes >+ // adjust width to account for text box border Same here; having the caller pass in the inner rect to match is probably the best approach. >+ // XXX Figure out to close popup when window moves or is scrolled by mouse >+ // This should ideally be done elsewhere with a Rollup listener The scrolling definitely needs to be handled elsewhere, so the XXX should go there (I'd be fine with checking this in without that fixed and filing a follow-up bug). I don't think we want the popup to close when the window is moved though, since it's a child window and should follow correctly. >+ [[NSNotificationCenter defaultCenter] addObserver:self >+ selector:@selector(onResize:) >+ name:NSWindowDidResizeNotification >+ object:nil]; object:nil means this will trigger on resizing of any NSWindow at all, which is probably not what you want. >+// XXX - see comment above >+- (void)onResize:(NSNotification *)aNote I'm not sure which comment that refers to, so please just restate whatever needs to be said here. >-- (void)promptToStoreLogin:(NSDictionary*)loginInfo inWindow:(NSWindow*)window; >+- (void)promptToStoreLogin:(NSDictionary*)loginInfo updateEntry:(BOOL)update inWindow:(NSWindow*)window; > - (void)promptToUpdateKeychainItem:(KeychainItem*)keychainItem > withUsername:(NSString*)username > password:(NSString*)password We really shouldn't have both of these; it looks like you removed all uses of promptToUpdateKeychainItem..., but not the method itself. However, I don't see how your method would work without regressing bug 177941 in the case of a changed password, since it doesn't use the cache at all. Combining storage and update is fine (and may well simplify a lot of things), but any combined system should try to use the cache for those cases where it's useful. I would suggest passing the new login information and any cache to a single function that looks at the username of the new account info, the username of the cached entry if any, searches the keychain if necessary, and then calls either the update prompt method (passing in the appropriate keychain item that it found for an update to avoid a re-authorization, and with the crufty part of the update method removed) or the store prompt method. HTTP auth will need a different method to make determinations, since it still has the old behavior. As a caveat, I haven't dug deeply back into the interplay of the various UI and storage/update methods, so that may have issues I overlooked. I'll take some time soon to look more thoroughly at how a refactoring like that would work and get back to you. >+ // Pointers to the login elements so that the keychain and form don't need to >+ // be searched for the same element >+ nsCOMPtr<nsIDOMHTMLInputElement> mUsernameElement; >+ nsCOMPtr<nsIDOMHTMLInputElement> mPasswordElement; What does the comment mean? Searching the keychain for an HTML element doesn't make sense, but that's how it parses to me. >+ KeychainAutoCompleteDOMListener* mDOMListener; >... >+ nsIDOMHTMLInputElement* mUsernameElement; Strong or weak? >+nsresult >+getHostForElement(nsIDOMHTMLInputElement* aElement, >+ NSString** host, >+ PRInt32* port, >+ NSString** scheme); s/get/Get/, but since it gets three different things, it needs a new name as well (GetFormInfoForInput, maybe?) >+ // If a username is passed in, discard items with different usernames >+ if (username) { >+ NSMutableArray* matchingItems = [NSMutableArray array]; >+ KeychainItem* item; >+ NSEnumerator* keychainEnumerator = [newKeychainItems objectEnumerator]; >+ while ((item = [keychainEnumerator nextObject])) { >+ if ([[item username] isEqualToString:username]) >+ [matchingItems addObject:item]; >+ } >+ newKeychainItems = matchingItems; >+ } I was only doing post-search filtering in the cases where there could be legitimate matching entries with no value, which shouldn't be the case for username, so this part should be pushed into the keychain search up front. >+// KeychainAutoCompleteSession Implementation >+@implementation KeychainAutoCompleteSession I'd rather this class be in a new file; there's already way more stuff in KeychainService than there should be (which I intend to fix at some point). >+ NSArray* keychainEntries = [KeychainItem allKeychainItemsForHost:host >+ port:(UInt16)port >+ protocol:protocol >+ authenticationType:kSecAuthenticationTypeHTMLForm >+ creator:0]; >+ >+ NSEnumerator* keychainEnumerator = [keychainEntries objectEnumerator]; >+ KeychainItem* item; >+ while ((item = [keychainEnumerator nextObject])) { >+ [mUsernames addObject:[item username]]; >+ } >+ >+ // Get the default keychain and save the username >+ item = [keychain findKeychainEntryForHost:host >+ username:nil >+ port:(UInt16)port >+ scheme:scheme >+ securityDomain:nil >+ isForm:YES]; Since you already have all the kechain entries for the host, why not search them directly for the default, rather than going back to the keychain? >+ // Make sure that the items in the array are NSString objects before using them >+ NSEnumerator* prevResultEnumerator = [[previousSearchResults items] objectEnumerator]; >+ id item; >+ while ((item = [prevResultEnumerator nextObject])) { >+ if (![item isKindOfClass:[NSString class]]) >+ resultsValid = false; Again, this shouldn't be necessary. >+ if (searchPrevious) { >+ // Search through the previous results. >+ NSEnumerator* prevResultEnumerator = [[previousSearchResults items] objectEnumerator]; >+ NSString* username; >+ while ((username = [prevResultEnumerator nextObject])) { >+ if ([username hasPrefix:searchString]) { >+ [resultItems addObject:username]; >+ >+ // Check for the default >+ if ([username isEqualToString:mDefaultUser]) >+ [results setDefaultIndex:([resultItems count]-1)]; >+ } >+ } >+ } >+ else { >... There's quite a bit of duplication in the two clauses. Just assign the things to search to a local array based on the if/else, then have the search code once. >+#ifndef __AutoCompleteUtils_h__ >+#define __AutoCompleteUtils_h__ We don't generally do this in ObjC headers, since #import makes it unnecessary. >+// AutoCompleteResults >+// >+// Container object for generic auto complete (password, form history, url history). >+// Holds the search string, array of matched objects and the default item. Since it's intended to be generic, no examples in the description. >+ NSArray* mItems; >... >+- (NSArray*)items; >+- (void)setItems:(NSArray*)items; "items" isn't a descriptive enough name. I'd suggest "matches". >+typedef enum >+{ >+ kAutoCompleteFailed = -1, >+ kAutoCompleteNoMatch = 0, >+ kAutoCompleteMatchFound = 1, >+ kAutoCompleteIgnored = 2 >+} AutoCompleteResultStatus; kAutoCompleteIgnored is never used (or explained) and the difference between kAutoCompleteNoMatch and kAutoCompleteMatchFound is already captured in the number of items returned in the array, so it seems like this enum could be removed and replaced with a BOOL indicating success. >+@protocol AutoCompleteListener >+- (void)onAutoComplete:(AutoCompleteResults*)results >+ status:(AutoCompleteResultStatus)status; >+@end It's not clear from the name alone whether this would be called to start autocomplete, or when it's done, so how about something like: - (void)autoCompleteFoundResults:(AutoCompleteResults*)results withSuccess:(BOOL)success Although going a step further, do we expect that any callers would realistically handle "success with no results" differently than "failed to search"? I would think not, so I'd lean toward removing the second parameter entirely; we could always go back and add it if we really needed it later. >+// An AutoCompleteSession object listens for search requests (eg. from the form fill >+// controller) and // searches a set of data (eg. keychain items). |onStartLookup| >+// initiates the// process. Previous results are passed in as well as the listener >+// object for when the search is complete. There are some embedded comment markers here. >+@protocol AutoCompleteSession >+- (void)onStartLookup:(NSString*)searchString >+ previousResults:(AutoCompleteResults*)previousSearchResults >+ listener:(id<AutoCompleteListener>)listener; This isn't really a notification, but rather a request for a search to begin, so name it something like startAutoCompleteWithSearch:... >+ mPopup = [[FormFillPopup alloc] init]; >+ >+ [[FormFillController instance] attachToBrowser:mBrowserView withPopup:mPopup]; Given that this will be relatively infrequently used, what about having the FormFillController created the popup window itself, lazily? (The close-on-deactive could be done as a passthrough on the controller.) >+ /* Return the screen location (nsIntRect) of a DOM Element */ >+ static void GetScreenOrigin(nsIDOMElement* aElement, nsIntRect* aRect); This needs to be called something like GetOriginInScreenCoordinates, since the screen origin is something very different. Also, don't use "Return" in the doc comment, since it doesn't actually return anything.

Attachment #286943 - Flags: review?(stuart.morgan) → review-

Bryan Atwood

Assignee

Comment 78

•

18 years ago

(In reply to comment #77) Thank you Stuart! This is excellent feedback and I appreciate you looking at it so closely. I have two of your changes left to make, but wanted to clarify one thing before I uploaded another big patch. > >+ // If a username is passed in, discard items with different usernames > >+ if (username) { > >+ NSMutableArray* matchingItems = [NSMutableArray array]; > >+ KeychainItem* item; > >+ NSEnumerator* keychainEnumerator = [newKeychainItems objectEnumerator]; > >+ while ((item = [keychainEnumerator nextObject])) { > >+ if ([[item username] isEqualToString:username]) > >+ [matchingItems addObject:item]; > >+ } > >+ newKeychainItems = matchingItems; > >+ } > > I was only doing post-search filtering in the cases where there could be > legitimate matching entries with no value, which shouldn't be the case for > username, so this part should be pushed into the keychain search up front. Does this mean that we should move the logic for returning the Keychain for a specific username into KeychainItem itself? (Sorry, when you said "no value", I wasn't sure if you meant "empty value" or "worthless")

Stuart Morgan

Comment 79

•

18 years ago

(In reply to comment #78) > Does this mean that we should move the logic for returning the Keychain for a > specific username into KeychainItem itself? (Sorry, when you said "no value", > I wasn't sure if you meant "empty value" or "worthless") That's correct, yes. Sorry for the muddled comment; "no value" meaning empty (a concrete case being: a keychain item without a port set is a valid match for a form on a site served from port 80), and "up front" meaning in KeychainItem.

Bryan Atwood

Assignee

Comment 80

•

18 years ago

Attached patch Patch for comments in #77 (obsolete) — Details — Splinter Review

This patch address Stuart's comments in #77. I'll make more detailed comments in a follow-up post.

Attachment #286943 - Attachment is obsolete: true

Attachment #291056 - Flags: review?(stuart.morgan)

Bryan Atwood

Assignee

Comment 81

•

18 years ago

(In reply to comment #77) OK, making some progress. Before I get to your comments, some quick notes. Added: - Now caches the keychain when a password is filled after selecting a row in the username popup - Username popup closes when window is resized by adding NSWindowDidResizeNotification observer - Username popup moves with window moves To Do: - Username popup should close when window is scrolled Questions: - When submitting a form, the next page loads while "save password" confirmation sheet shows. In 1.5, "save password" sheet must be closed before next page loads. Is this an issue with the trunk, or is it new behavior? - In this patch, FindPasswordField() and FindUsernamePasswordFields() are moved to KeychainService.mm to KeychainService.h. Any problems with that? - How to handle securityDomain in findKeychainEntryForHost where username is specified? In this patch, it is ignored. I'll respond to some of your suggestions below. To save writing a huge post, if I don't mention a change it means that I added it to the patch. Would be happy to re-post and add "Done" below each comment if that's useful. > +static FormFillController *sInstance = nil; > + > ++ (FormFillController*)instance > +{ > + return sInstance ? sInstance : sInstance = [[self alloc] init]; > +} > > Why is this a singleton tracking a bunch of browsers, rather than having an > instance per browser? It doesn't appear to be controlling a single-access > resource. Mainly because that's how it was set up when I cribbed the architecture from Firefox :) OK, moved to separate instance per browser. > +// listeners //////////////////////////// > > Use #pragma mark for grouping methods. Removed completely > + // Set the lower left corner of the input element as the upper left corner > + // of the popup and the width of the element as the width of the popup > + NSPoint point = NSMakePoint(inputRect.x, inputRect.y + inputRect.height); > + [mFocusedPopup openPopup:mFocusedView orig:point width:inputRect.width]; > > What if the input is right at the bottom of the screen? I'd suggest passing in > the entire rect and letting the popup class decide how to best position itself > accordingly (you don't necessarily need to handle all positioning cases now, a > TODO is fine; I'd just like an API that puts the logic in the right place and > has all the information we'll need). Hmm. I think that the popup window itself shouldn't need to know about it's relative position. That's what the job of the controller seems to be. Otherwise we need to move a lot of screen positioning code into the popup window class itself and so far it only handles its local display. I can make your changes if you let me know though. > +// FormFillDataSource > +// > +// Manages the data returned by the FormFillController > +// > +@interface FormFillDataSource : NSObject > +{ > + NSArray* mItems; > +} > +- (int)rowCount; > +- (id)resultForRow:(int)aRow columnIdentifier:(NSString *)aColumnIdentifier; > + > +- (void)setItems:(NSArray*)items; > +- (NSArray*)items; > > Since the FormFillPopup ends up implementing the datasource methods anyway, and > it's a very simple source, it's probably best to just make FormFillPopup > implement the necessary methods directly as a combination model/controller. Good idea, done. > >+ [mPopupWin setReleasedWhenClosed:NO]; > >+ [mPopupWin setHasShadow:YES]; > >+ [mPopupWin setAlphaValue:0.9]; > >+ > >+ // construct and configure the view > >... > > Would it be worthwhile to make a nib for this instead? (I'm fine with it either > way) I'll leave it this way for now, but we can change it later if needed. > >+ // XXX Figure out to close popup when window moves or is scrolled by mouse > >+ // This should ideally be done elsewhere with a Rollup listener > > The scrolling definitely needs to be handled elsewhere, so the XXX should go > there (I'd be fine with checking this in without that fixed and filing a > follow-up bug). I don't think we want the popup to close when the window is > moved though, since it's a child window and should follow correctly. > > >+ [[NSNotificationCenter defaultCenter] addObserver:self > >+ selector:@selector(onResize:) > >+ name:NSWindowDidResizeNotification > >+ object:nil]; > > object:nil means this will trigger on resizing of any NSWindow at all, which is > probably not what you want. > >+// XXX - see comment above > >+- (void)onResize:(NSNotification *)aNote > > I'm not sure which comment that refers to, so please just restate whatever > needs to be said here. Popup now stays open and moves along with window moves. I modified the NSWindowDidResizeNotification to work better and now the popup closes with resize. I'll open a bug for closing the popup on window scroll. I think it may require some deep changes in embed code to get a scroll to trigger a notification or event. > >-- (void)promptToStoreLogin:(NSDictionary*)loginInfo inWindow:(NSWindow*)window; > >+- (void)promptToStoreLogin:(NSDictionary*)loginInfo updateEntry:(BOOL)update inWindow:(NSWindow*)window; > > - (void)promptToUpdateKeychainItem:(KeychainItem*)keychainItem > > withUsername:(NSString*)username > > password:(NSString*)password > > We really shouldn't have both of these; it looks like you removed all uses of > promptToUpdateKeychainItem..., but not the method itself. However, I don't see > how your method would work without regressing bug 177941 in the case of a > changed password, since it doesn't use the cache at all. Combining storage and > update is fine (and may well simplify a lot of things), but any combined system > should try to use the cache for those cases where it's useful. > > I would suggest passing the new login information and any cache to a single > function that looks at the username of the new account info, the username of > the cached entry if any, searches the keychain if necessary, and then calls > either the update prompt method (passing in the appropriate keychain item that > it found for an update to avoid a re-authorization, and with the crufty part of > the update method removed) or the store prompt method. HTTP auth will need a > different method to make determinations, since it still has the old behavior. > As a caveat, I haven't dug deeply back into the interplay of the various UI and > storage/update methods, so that may have issues I overlooked. I'll take some > time soon to look more thoroughly at how a refactoring like that would work and > get back to you. Hm.. What you are describing is similar to the current (1.5) functionality and I prefer it over the patch. I've reverted my code to the original, with separate update and store methods, and to use the cache. This is another case of "we can add it later if we need". Please take a look at updateKeychainEntry in KeychainService. It's been simplified to not care whether the entry was from Camino or Safari. In other words, keychains added from Safari will show up in Camino, and vice-versa. > >+ // If a username is passed in, discard items with different usernames > >+ if (username) { > >+ NSMutableArray* matchingItems = [NSMutableArray array]; > >+ KeychainItem* item; > >+ NSEnumerator* keychainEnumerator = [newKeychainItems objectEnumerator]; > >+ while ((item = [keychainEnumerator nextObject])) { > >+ if ([[item username] isEqualToString:username]) > >+ [matchingItems addObject:item]; > >+ } > >+ newKeychainItems = matchingItems; > >+ } > > I was only doing post-search filtering in the cases where there could be > legitimate matching entries with no value, which shouldn't be the case for > username, so this part should be pushed into the keychain search up front. OK, updated the method in KeychainItem to accept a username. > >+// KeychainAutoCompleteSession Implementation > >+@implementation KeychainAutoCompleteSession > > I'd rather this class be in a new file; there's already way more stuff in > KeychainService than there should be (which I intend to fix at some point). OK, this was split into a new file. This probably needs closer review to make sure each object handles only the things it needs to. > >+ NSArray* keychainEntries = [KeychainItem allKeychainItemsForHost:host > >+ port:(UInt16)port > >+ protocol:protocol > >+ authenticationType:kSecAuthenticationTypeHTMLForm > >+ creator:0]; > >+ > >+ NSEnumerator* keychainEnumerator = [keychainEntries objectEnumerator]; > >+ KeychainItem* item; > >+ while ((item = [keychainEnumerator nextObject])) { > >+ [mUsernames addObject:[item username]]; > >+ } > >+ > >+ // Get the default keychain and save the username > >+ item = [keychain findKeychainEntryForHost:host > >+ username:nil > >+ port:(UInt16)port > >+ scheme:scheme > >+ securityDomain:nil > >+ isForm:YES]; > > Since you already have all the kechain entries for the host, why not search > them directly for the default, rather than going back to the keychain? This was one that I didn't change. findKeychainEntryForHost returns the default entry. The logic to choose the default (first check "camino_default" comment, then "default" comment, then creator code, etc.) is somewhat gnarly so I didn't want to have it in two places. Also, you'll notice that I grabbed all entries without filtering for securityDomain. Let me know if that's the correct behavior. Maybe in attachToInput: I need to get the actionHost if there is one? > >+typedef enum > >+{ > >+ kAutoCompleteFailed = -1, > >+ kAutoCompleteNoMatch = 0, > >+ kAutoCompleteMatchFound = 1, > >+ kAutoCompleteIgnored = 2 > >+} AutoCompleteResultStatus; > > kAutoCompleteIgnored is never used (or explained) and the difference between > kAutoCompleteNoMatch and kAutoCompleteMatchFound is already captured in the > number of items returned in the array, so it seems like this enum could be > removed and replaced with a BOOL indicating success. > > >+@protocol AutoCompleteListener > >+- (void)onAutoComplete:(AutoCompleteResults*)results > >+ status:(AutoCompleteResultStatus)status; > >+@end > > It's not clear from the name alone whether this would be called to start > autocomplete, or when it's done, so how about something like: > - (void)autoCompleteFoundResults:(AutoCompleteResults*)results > withSuccess:(BOOL)success > > Although going a step further, do we expect that any callers would > realistically handle "success with no results" differently than "failed to > search"? I would think not, so I'd lean toward removing the second parameter > entirely; we could always go back and add it if we really needed it later. OK, I've removed AutoCompleteResultStatus completely, and return an empty AutoCompleteResults if there are no results or search failed. > >+ mPopup = [[FormFillPopup alloc] init]; > >+ > >+ [[FormFillController instance] attachToBrowser:mBrowserView withPopup:mPopup]; > > Given that this will be relatively infrequently used, what about having the > FormFillController created the popup window itself, lazily? (The > close-on-deactive could be done as a passthrough on the controller.) OK, popup is only created if form fill input field is controlled (keychain, history, etc. autofill listener is attached)

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 82

•

18 years ago

(In reply to comment #81) > - When submitting a form, the next page loads while "save password" > confirmation sheet shows. In 1.5, "save password" sheet must be closed before > next page loads. Is this an issue with the trunk, or is it new behavior? New behavior; see bug 369245.

Stuart Morgan

Comment 83

•

18 years ago

Comment on attachment 291056 [details] [diff] [review] Patch for comments in #77 A couple of quick things that prevent compiling: - s/getContentWindow/contentWindow/ (this changed since the patch was made) - KeychainAutoCompleteSession needs to be .mm in order to include NSString+Gecko.h - KeychainAutoCompleteSession.mm needs -DNO_NSPR_10_SUPPORT set as a build flag (you may well already have this locally) >Index: mozilla/camino/src/formfill/FormFillController.h >... >+ FormFillPopup* mPopupWindow; // strong This needs to be released in dealloc, since it's a strong ref. >+- (void)openPopup >... >+ GeckoUtils::GetOriginInScreenCoordinates(mFocusedInput, &inputRect); >+ >+ // Set the lower left corner of the input element as the upper left corner >+ // of the popup and the width of the element as the width of the popup >+ NSPoint point = NSMakePoint(inputRect.x, inputRect.y + inputRect.height); We'll need a follow-up bug about making this correct under resolution independence on trunk. >+// or keyboard movement. Like Safari, we want to set the form field to the >... >+ // Auto-fill the input like Safari where untyped letters are selected Again, we don't want to define behavior in terms of Safari. >Index: mozilla/camino/src/formfill/FormFillPopup.h >... >+// openPopup expects a point relative to the top left coordinate of the screen This should take standard Cocoa coordinates; it should be the caller's job to do the conversion to get that. >+- (void)openPopup:(CHBrowserView*)aBrowser orig:(NSPoint)point width:(float)width; openPopup:withOrigin:width: >+- (void)resizePopup >... + // The table view is resized automatically. Set the window frame + // to account for any table view height change + NSRect popupWinFrame = [mPopupWin frame]; + NSRect tableViewFrame = [mTableView frame]; + + popupWinFrame.origin.y += NSHeight(popupWinFrame) - NSHeight(tableViewFrame); + popupWinFrame.size.height = NSHeight(tableViewFrame); This will also need to be called out in the bug about making this code resolution-independence-safe on trunk, since it won't do what you expect. >Index: mozilla/camino/src/extensions/GeckoUtils.h >+ static void GetOriginInScreenCoordinates(nsIDOMElement* aElement, nsIntRect* aRect); Sorry, I missed that this was returning a rect when I suggested this name, so how about GetFrameInScreenCoordinates Index: mozilla/camino/src/formfill/KeychainAutoCompleteSession.h =================================================================== + // Pointers to the login elements so that the form doesn't need to + // be searched when the same input element is attached again + nsIDOMHTMLInputElement* mUsernameElement; + nsIDOMHTMLInputElement* mPasswordElement; Mark these as strong refs. Index: mozilla/camino/src/formfill/KeychainAutoCompleteSession.m =================================================================== +KeychainAutoCompleteDOMListener::SetElements(nsIDOMHTMLInputElement* usernameElement, + nsIDOMHTMLInputElement* passwordElement) +{ + mUsernameElement = usernameElement; + NS_IF_ADDREF(mUsernameElement); + mPasswordElement = passwordElement; + NS_IF_ADDREF(mPasswordElement); +} These need to NS_IF_RELEASE first, or consecutive calls will leak. >+ // Cache all of the usernames in the object so that the keychain >+ // doesn't have to be read again. This should be a faster way to search and sort. >+ SecProtocolType protocol = [scheme isEqualToString:@"https"] ? kSecProtocolTypeHTTPS : kSecProtocolTypeHTTP; >+ NSArray* keychainEntries = [KeychainItem allKeychainItemsForHost:host >+ port:(UInt16)port >+ protocol:protocol >+ authenticationType:kSecAuthenticationTypeHTMLForm >+ creator:0]; >+ >+... >+ >+ // Get the default keychain and save the username >+ item = [keychain findKeychainEntryForHost:host >+ port:(UInt16)port >+ scheme:scheme >+ securityDomain:nil >+ isForm:YES]; The duplication still bothers me here, and also having thought about it more the fact that the first call is bypassing KeychainService, which is there to abstract out things like the protocol, authentication type, and creator stuff. How about this: we add a new allWebFormKeychainItemsForHost:port KeychainService method to wrap your first call, and pull the defaults logic out into a new defaultFromKeychainItems: KeychainService method that you can pass that array into to get a default back. You can't use just kSecAuthenticationTypeHTMLForm for the find-all, as that will ignore all the old-style entries. The new KS method should find new and old, and merge them into one array. The find-all code (and the corresponding password lookup in FillPassword) also needs to be called twice--once punycoded and once not--with the results merged together, in the style of http://mxr.mozilla.org/mozilla/source/camino/src/formfill/KeychainService.mm#1248 Legacy keychain entries are the bane of our keychain code :( You'll also want to unique the usernames, since there are almost definitely people with duplicate account information (most likely old-style Camino, new-style Safari). (You don't have to worry about which one it is at password lookup time though; just always use the one you are currently getting, since they should be the same.) Index: mozilla/camino/src/formfill/KeychainService.mm =================================================================== >+ // First, check for the default Camino entry. >... >+ // Next, check for Safari's default >... >+ // Then check for a new-style Camino keychain entry >... >+ // If there isn't one, check for an old-style Camino entry This order will potentially break HTTP Auth lookup... > - (KeychainItem*)updateKeychainEntry:(KeychainItem*)keychainItem > withUsername:(NSString*)username > password:(NSString*)password > { >- if ([username isEqualToString:[keychainItem username]] || [keychainItem creator] == kCaminoKeychainCreatorCode) { >- [keychainItem setUsername:username password:password]; >- } >- else { >- keychainItem = [KeychainItem addKeychainItemForHost:[keychainItem host] >- port:[keychainItem port] >- protocol:[keychainItem protocol] >- authenticationType:[keychainItem authenticationType] >- withUsername:username >- password:password]; >- [keychainItem setCreator:kCaminoKeychainCreatorCode]; >- } >+ [keychainItem setUsername:username password:password]; ... and this will make it impossible to have different HTTP Auth information in Camino and Safari. We really need to split the different types apart now that the use is becoming so different. I'd suggest the following: - Split findKeychainEntryForHost:port:scheme:securityDomain:isForm: into two parts: findDefaultWebFormKeychainEntryForHost:port:scheme: which contains just the lookup and your new defaults check order (or rather, a call to the new defaultFromKeychainItems:) To answer your question about securityDomain: it's always ignored for web forms, which is why it's being dropped here (it briefly wasn't in nightlies post 1.0 but pre 1.5, but I learned that that was a mistake, and stopped trying to use it). findAuthKeychainEntryForHost:port:scheme:securityDomain: which contains the lookup, the security domain wrangling loop, then the current logic (minus the Safari default check, which doesn't apply to Auth stuff) for picking one to return. - Rename updateKeychainEntry:withUsername:password: to updateAuthKeychainEntry:withUsername:password and leave the logic as-is. - Use your new logic to decide when to do a store vs. update on web form passwords, but call setUsername:password directly for the update case. So the bad news is that we need another round here, but the good news is that I only need to look at the keychain logic itself in the future patches, which means I'll be turning around reviews much, much faster. The other good news is that I actually built and played with this for the first time, and it's awesome :)

Attachment #291056 - Flags: review?(stuart.morgan) → review-

Stuart Morgan

Updated

•

18 years ago

Blocks: 410501

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 84

•

18 years ago

Bryan, will you have time to work on this (semi-)actively in the next week-and-a-half? If so, we'd really like to take it in b1 which will freeze in about a week-and-a-half (and Stuart says the patch is close, if you have the time).

Flags: camino1.6b1?

Bryan Atwood

Assignee

Comment 85

•

18 years ago

Hi Smokey, OK, I'll give it a shot, thanks for the heads up. Will need to do this over the weekend, though. It seems like a few changes are due to the fact that I'm working from an old snapshot. By any chance, do you know how to update my tree to the latest version without overwriting the changes in my patch?

Stuart Morgan

Comment 86

•

18 years ago

Doing a 'cvs update' will merge your local changes with ToT; you may have to resolve a conflict or two manually, but nothing will be stomped.

Bryan Atwood

Assignee

Comment 87

•

18 years ago

Attached patch Patch for comments in #83 (obsolete) — Details — Splinter Review

Great review, Stuart. I'm happy with these changes, they make the code easier to understand in general. I've implemented your changes and will file bugs for the resolution independent code once this is checked in. You should be able to follow the changes that I made without comment here, but ping me if you have any questions. I still want to add a scroll view to the username popup and hide the popup on browser scroll, but in the interest of time I'll work on these in a separate feature request.

Attachment #291056 - Attachment is obsolete: true

Attachment #295610 - Flags: review?(stuart.morgan)

Stuart Morgan

Comment 88

•

18 years ago

Comment on attachment 295610 [details] [diff] [review] Patch for comments in #83 >+- (KeychainItem*)findWebFormKeychainEntryForUsername:(NSString*)username >+ forHost:(NSString*)host >+ port:(PRInt32)port >+ scheme:(NSString*)scheme >+{ >+ // There may be multiple entries for the same username so we load them all then filter I think you can simplify this method by starting with the KeychainItem keychainItemForHost:username:port:protocol:authenticationType: call, then if that doesn't return anything call findLegacyKeychainEntryForHost: (and checking that the username on that is right before returning it). Since we only every could have had one old-style entry per host, it's fine that you don't have a username parameter, because if you get one with the wrong username you can be confident that there isn't another entry with the right one. I can see arguments for either approach though, so your method is fine as is if you disagree about that being simpler ;) >+- (NSArray*)allWebFormKeychainItemsForHost:(NSString*)host port:(PRInt32)port scheme:(NSString*)scheme >+{ ... >+ while ((item = [keychainEnumerator nextObject])) { >+ if ([item authenticationType] == kSecAuthenticationTypeHTMLForm) >+ [returnItems addObject:item]; >+ else if ([item authenticationType] == kSecAuthenticationTypeHTTPDigest || >+ [item authenticationType] == kSecAuthenticationTypeHTTPDigestReversed) { >+ [[KeychainService instance] upgradeLegacyKeychainEntry:item withScheme:scheme isForm:YES]; >+ [returnItems addObject:item]; >+ } >+ } As much as I would love to upgrade these here, I'm afraid we can't. Until the user actually submits that account info in a form, we don't know that it's not really an HTTP auth entry (or even in rare cases, .Mac account info we didn't store), and we don't want to upgrade the wrong way. So this should just be something like: if type == form || type == digest [returnItems addObject:item]; else if type == reverseDigest [item setAuthenticationType:kSecAuthenticationTypeHTTPDigest]; [returnItems addObject:item]; With those changes, r=me! Thank you yet again for you patience with both my reviews and with what I know (all too well) is some very messy and confusing code.

Attachment #295610 - Flags: review?(stuart.morgan) → review+

Stuart Morgan

Comment 89

•

18 years ago

Oops, one more thing: KeychainAutoCompleteSession's init should call super: - (id)init { if ((self = [super init])) { mUsernames = [[NSMutableArray alloc] init]; mDOMListener = new KeychainAutoCompleteDOMListener; } return self; }

Bryan Atwood

Assignee

Comment 90

•

18 years ago

Attached patch Patch for comments in #88 / #89 (obsolete) — Details — Splinter Review

Yeah, this keychain code is not very fun. I wish we could just drop support for old entries, it would make thinks much easier. >>+- (KeychainItem*)findWebFormKeychainEntryForUsername:(NSString*)username >> ... > I think you can simplify this method by starting with the KeychainItem > keychainItemForHost:username:port:protocol:authenticationType: call, then if > that doesn't return anything call findLegacyKeychainEntryForHost: Done. >>>+- (NSArray*)allWebFormKeychainItemsForHost:(NSString*)host... > this should just be something like: > if type == form || type == digest > [returnItems addObject:item]; > else if type == reverseDigest > [item setAuthenticationType:kSecAuthenticationTypeHTTPDigest]; > [returnItems addObject:item]; Done. > Oops, one more thing: KeychainAutoCompleteSession's init should call super: Done. Feel like we're making some progress! Thanks again for catching all of the keychain gaffs. I think it takes someone who understands the history to update it correctly. Oh, do you know who I can ask to do superreview?

Attachment #295610 - Attachment is obsolete: true

Attachment #295666 - Flags: review+

Stuart Morgan

Comment 91

•

18 years ago

Comment on attachment 295666 [details] [diff] [review] Patch for comments in #88 / #89 (In reply to comment #90) > I wish we could just drop support for old entries, it would make thinks > much easier. Ignoring the support requests from people wanting to know why they can't log in to sites anymore, yes ;) I am trying to phase things out as fast as I think we can get away with though, and post-1.6 things will get somewhat better. > Oh, do you know who I can ask to do superreview? Mento or pink; let's try mento.

Attachment #295666 - Flags: superreview?(mark)

Mark Mentovai

•

18 years ago

Wow, that was a damn fine review, Mark. Thank you and Stuart for putting so much time into this. Luckily no major changes are needed so I'm getting close to finishing. I have some questions before I submit. 1. After having to rewrite a big chunk of my code to remove deprecated Gecko functions (xpcom), I'm hesitant to use any more of it than I need. I want to change the functions I wrote that return 'nsresult' to return BOOL (or void) instead. Particularly GetFormInfoForInput() in KeychainAutoCompleteSession.mm. Any objections to that? 2. If we don't use nsresult as a return type, should functions like GetFormInfoForInput return BOOL to indicate success, or just leave the pointers alone or set to nil on error? In general, the style in Camino that I see is to return void, and leave the pointers untouched or nil, but want to double-check. 3. > >Index: mozilla/camino/src/formfill/AutoCompleteUtils.mm > > >+- (void)setSearchString:(NSString*)string > > >+ [mSearchString autorelease]; > > Should be release. > > >+- (void)setMatches:(NSArray*)matches > > >+ [mMatches autorelease]; > > As above. I followed the advice of the "Value Objects and Copying" section of this doc: http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAccessorMethods.html Any reason why Apple is recommending autorelease instead of release? 4. > >+ // TODO: add a scroll view to limit number of passwords shown to kMaxRows > >+ // For some reason, the scroll bar is greyed out so I've commented it out for now > We had this in Gecko once we switched to Cocoa widgets, it's because the window > wasn't main. See how they fixed it in Gecko? I still can't get the crummy scroll bar in the username popup window to be blue instead of grey even though the selected row in the table is blue. In Gecko (Firefox), they use Carbon and there is some function like hiliteControl that forces the control "on". I couldn't find an equivalent in Cocoa. I use the same method as the AutoCompleteTextField popup, but it just won't activate. If you open TextEdit and do command-T to get the font panel to pop up, the scroll bars there are blue. Argh, very frustrating... OK, let me know what to do about the above (well, we can skip #4 if we are in a rush to get this patch in). Thanks!

Mark Mentovai

Comment 96

•

18 years ago

>1. After having to rewrite a big chunk of my code to remove deprecated Gecko >functions (xpcom), I'm hesitant to use any more of it than I need. I want to >change the functions I wrote that return 'nsresult' to return BOOL (or void) >instead. Particularly GetFormInfoForInput() in KeychainAutoCompleteSession.mm. > Any objections to that? > >2. If we don't use nsresult as a return type, should functions like >GetFormInfoForInput return BOOL to indicate success, or just leave the pointers >alone or set to nil on error? In general, the style in Camino that I see is to >return void, and leave the pointers untouched or nil, but want to double-check. I don't have any objections to using BOOL at all. You only need to return an nsresult from functions that will be called by Gecko, where it's dictated by the API. It's also acceptable to return an nsresult if everything you call provides one: then the return value is unambiguous. For GetFormInfoForInput, a BOOL return value would be more than sufficient. I wouldn't use void on this one because the function has several out parameters, and it's not clear which ones a caller should check to determine success. Generally speaking, outside of Gecko: - void is OK when callers don't have any reason to care about the success of an operation. A lot of Camino code is like this because the technicalities of success are intricate details that don't concern us. For example, a lot of code that calls -[PreferenceManager getBooleanPref:withSuccess:] passes NULL for withSuccess and never checks it because it's perfectly safe to assume NO (false) as a default for boolean preferences that are unset. - When callers might care about success or failure: - For functions that return a single value: - Where one of the possible values can unambiguously indicate failure, just return the result. Most pointers fall into this category, because nil (or NULL or nsnull) can unambiguously indicate failure. - When you need to return some data type that either doesn't have a unique value that can be used to indicate failure, or when checking it for failure would be cumbersome, return a boolean value and use an out parameter to get the actual data back to the caller. A function like |char charAt(std::string s, int index)| might fall into this category, because, assuming that your string might have embedded NUL bytes, there's no illegal char value that could be used to signal failure. An example of something cumbersome to check would be an nsIntRect: while you might be able to define an "empty" rectangle as invalid and use it to signal failure, in reality, checking a rectangle for emptiness involves multiple comparisons and isn't a simple operation. If you need to signal failure, it's better to return a boolean value. I made this suggestion for your GeckoUtils::GetFrameInScreenCoordinates function. - For functions that return multiple values, unless there's one "primary" return value and several subsidiary return values, stick everything you need to return into its own out parameter, and return a boolean value if you need to indicate success or failure. >3. [...] >I followed the advice of the "Value Objects and Copying" section of this doc: > >http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAccessorMethods.html > >Any reason why Apple is recommending autorelease instead of release? Apple's document is a little bit simplistic. Take the whole thing, including the top section which suggests three patterns, and you can distill it into a few decisions you need to make about your code: - Should the getter return the internal pointer directly or return a copy? - Returning a copy ensures that the caller can't trample the object's own copy of the data. In Cocoa, a returned copy will usually be autoreleased, unless the API specifies that the callee will assume ownership - this is usually signified by a call with "copy" in its name. - Returning the pointer is most efficient, but the callee can now affect the object's internal state, and other changes made to the pointed-to data by the object itself will show up in the caller's copy. That might be undesirable. Also: should the returned value be retained and autoreleased or just returned outright? - Returning the pointer outright is most efficient, but the returned pointer's lifetime is only guaranteed as long as the object itself holds on to it. If the pointer is reset or the object is deallocated, the callee's copy can become invalid. - Returning an autoreleased pointer is less efficient but guarantees that the data will last at least until the autorelease pool is popped. - Should the setter set the internal pointer directly or set and retain a copy? - As in the getter case, storing a copy ensures that the caller and the object can't interfere with each other by manipulating the data, but is less efficient by virtue of copying. - Retaining is more efficient, but there's the potential for the object and its caller to interfere with each other. - When setting a new pointer, should the setter release or autorelease the old? - Efficiency is debatable, because a release might trigger an immediate dealloc, which can cause more code to be executed in the near term. An autorelease would cause that same dealloc to be deferred until later, so you might get more other code executed in the near term, although overall efficiency is reduced. - Usually, release is the right choice, if all callers have observed the rules above according to your chosen getter/setter pattern. Because autorelease results in a deferred dealloc, it might mask bugs that occur in poorly-written caller code if your getter returns the internal pointer directly without autoreleasing. If you control all of the caller code (as we do), it's probably a better idea to fix the caller code to not be so ignorant of proper memory management techniques. The decision between release and autorelease can be tricky: usually, you'll choose autorelease because you can do it quickly when you allocate an object that you don't need for very long, and you don't need to concern yourself with tricky memory management thereafter. Note that this doesn't usually apply to class members. It also doesn't have anything to do with whether a getter returns its pointer or retains and autoreleases. When you mean to say "my code is done with this now, I don't care what happens to it, and as far as I'm concerned, it can be deallocated," you should write "release", and your attitude should be that if some other code still refers to the object and it hasn't retained it, it's the other code's fault and problem. I think that most setters should fall into this category. You should answer these questions independently for your code. In Apple's "Value Objects and Copying" section, their getter returns a copy and autoreleases; their setter sets a copy and autoreleases the old pointer, but this is merely an example to illustrate the use of "copy" instead of "retain". The "copy" pattern is applicable to straight release, too. I only caught two instances of "copy" in your code: -[AutoCompleteResults setSearchString:] and -[KeychainAutoCompleteSession attachToInput:]. I commented on the former but not the latter, but really, in both cases, you should verify that you really need a "copy" instead of a "retain". My advice, unless you're trying to maintain some public-facing API where you neither control nor trust your callers, is to let getters return the internal pointer without retaining and autoreleasing it, and for setters to release the existing pointer and retain the new one. That gives you maximum efficiency and semantics that work for the bulk of the cases you'll come across. If an individual caller has special needs, it's of course free to make its own copies without degrading the performance of other callers and the class as a whole by imposing the copy behavior globally. >4. [...] >I still can't get the crummy scroll bar in the username popup window to be blue >instead of grey even though the selected row in the table is blue. In Gecko >(Firefox), they use Carbon and there is some function like hiliteControl that >forces the control "on". I couldn't find an equivalent in Cocoa. I use the >same method as the AutoCompleteTextField popup, but it just won't activate. If >you open TextEdit and do command-T to get the font panel to pop up, the scroll >bars there are blue. Argh, very frustrating... HiliteControl is a Carbon function and it's only used in Carbon widgets, as used by Firefox on the 1.8[.1] branch and earlier. The current Firefox trunk uses Cocoa widgets, and that's what I was suggesting you look into, because I know that this problem existed in early Cocoa-widgeted Firefoxen, and I know it's been resolved since. (Although I'm secretly glad that you found HiliteControl - the Carbon widget stuff was my very own personal dead-end codebase for a couple of years.) You can file a followup bug for this and for everything else I suggested you might want a followup bug for.

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 97

•

18 years ago

(In reply to comment #96) > HiliteControl is a Carbon function and it's only used in Carbon widgets, as > used by Firefox on the 1.8[.1] branch and earlier. The current Firefox trunk > uses Cocoa widgets, and that's what I was suggesting you look into, because I > know that this problem existed in early Cocoa-widgeted Firefoxen, and I know > it's been resolved since. That was bug 339447, and it looks like Håkan did a bunch of work on it and got it working, but it seems instead Firefox ultimately solved the problem by switching Gecko from using native scrollbars to using fake ones (bug 370439), according to the last comment. Since I don't think we want to use fake scrollbars for this, perhaps Håkan's patch will prove useful for us. (But we should take this to a new bug, and copy the useful bits over, once the patch is ready.)

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 98

•

18 years ago

We shipped 1.6b1, but we'd like to get this in as early as possible going forward to b2 (we know you need weekends to work on this)....

Flags: camino1.6b2?

Flags: camino1.6b1?

Flags: camino1.6b1-

Bryan Atwood

Assignee

Comment 99

•

18 years ago

No problem Smokey. It was good to wait to get in Mark's fixes. It's a better patch. This is almost ready, just doing clean-up. Will be ready Friday or Saturday.

Bryan Atwood

Assignee

Comment 100

•

18 years ago

Attached patch Patch for comments #92/93 (obsolete) — Details — Splinter Review

Patch for Mark's comments. Will post details in a separate comment.

Attachment #295666 - Attachment is obsolete: true

Attachment #297870 - Flags: superreview?(mark)

Attachment #297870 - Flags: review+

Bryan Atwood

Assignee

Comment 101

•

18 years ago

This should hopefully be the final patch, unless there are very minor edits. I'll be filing a few follow-up bugs, the main ones being: 1. The username popup box has a scroll bar if there are more than 10 usernames, but the scroll bar is greyed out. It should be blue. 2. If the username popup is open and the user scroll the web page, the page moves but the popup is stationary. I'd like to get those two fixed before 1.6 final so let me know ahead of time if we're getting close to the deadline. Other than that, comments are inline. If I skipped a comment, it means I implemented your suggestion. (In reply to comment #92) > (From update of attachment 295666 [details] [diff] [review]) > >Index: mozilla/camino/src/formfill/FormFillController.mm > > >+- (void)removeWindowListeners:(nsIDOMWindow*)aWindow > > >+ nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(chromeEventHandler); > > I wonder if it makes sense to cache target so we can avoid doing all of this > work to figure it out again. > > If you don't want to cache target, you still might want to add a function to > get target from aWindow and have addWindowListeners: and removeWindowListeners: > both call that, eliminating duplicated code. > > If you don't like either of the two suggestions above, there's still this: the > previous comment about separating the declaration from its initial assignment > applies here, too. I also noticed that addWindowListeners: checks for aWindow > before using it, but this doesn't - shouldn't they match? Since this is only called when the browser window is closed, there isn't much motivation for caching. I also don't think it's worth a separate customized Gecko function since it is so infrequently used and it would save us only 3 lines of code. If you feel strongly that it should be there, let me know. I made the changes you mentioned in the last paragraph. > >+- (void)startControllingInput:(nsIDOMHTMLInputElement*)aInput > > >+ // Make sure we're not still attached to an input > > An input? Input what? Input element, right? The comment should say so. > Also, I think this name and stopControllingInput: would be better if slightly > more descriptive: startControllingInputElement:? Same goes for mFocusedInput, > aInput, etc. If you meant something else, by all means, say so. OK, changed these to stopControllingInputElement, etc. > >+- (void)openPopup > > >+ GeckoUtils::GetFrameInScreenCoordinates(mFocusedInput, &inputRect); > > GetFrameInScreenCoordinates might return without touching inputRect. Are those > cases failures? Should openPopup bail out if that happens? Maybe > GetFrameInScreenCoordinates should have a return value indicating success or > failure, allowing you to handle it appropriately in openPopup. Nice catch. I changed this to return a PRBool and noted it in the comments. > >+ // Convert top-left origin to bottom-left origin (Cocoa coordinates) > > Is there a utility to convert between coordinate systems already? Not that I could find. In Cocoa, you can use NSAffineTransform object, but it's more trouble than it's worth and you still have to call [screen frame] to get the size of the screen. I changed this slightly to match the similar code we use for <select> popups. > >+ NSScreen* screen = [NSScreen screenForPoint:point]; > > Have you tested this with display spanning on multiple displays, and with > windows that straddle both displays? Glad you pointed this out. It was putting the popup in the wrong place, but I (think I) fixed it when I found the <select> popup code. > >+- (void)popupSelected > > >+ nsInput->SetSelectionStart((PRInt32)searchLength); > > Do you need to call SetSelectionEnd too? Maybe not, I don't know. If you do > need to, you can call SetSelectionRange. The same comment applies in > autoCompleteFieldText. In fact, since there's duplicated code in popupSelected > and autoCompleteFieldText, maybe you should move it out into its own private > method that they can each call. The Start point seems to be good enough to highlight to the end of the line. > >+- (void)filledAutoCompleteFieldText > > >+ mFocusedInput->GetOwnerDocument(getter_AddRefs(domDoc)); > >+ > >+ nsCOMPtr<nsIDOMDocumentEvent> doc = do_QueryInterface(domDoc); > > I'm not posiitve it's kosher to do_QueryInterface something that might be null. > It's easy enough to find out. If it's not cool, just add another early > return. You can probably just check |if (NS_SUCCEEDED(GetOwnerDocument))|. > Same thing a few lines down where you call CreateEvent. Yes, if you pass in a null pointer to do_QueryInterface, it will return a null pointer. So I only added the checks afterwards. This seems to match the style of other code in the Camino tree. > >+- (BOOL)handleKeyNavigation:(int)aKey > > >+ case nsIDOMKeyEvent::DOM_VK_PAGE_DOWN: > > Do you want to start a search if you get this key while the caret's at EOL too, > like in the DOM_VK_DOWN case? Sounds good. I have it start the search on page down regardless of where the cursor is, like Firefox does. In general I try to make minor functionality like this the same as existing browsers since it always bugs me when the UI behaves slightly different when I'm working in another browser. > >Index: mozilla/camino/src/formfill/FormFillPopup.mm > > >+- (int)visibleRows > > >+ return minRows < kMaxRows ? minRows : kMaxRows; > > This would be clearer if the condition (were < parenthesized). I caught > another ternary earlier in the patch that was parenthesized but didn't need to > be. ok, I think that was isPopupOpen in FormFillController.mm so I removed the parens there. > >+ if (!mItems) > >+ return 0; > >+ > >+ return [mItems count]; > > You don't need the conditional at all, it's safe to return [mItems count];. > It'll evaluate to 0 if mItems is nil. The same applies to resultForRow, if you > can tolerate a nil string instead of @"". (I think you should tolerate a nil > string, as an indication of "no! bad!", compared to @"", which is a perfectly > valid string.) Yep, changed. > >+- (void)setItems:(NSArray*)items > >+{ > >+ [mItems autorelease]; > > >+ mItems = [items retain]; > > You wanted to release, not autorelease, right? > OK, changed to release like you mentioned in the later comment. This now checks to make sure that it doesn't release mItems if it's the same pointer as items in case it's the only reference. (In reply to comment #93) > (From update of attachment 295666 [details] [diff] [review]) > > >Index: mozilla/camino/src/formfill/KeychainService.h > > >+nsresult > >+FindUsernamePasswordFields(nsIDOMHTMLFormElement* inFormElement, nsIDOMHTMLInputElement** outUsername, > >+ nsIDOMHTMLInputElement** outPassword, PRBool inStopWhenFound); > >+nsresult > >+FindPasswordField(nsIDOMHTMLInputElement* inUsername, nsIDOMHTMLInputElement** outPassword); > > Easy enough to keep these guys within 80 columns by splitting the lines at the > arguments. > > FindPasswordFields is used by other files, but FindUsernamePasswordField is > only used in the file in which it's defined, KeychainService.mm. As such, we > should make it static in KeychainService.mm, and we shouldn't declare it in > KeychainService.h, unless you foresee needing to call it from elsewhere. > > I also see KeychainService.mm has a function called GetNSWindow, which is also > only used in that file - it, too, should be made static. FindPasswordField is only used in KeychainAutoCompleteSession so I moved it there and made it static. It relies on FindUsernamePasswordFields in KeychainService, so I moved the declaration of FindUsernamePasswordFields to the .h file > >Index: mozilla/camino/src/formfill/KeychainService.mm > > >+- (KeychainItem*)findWebFormKeychainEntryForUsername:(NSString*)username > >+ forHost:(NSString*)host > >+ port:(PRInt32)port > >+ scheme:(NSString*)scheme > > I think it's hokey to use a PRType in this interface, but we do it throughout. > I advise filing a followup bug to clean this up. Since this patch is already pretty big and there are lots of marginally related changes like this, I'll file a followup bug > >+// setDefaultWebFormKeychainEntry > >+// > >+// This method sets the default Camino keychain entry. The default Camino entry will > >+// have a comment value set to "camino_default". It first clears any existing default > >+// Camino entry before setting the passed in keychain to the new default. It doesn't > >+// modify any other comment values, meaning that the Safari default will preserved. > > How does this interact with Safari? If we set camino_default on an item and > Safari comes along later and wants to make the item its default, does it > clobber camino_default or is it unable to cope? I assume we're mostly OK if it > replaces camino_default with default, since we'll still at least see the entry > as our default. Still, this is all kind of unclear. I think you should have a > higher-level comment explaining how the keychain item's comment field is used > and how the camino_default and default tags interact in a little more detail. > The comment you have here now is a start, but if all you read is the comment > and the method that follows, you're left wondering what's missing. Agreed. Comment made more clear. Safari manhandles the comment field and sets it to 'default' regardless of what's there before. So the logic is: Safari always sets its default username via the comment field. Camino can use another username by setting 'camino_default'. If there is no Camino default, we use Safari's default. This means that if Safari clobbers the comment field, Camino will still use the same username it was before. And we prevent Camino from changing the comment for Safari's default since Safari wouldn't choose it afterwards. > >+- (NSArray*)allWebFormKeychainItemsForHost:(NSString*)host port:(PRInt32)port scheme:(NSString*)scheme > > >+ NSMutableArray* returnItems = [[NSMutableArray alloc] init]; > [...] > >+ return returnItems; > > This leaks. Ownership is transferred to the caller here. I see two call > sites, both in -[KeychainAutoCompleteSession attachToInput:], and neither > releases the returned array. > > Either note that the caller owns the returned array in the comments that apply > to this method, and fix the callers to release the array, or note that this > method returns an autoreleased array and autorelease returnItems when you > return it. Got it. As Stuart pointed out, changed this to autorelease. > >+- (KeychainItem*)findAuthKeychainEntryForHost:(NSString*)host > > Hmm, so no multiple keychain item support for HTTP AUTH? Maybe file an RFE to > add that in the future. > > >- // Finally, check for a new style entry created by something other than Camino. > > Why is this section coming out for the HTTP AUTH case? The only thing that was removed was the check for the Safari default since it doesn't apply to AUTH case according to Stuart in comment #83 > >+FindPasswordField(nsIDOMHTMLInputElement* inUsername, nsIDOMHTMLInputElement** outPassword) > > >+ if (!inUsername) > >+ return NS_OK; > > Doesn't seem like NS_OK to me. NS_ERROR_FAILURE is a generic return value you > can use for functions that return nsresult when they don't complete > successfully. Change the other early returns in this function, too. For > example, later on: > > rv = FindUsernamePasswordFields(formElement, getter_AddRefs(usernameElement), > getter_AddRefs(passwordElement), PR_TRUE); > if (NS_FAILED(rv)) > return rv; > if (usernameElement != inUsername || !passwordElement) > return NS_ERROR_FAILURE; > > The existing FindUsernamePasswordFields should be changed accordingly: early > returns for failures shouldn't be NS_OK. If these functions both always return > NS_OK, they might as well be type void instead of nsresult. OK, for FindPasswordField I ended up making it a void function. I ended up leaving FindUsernamePasswordFields as is w/r/t NS_OK. I'd like to chat with Stuart about this and possibly change this to a void function. I'll follow a separate bug. > If you keep the nsresult return type and fix it to be usable, you can use the > return value in KeychainAutoCompleteSession.mm instead of checking the out > parameter: > > >+ FindPasswordField(usernameElement, &passwordElement); > >+ if (!passwordElement) > >+ return NO; > > becomes > > if (NS_FAILED(FindPasswordField(usernameElement, &passwordElement))) > return NO; See above. This was changed to: nsCOMPtr<nsIDOMHTMLInputElement> passwordElement = FindPasswordField(usernameElement); if (!passwordElement) return NO; > >Index: mozilla/camino/src/formfill/AutoCompleteUtils.mm > > >+- (void)setSearchString:(NSString*)string > > >+ [mSearchString autorelease]; > > Should be release. OK, changed to release as mentioned above. > >+ mSearchString = [string copy]; > > The caller might change the string, so we need a new object instead of > retaining |string| here? Yes, the same NSString object is used every time a new search is performed. I had a problem when that changed, it also changed the AutoCompleteResults mSearchString value so had to make a copy instead. > >Index: mozilla/camino/src/formfill/KeychainAutoCompleteSession.h > > >+ nsIDOMHTMLInputElement* mUsernameElement; // strong > >+ nsIDOMHTMLInputElement* mPasswordElement; // strong > > Should these be nsCOMPtrs? Seems like that would simplify matters over in the > .mm file: you shouldn't need a constructor or destructor at all, and you > wouldn't have that manual AddRef/Release tracking in SetElements. Yes, thanks, changed these to nsCOMPtr. They used to be inside of a Objective-C class and didn't get updated when it moved to a separate C++ class. > >+KeychainAutoCompleteDOMListener::KeychainAutoCompleteDOMListener() > > If you don't make mUsernameElement and mPasswordElement into nsCOMPtrs: > > Follow the C++ initializer list pattern I showed you in the previous comment, > the constructor should wind up with an empty {body}. > > If you do make them into nsCOMPtrs (see my comments for the related .h, above): > > The constructor and destructor shouldn't be necessary, as an nsCOMPtr is > properly initialized to point to nsnull, and cleans up after itself on > destruction. Done as mentioned above. Removed constructor/destructor > >+NS_IMETHODIMP KeychainAutoCompleteDOMListener::HandleEvent(nsIDOMEvent *aEvent) > > This method has the return type on the same line as the method name like this > one, but the others have the return type on their own line. Be consistent. > (My preference is for the return type on the same like, like you've got here.) > > >+ aEvent->GetTarget(getter_AddRefs(target)); Done. Moved all these to the same line. > >+ [results setMatches:resultMatches]; > >+ > >+ [listener autoCompleteFoundResults:results]; > > This method seems to leaks results and resultMatches. Both of these calls will > retain the objects that you pass to them, but you never release them in this > method. You should release (not autorelease) resultMatches after giving it to > results, and release results after giving it to listener. I didn't spot any > other unmentioned leaks like this as I read through, but I might have missed > something, so you might want to look for other cases where you alloc your own > object. As I recall, you did a good job releasing objects that were stuck into > member variables. Another great catch. Vetted all the code I added and didn't find any others. > >Index: mozilla/camino/src/formfill/KeychainItem.m > > >+ const char* accountName = username ? [username UTF8String] : NULL; > > You (and the existing code above that works with serverName and host) don't > need to be so careful here. If username is nil, [username UTF8String] will be > too, and that's suitable to assign to a char* to get NULL. There is another method in the class that has this format. In the interest of keeping this patch smaller, I'll keep my code in the format of the existing code, then file a followup bug. > >Index: mozilla/camino/src/extensions/GeckoUtils.cpp > > >+void GeckoUtils::GetFrameInScreenCoordinates(nsIDOMElement* aElement, nsIntRect* aRect) > > There are a bunch of early returns here - can callers tolerate not having their > rect set in a failure? Do you need to add a return value instead of having > this be void? Caller can tolerate unset rect. Also, this returns PRBool for success/failure. > >Index: mozilla/camino/src/extensions/GeckoUtils.h > > > static void GetIntrisicSize(nsIDOMWindow* aWindow, PRInt32* outWidth, PRInt32* outHeight); > > Existing code, that should be spelled GetIntrinsicSize. You don't need to fix > it in this bug (although you're welcome to if there aren't many uses), but we > should file a new bug to fix it. I'll file another bug since I don't want to add non-related files to this patch.

Bryan Atwood

Assignee

Updated

•

18 years ago

Blocks: 413034

Chris Lawson (gone)

Comment 102

•

18 years ago

(In reply to comment #101) > > >+ // Convert top-left origin to bottom-left origin (Cocoa coordinates) > > > > Is there a utility to convert between coordinate systems already? > > Not that I could find. In Cocoa, you can use NSAffineTransform object, but Mark and I talked about this on IRC tonight and we're both pretty sure that there is, but it only exists on trunk. Since this is a branch patch too, that's probably just fine, but there might be a better way to do this on trunk eventually, so it might be worth opening a followup bug to fix the code for that. Håkan and I looked at bug 384657 during the meetup, which was the impetus (IIRC) for getting a real solution for this sort of thing into widget/cocoa (see bug 392395). Just FYI.

Bryan Atwood

Assignee

Comment 103

•

18 years ago

Attached patch Update to patch for comments #92/93 (obsolete) — Details — Splinter Review

I thought Håkan's work on getting the scroll bars activated would be complicated, but his solution for bug 339447 was elegant and simple to add. I took a similar approach, subclassed NSPanel in FormFillPopup.h to return YES on |isKeyWindow|. Since that fixed the scroll bar issue, I wanted to update the patch. (Sorry for the double post Mark!) > > >Index: mozilla/camino/src/formfill/KeychainItem.m > > > > >+ const char* accountName = username ? [username UTF8String] : NULL; > > > > You (and the existing code above that works with serverName and host) don't > > need to be so careful here. If username is nil, [username UTF8String] will be > > too, and that's suitable to assign to a char* to get NULL. > There is another method in the class that has this format. In the interest of > keeping this patch smaller, I'll keep my code in the format of the existing > code, then file a followup bug. It ended up being in only one spot so I fixed it in this updated patch.

Attachment #297870 - Attachment is obsolete: true

Attachment #297926 - Flags: superreview?(mark)

Attachment #297926 - Flags: review+

Attachment #297870 - Flags: superreview?(mark)

Mark Mentovai

Comment 104

•

18 years ago

Comment on attachment 297926 [details] [diff] [review] Update to patch for comments #92/93 >Index: mozilla/camino/src/formfill/FormFillController.mm >+- (void)browserResized:(NSNotification *)notification Your style seems to not put a space between the type name and the * that makes it a pointer - be consistent. I wouldn't have said anything, but I also noticed that this method isn't declared in the @interface section in the header or the private category in this .mm. >+- (void)addWindowListeners:(nsIDOMWindow*)aWindow >+{ >+ if (!aWindow) >+ return; >+ >+ nsCOMPtr<nsPIDOMWindow> privateDOMWindow = do_QueryInterface(aWindow); [...] >+- (void)removeWindowListeners:(nsIDOMWindow*)aWindow >+{ >+ [self stopControllingInputElement]; >+ >+ nsCOMPtr<nsPIDOMWindow> privateDOMWindow = do_QueryInterface(aWindow); I saw your comments about not sharing the code that gets the nsIDOMEventTarget for the nsIDOMWindow, and that's fine, but I also asked why addWindowListeners: checked aWindow before using it, and removeWindowListeners: did not. According to your comments, calling do_QueryInterface(NULL) is safe and evaluates to NULL, so I think you should drop the (!aWindow) check from addWindowListeners. (For what it's worth, if this were the first review round, I'd probably suggest having a static array of event names and handler selectors, and using that array in a loop in FormFillListener::HandleEvent, -[FormFillController addWindowListeners:], and -[FormFillController removeWindowListeners:]. Since this isn't the first review round, let's leave it as-is, since it's really fine. Just something to keep in mind for the future - when you do similar operations with the same five variables a few times straight, it can probably benefit from a loop.) >+-(void)autoCompleteFoundResults:(AutoCompleteResults*)results >+{ >+ [mResults release]; >+ mResults = nil; One of your responses to my comments made a very good point: in a method that functions as a setter, it's safest to make sure that the pointer is actually changing before calling release. If the pointer's not changing and you don't check it, release-retain could dealloc. You don't need to change these if they're guaranteed to be safe: if they're never called to set the same object that's already set. But I know you've got a lot of setters, so you might want to have a look at this possibility if you haven't yet. >+- (void)openPopup >+ NSScreen* mainScreen = [[NSScreen screens] firstObject]; // NSArray category method You say you "think" you've fixed this bit - if you don't have the equipment to test with multiple displays, be sure someone else checks it out for you. This can wait until after it's checked in, and it's OK to file a "please help me test this" bug - it can just be marked invalid or worksforme if everything checks out and no action is needed. >+ // TODO: check shift here, still not aligned sometimes. Followup bug! >Index: mozilla/camino/src/formfill/FormFillPopup.mm >+- (id)init >+ // TODO: For some reason, the scroll bar is greyed out. This comment no longer applies, right? (Nice job!) For a followup bug: handle the cases where the main window loses and gains key status gracefully. We don't want the main window to have gray scroll thumbs while the form fill popup hangs on to its blue one. It might even be reasonable to just close the popup when the main window loses key status - but it'd be better to track the main window's state. You also mentioned moving and scrolling in the main window, and I'll add minimizing to the dock. It's totally reasonable to just close the popup in each of these cases, I wouldn't even worry about trying to make the popup follow the action. Followup bug! >Index: mozilla/camino/src/formfill/KeychainService.mm >+// Discussion of how the default keychain entry is chosen in Camino and Safari: Thanks for adding this comment, just something to make it read more cleanly: >+// 'camino_default', then looks for 'default' if there's not one. Safari will "there isn't one" sounds better. Also, it seems like you usually put two spaces after a period - good! so do I! - why didn't you here? >+// overwrite the comment field when setting the default. So if it used to be >+// the Camino default, Camino will still get the correct one. Camino Change this sentence to: "If Safari changes the 'camino_default' entry to 'default', Camino will still treat it as the default." >+// preserved if Camino selects the same one. "preserved if the user selects the same item in Camino." >OK, for FindPasswordField I ended up making it a void function. I ended up >leaving FindUsernamePasswordFields as is w/r/t NS_OK. I'd like to chat with >Stuart about this and possibly change this to a void function. I'll follow a >separate bug. FindUsernamePasswordFields definitely shouldn't be made void because it has multiple out parameters, and it's a pain to figure out if you need to check them all or check one or whatever else. If you like BOOL better than nsresult, that's cool, though. My own preference would be for FindPasswordField to return something and not be void to indicate success - you're tied to the out-param thing for proper Gecko nsCOMPtr behavior, but which reads more cleanly to you?: >+ FindPasswordField(usernameElement, getter_AddRefs(passwordElement)); >+ if (!passwordElement) >+ return NO; if (!FindPasswordField(usernameElement, getter_AddRefs(passwordElement))) return NO; The second one is clearer to me, and the compiler can often produce better code for the second one, too. The second one also lets you avoid explicitly setting *outPassword in FindPasswordField, which is a win because you do it twice. Again, it can be a BOOL, it doesn't have to be a nsresult. Part of the reason it reads clearer has to do with what happens when someone's trying to decipher a function's behavior based solely on a declaration where there are no comments, or when they're implying a declaration based on how a function is used in context. This: >+static void FindPasswordField(nsIDOMHTMLInputElement* inUsername, nsIDOMHTMLInputElement** outPassword); says to me: "success and failure is unimportant; I might set outPassword, but then again, I might not. You need to look somewhere else to figure it out." This: static BOOL FindPasswordField(nsIDOMHTMLInputElement* inUsername, nsIDOMHTMLInputElement** outPassword); says: "I have a return value! Use me!" >Index: mozilla/camino/src/formfill/KeychainAutoCompleteSession.mm >\ No newline at end of file There should be. >Index: mozilla/camino/src/extensions/GeckoUtils.cpp >+PRBool GeckoUtils::GetFrameInScreenCoordinates(nsIDOMElement* aElement, nsIntRect* aRect) Thanks for making this return something, but I'd go with BOOL instead of PRBool here. No sense in using a PRType where we aren't tied by an API to using one. >+ *aRect = nsIntRect(0,0,0,0); This isn't strictly necessary now that you have a return value. >+ if (!aElement) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aElement); >+ if (!content) >+ return PR_FALSE; Since you told me that do_QueryInterface(NULL) == NULL and is safe, you can get rid of the first (!aElement) check here too. The same comment applies to GeckoUtils::ScrollElementIntoView. The compiler might not be smart enough to figure out that do_QueryInterface(NULL) == NULL and optimize the extra conditionals away, but you are, so let's keep the compiled code size down (as long as it doesn't impact source code readability.) >Index: mozilla/camino/src/extensions/GeckoUtils.h >+ // Finds the screen location (nsIntRect) in screen coordinates of a DOM Element. [...] >+ /* Given a DOM Element, scroll the view so that the element is shown */ There are existing /* C-style */ comments, but if you like // C++ style better, use it - especially since you're already adding a C++ style comment here. If the whole file uses only C-style, then you might want to stick with that for these two guys. Remember, consistency's the name of the game - people will be much less fatigued when reading and maintaining your code if adheres to consistent conventions, even if you've chosen conventions they don't personally agree with. For example, I hate tabs, but one thing I hate more than tabs is trying to figure out how to add lines to code that freely mixes tabs with spaces for indentation. I can waste more time trying to figure out what tab settings to use than I do actually reading or writing code in a file like that. As far as I'm concerned, this patch is in excellent shape. You might want to provide a new patch for checkin since the above might be a lot to ask your checkin buddy to do for you, but I don't think we need another round of review. Thanks for contributing this feature, and I look forward to using it! If you haven't already, be sure to file followups for everything that's been mentioned! Anything that you file as a followup should be marked as dependent on this bug.

Attachment #297926 - Flags: superreview?(mark) → superreview+

Bryan Atwood

Assignee

Comment 105

•

18 years ago

Attached patch Patch for comment #104 (obsolete) — Details — Splinter Review

(In reply to comment #104) > (From update of attachment 297926 [details] [diff] [review]) > >Index: mozilla/camino/src/formfill/FormFillController.mm > > >+- (void)browserResized:(NSNotification *)notification > > Your style seems to not put a space between the type name and the * that makes > it a pointer - be consistent. > > I wouldn't have said anything, but I also noticed that this method isn't > declared in the @interface section in the header or the private category in > this .mm. Done. Added a declaration. > >+- (void)addWindowListeners:(nsIDOMWindow*)aWindow > >+{ > >+ if (!aWindow) > >+ return; > >+ > >+ nsCOMPtr<nsPIDOMWindow> privateDOMWindow = do_QueryInterface(aWindow); > [...] > >+- (void)removeWindowListeners:(nsIDOMWindow*)aWindow > >+{ > >+ [self stopControllingInputElement]; > >+ > >+ nsCOMPtr<nsPIDOMWindow> privateDOMWindow = do_QueryInterface(aWindow); > > I saw your comments about not sharing the code that gets the nsIDOMEventTarget > for the nsIDOMWindow, and that's fine, but I also asked why addWindowListeners: > checked aWindow before using it, and removeWindowListeners: did not. According > to your comments, calling do_QueryInterface(NULL) is safe and evaluates to > NULL, so I think you should drop the (!aWindow) check from addWindowListeners. Ok, I removed the redundant check. > > >+-(void)autoCompleteFoundResults:(AutoCompleteResults*)results > > >+{ > >+ [mResults release]; > >+ mResults = nil; > > One of your responses to my comments made a very good point: in a method that > functions as a setter, it's safest to make sure that the pointer is actually > changing before calling release. If the pointer's not changing and you don't > check it, release-retain could dealloc. You don't need to change these if > they're guaranteed to be safe: if they're never called to set the same object > that's already set. But I know you've got a lot of setters, so you might want > to have a look at this possibility if you haven't yet. Good point. In this case, results is guaranteed to be a newly allocated object so it's safe. I ran through the rest of my code and it appears to be safe. > >+- (void)openPopup > > >+ NSScreen* mainScreen = [[NSScreen screens] firstObject]; // NSArray category method > > You say you "think" you've fixed this bit - if you don't have the equipment to > test with multiple displays, be sure someone else checks it out for you. This > can wait until after it's checked in, and it's OK to file a "please help me > test this" bug - it can just be marked invalid or worksforme if everything > checks out and no action is needed. I tend to use the phrase "I think" a lot :) I did test on two separate dual-monitor setups and it works with the changes I made, meaning the popup shows correctly. > > >+ // TODO: check shift here, still not aligned sometimes. > > Followup bug! My favorite type of bug! > >Index: mozilla/camino/src/formfill/FormFillPopup.mm > > >+- (id)init > > >+ // TODO: For some reason, the scroll bar is greyed out. > > This comment no longer applies, right? (Nice job!) Oh yeah. OK, removed the comment. > For a followup bug: handle the cases where the main window loses and gains key > status gracefully. We don't want the main window to have gray scroll thumbs > while the form fill popup hangs on to its blue one. It might even be > reasonable to just close the popup when the main window loses key status - but > it'd be better to track the main window's state. When the main window loses focus, the text box gets a blur event, which leads to the popup being closed. > You also mentioned moving and scrolling in the main window, and I'll add > minimizing to the dock. It's totally reasonable to just close the popup in > each of these cases, I wouldn't even worry about trying to make the popup > follow the action. Followup bug! Good catch on the minimize. When the window is restored, the popup shows up but it's empty. I'm not sure the best way to handle all of these cases. I wish there was a Rollup event that it could listen for. Maybe there is even an autocomplete popup widget in the new Cocoa Firefox that we could use. Ten-Four on the followup bug. > >Index: mozilla/camino/src/formfill/KeychainService.mm > > >+// Discussion of how the default keychain entry is chosen in Camino and Safari: > > Thanks for adding this comment, just something to make it read more cleanly: > > >+// 'camino_default', then looks for 'default' if there's not one. Safari will > > "there isn't one" sounds better. Also, it seems like you usually put two > spaces after a period - good! so do I! - why didn't you here? Fixed grammar and spacing. > >+// overwrite the comment field when setting the default. So if it used to be > >+// the Camino default, Camino will still get the correct one. Camino > > Change this sentence to: "If Safari changes the 'camino_default' entry to > 'default', Camino will still treat it as the default." Done. > >+// preserved if Camino selects the same one. > > "preserved if the user selects the same item in Camino." Done. > >OK, for FindPasswordField I ended up making it a void function. I ended up > >leaving FindUsernamePasswordFields as is w/r/t NS_OK. I'd like to chat with > >Stuart about this and possibly change this to a void function. I'll follow a > >separate bug. > > FindUsernamePasswordFields definitely shouldn't be made void because it has > multiple out parameters, and it's a pain to figure out if you need to check > them all or check one or whatever else. If you like BOOL better than nsresult, > that's cool, though. > > My own preference would be for FindPasswordField to return something and not be > void to indicate success - you're tied to the out-param thing for proper Gecko > nsCOMPtr behavior, but which reads more cleanly to you?: > > >+ FindPasswordField(usernameElement, getter_AddRefs(passwordElement)); > >+ if (!passwordElement) > >+ return NO; > > if (!FindPasswordField(usernameElement, getter_AddRefs(passwordElement))) > return NO; > > The second one is clearer to me, and the compiler can often produce better code > for the second one, too. The second one also lets you avoid explicitly setting > *outPassword in FindPasswordField, which is a win because you do it twice. > > Again, it can be a BOOL, it doesn't have to be a nsresult. > > Part of the reason it reads clearer has to do with what happens when someone's > trying to decipher a function's behavior based solely on a declaration where > there are no comments, or when they're implying a declaration based on how a > function is used in context. This: > > >+static void FindPasswordField(nsIDOMHTMLInputElement* inUsername, nsIDOMHTMLInputElement** outPassword); > > says to me: "success and failure is unimportant; I might set outPassword, but > then again, I might not. You need to look somewhere else to figure it out." > This: > > static BOOL FindPasswordField(nsIDOMHTMLInputElement* inUsername, > nsIDOMHTMLInputElement** outPassword); > > says: "I have a return value! Use me!" That logic makes sense to me. Changed to return BOOL. I wish all of the Camino was consistent on these types of coding styles, but I guess that's hard considering how many people have worked on it at various times. > > >Index: mozilla/camino/src/formfill/KeychainAutoCompleteSession.mm > > >\ No newline at end of file > > There should be. Done. > >Index: mozilla/camino/src/extensions/GeckoUtils.cpp > > >+PRBool GeckoUtils::GetFrameInScreenCoordinates(nsIDOMElement* aElement, nsIntRect* aRect) > > Thanks for making this return something, but I'd go with BOOL instead of PRBool > here. No sense in using a PRType where we aren't tied by an API to using one. Hm, it doesn't compile with BOOL type. Looks like we would need to #import Cocoa.h. I recommend we leave it as PRBool so that we don't need the import and to match the style of the other functions. Sound OK? > >+ *aRect = nsIntRect(0,0,0,0); > > This isn't strictly necessary now that you have a return value. OK, removed. > >+ if (!aElement) > >+ return PR_FALSE; > >+ > >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aElement); > >+ if (!content) > >+ return PR_FALSE; > > Since you told me that do_QueryInterface(NULL) == NULL and is safe, you can get > rid of the first (!aElement) check here too. > > The same comment applies to GeckoUtils::ScrollElementIntoView. Done x 2 > The compiler might not be smart enough to figure out that > do_QueryInterface(NULL) == NULL and optimize the extra conditionals away, but > you are, so let's keep the compiled code size down (as long as it doesn't > impact source code readability.) > > >Index: mozilla/camino/src/extensions/GeckoUtils.h > > >+ // Finds the screen location (nsIntRect) in screen coordinates of a DOM Element. > [...] > > >+ /* Given a DOM Element, scroll the view so that the element is shown */ > > There are existing /* C-style */ comments, but if you like // C++ style better, > use it - especially since you're already adding a C++ style comment here. If > the whole file uses only C-style, then you might want to stick with that for > these two guys. Remember, consistency's the name of the game - people will be > much less fatigued when reading and maintaining your code if adheres to > consistent conventions, even if you've chosen conventions they don't personally > agree with. For example, I hate tabs, but one thing I hate more than tabs is > trying to figure out how to add lines to code that freely mixes tabs with > spaces for indentation. I can waste more time trying to figure out what tab > settings to use than I do actually reading or writing code in a file like that. > > As far as I'm concerned, this patch is in excellent shape. You might want to > provide a new patch for checkin since the above might be a lot to ask your > checkin buddy to do for you, but I don't think we need another round of review. > Thanks for contributing this feature, and I look forward to using it! Sounds good. How do I make sure it makes it in 1.6b3? Will it automatically if it's super-reviewed? > If you haven't already, be sure to file followups for everything that's been > mentioned! Anything that you file as a followup should be marked as dependent > on this bug. OK, will do. Thanks again for the excellent comments.

Attachment #297926 - Attachment is obsolete: true

Attachment #298049 - Flags: superreview+

Attachment #298049 - Flags: review+

Bryan Atwood

Assignee

Updated

•

18 years ago

Blocks: 413188

Bryan Atwood

Assignee

Updated

•

18 years ago

Blocks: 413189

Mark Mentovai

Comment 106

•

18 years ago

>Hm, it doesn't compile with BOOL type. Looks like we would need to #import >Cocoa.h. I recommend we leave it as PRBool so that we don't need the import and >to match the style of the other functions. Sound OK? Good point. Let's leave it alone, then. >Sounds good. How do I make sure it makes it in 1.6b3? Will it automatically >if it's super-reviewed? By the book, it's as described in http://wiki.caminobrowser.org/Development:Reviewing , specifically the #Checking_In section. You set checkin-needed in the keywords field (like this) and make sure that it's got the camino1.6b3 flag set to either ? or +. Practically, though, we're all excited about this patch and are watching for it, so it's as good as automatic in this case. In fact, I think I've done enough damage here, let's close up shop and keep it moving. I'll even grease the wheels and give you your 1.6b3+ right now. A note to whoever decides to be your checkin buddy: there are eight "cvs add"s in here, and you'll need to hit the project file.

Flags: camino1.6b3? → camino1.6b3+

Keywords: checkin-needed

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 107

•

18 years ago

Bryan, I went to build before checking this in, and I've hit two problems (so far): 1) On trunk, compiling KeychainAutocompleteSession.mm fails with /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/Security.framework/Headers/cssmconfig.h:47: error: conflicting declaration ‘typedef UInt32 uint32’ ../dist/include/nspr/obsolete/protypes.h:117: error: ‘uint32’ has a previous declaration as ‘typedef PRUint32 uint32’ This I worked around by compiling that file with -DNO_NSPR_10_SUPPORT (may or may not be the correct fix), which got me a complete build. 2) On branch, I fail earlier, in GeckoUtils.cpp: /Users/smokey/Camino/dev/18branch/CaminoBranch/camino/src/extensions/GeckoUtils.cpp: In static member function ‘static PRBool GeckoUtils::GetFrameInScreenCoordinates(nsIDOMElement*, nsIntRect*)’: /Users/smokey/Camino/dev/18branch/CaminoBranch/camino/src/extensions/GeckoUtils.cpp:370: error: ‘class nsDerivedSafe<nsIDocument>’ has no member named ‘GetPrimaryShell’ /Users/smokey/Camino/dev/18branch/CaminoBranch/camino/src/extensions/GeckoUtils.cpp:374: error: no matching function for call to ‘nsDerivedSafe<nsIPresShell>::GetPrimaryFrameFor(nsCOMPtr<nsIContent>&)’ ../dist/include/layout/nsIPresShell.h:314: note: candidates are: virtual nsresult nsIPresShell::GetPrimaryFrameFor(nsIContent*, nsIFrame**) const /Users/smokey/Camino/dev/18branch/CaminoBranch/camino/src/extensions/GeckoUtils.cpp: In static member function ‘static void GeckoUtils::ScrollElementIntoView(nsIDOMElement*)’: /Users/smokey/Camino/dev/18branch/CaminoBranch/camino/src/extensions/GeckoUtils.cpp:393: error: ‘class nsDerivedSafe<nsIDocument>’ has no member named ‘GetPrimaryShell’ /Users/smokey/Camino/dev/18branch/CaminoBranch/camino/src/extensions/GeckoUtils.cpp:397: error: ‘class nsDerivedSafe<nsIPresShell>’ has no member named ‘ScrollContentIntoView’ I suspect this means the 1.8 branch version of some Gecko file is missing stuff that we're calling now. This part is beyond my ability to track down and/or guess a fix, so if you could have a look (and generate a branch version of the patch to account for the differences, if it's easily fixable), that would be helpful.

Smokey Ardisson (offline for a while; not following bugs - do not email)

Updated

•

18 years ago

Status: NEW → ASSIGNED

Keywords: checkin-needed

Mark Mentovai

•

18 years ago

Attached patch 1.8 Branch Patch (obsolete) — Details — Splinter Review

Thanks Stuart and Smokey for digging into this while I was out. I'm adding a patch with the changes you mentioned. I also tested it out and it works (*phew*). The alignment of the popup was a bit off so I tweaked that but it now looks better than the trunk version. Oh, Smokey, in your comment #107, -DNO_NSPR_10_SUPPORT is the correct fix (see #83) This should work but let me know if something else comes up.

Attachment #298218 - Flags: superreview+

Attachment #298218 - Flags: review+

Bryan Atwood

Assignee

Comment 112

•

18 years ago

Attached patch 1.8 Branch Patch (obsolete) — Details — Splinter Review

Darn it, there was an extra semi-colon in GeckoUtils::ScrollElementIntoView(). Use this patch instead.

Attachment #298218 - Attachment is obsolete: true

Attachment #298224 - Flags: superreview+

Attachment #298224 - Flags: review+

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 113

•

18 years ago

Attached file Crash logs from the branch — Details

I checked this in today without testing first by accident, and when I tested immediately upon realizing my mistake, I was getting persistent crashes or hangs on the branch and on the trunk that went away as soon as I backed the patch out locally, so I backed the patch out of the tree. I grabbed the cb-x1 tinderbuild that got built with the patch in and I'll verify with it locally later. STR were pretty much: 1. Navigate to https://mail.google.com 2. Approve access to Safari passwords 3. Switch between accounts a few times by typing in the password field 4. Do something else (click on history menu, try to focus the location bar with cmd-l, close the window, close the window and open a new one....)

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 114

•

18 years ago

Attached file trunk crash/hang logs — Details

Smokey Ardisson (offline for a while; not following bugs - do not email)

Updated

•

18 years ago

Blocks: 413400

Bryan Atwood

Assignee

Comment 115

•

18 years ago

Attached patch Trunk Patch for #112/113 — Details — Splinter Review

Smokey, sorry about that. I noticed the crashing, but thought it was not my code. I found in KeychainAutoCompleteSession that the KeychainAutoCompleteDOMListener wasn't ADDREF'd on creation. This patch fixes that and I haven't had any crashes.

Attachment #298049 - Attachment is obsolete: true

Attachment #298368 - Flags: superreview+

Attachment #298368 - Flags: review+

Bryan Atwood

Assignee

Comment 116

•

18 years ago

•

18 years ago

Direct addref'ing for ObjC members is standard practice for our codebase. We can re-examine that on a project-wide level if we want to post-1.6, but that's well beyond the scope of this patch. I don't have a strong feeling one way or the other about using void*, but it's certainly not critical for this patch.

Mark Mentovai

Comment 123

•

18 years ago

I agree with Stuart. Using AddRef (probably via NS_IF_ADDREF) on XPCOM objects owned by Objective-C objects, and storing them as raw pointers instead of nsCOMPtrs, is our current standard. We can look to changing this, but it should be done throughout the entire codebase. There's no reason to hold this patch up for that. Also, I'd advise against using void* unless you really don't know anything about the type of an object. If what we have will always be an nsIDOMHTMLInputElement*, then call it an nsIDOMHTMLInputElement*, even if we currently only care about pointer comparisons. On the other hand, in this specific instance, it may make sense to add a getter to the C++ class, and to have the Objective-C class call the C++ class' getter when it needs the element instead of keeping its own cached copy.

Smokey Ardisson (offline for a while; not following bugs - do not email)

Comment 124

•

18 years ago

Third time's the charm, right? Landed on the trunk and the MOZILLA_1_8_BRANCH in advance of 1.6b3. Please file any issues in new bugs. Bryan, thanks so much for doing all this work and bearing with the length of the process on this one.

Status: ASSIGNED → RESOLVED

Closed: 18 years ago

Keywords: fixed1.8.1.12

Resolution: --- → FIXED

Mark Mentovai

Comment 125

•

18 years ago

I had to check in a subsequent bustage fix for gcc 3.3 (used by the ppc build on the 1.8 branch - but not for long?) due to "jump to case label crosses initialization" warnings. Warnings are treated as errors now that we have -Werror enabled. This is a diff -w, it masks the indentation change: http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Fcamino%2Fsrc%2Fformfill&file=KeychainService.mm&rev1=1.32&rev2=1.33&whitespace_mode=ignore&diff_mode=context

Smokey Ardisson (offline for a while; not following bugs - do not email)

Updated

•

18 years ago

Blocks: 413602

Smokey Ardisson (offline for a while; not following bugs - do not email)

Updated

•

18 years ago

Depends on: 413768

Smokey Ardisson (offline for a while; not following bugs - do not email)

Updated

•

18 years ago

Depends on: 413826

Pascal

Comment 126

•

18 years ago

Sorry for this useless post, but: YOU GUYS RULE! Thank you so much for this, works like a charm so far! beer for everyone!

First patch: Work in progress 19 years ago Bryan Atwood 47.41 KB, patch		Details \| Diff \| Splinter Review
First pass of form fill classes 19 years ago Bryan Atwood 10.83 KB, application/zip		Details
First patch w/additional file 19 years ago Bryan Atwood 48.94 KB, application/octet-stream		Details
First patch w/patch selected 19 years ago Bryan Atwood 48.94 KB, patch		Details \| Diff \| Splinter Review
First patch for review 18 years ago Bryan Atwood 34.83 KB, patch		Details \| Diff \| Splinter Review
Form fill files for review 18 years ago Bryan Atwood 11.65 KB, application/zip		Details
Combined patch for first review 18 years ago Bryan Atwood 71.09 KB, patch	stuart.morgan+bugzilla : review-	Details \| Diff \| Splinter Review
Patch for comments #72,73 18 years ago Bryan Atwood 75.31 KB, patch		Details \| Diff \| Splinter Review
Patch for #72,73 that adds missing files 18 years ago Bryan Atwood 82.02 KB, patch	stuart.morgan+bugzilla : review-	Details \| Diff \| Splinter Review
Patch for comments in #77 18 years ago Bryan Atwood 81.86 KB, patch	stuart.morgan+bugzilla : review-	Details \| Diff \| Splinter Review
Patch for comments in #83 18 years ago Bryan Atwood 87.54 KB, patch	stuart.morgan+bugzilla : review+	Details \| Diff \| Splinter Review
Patch for comments in #88 / #89 18 years ago Bryan Atwood 90.72 KB, patch	batwood.bugs : review+ mark : superreview-	Details \| Diff \| Splinter Review
Patch for comments #92/93 18 years ago Bryan Atwood 93.77 KB, patch	batwood.bugs : review+	Details \| Diff \| Splinter Review
Update to patch for comments #92/93 18 years ago Bryan Atwood 94.15 KB, patch	batwood.bugs : review+ mark : superreview+	Details \| Diff \| Splinter Review
Patch for comment #104 18 years ago Bryan Atwood 93.96 KB, patch	batwood.bugs : review+ batwood.bugs : superreview+	Details \| Diff \| Splinter Review
Problem #3 18 years ago Smokey Ardisson (offline for a while; not following bugs - do not email) 6.38 KB, text/plain		Details
1.8 Branch Patch 18 years ago Bryan Atwood 94.06 KB, patch	batwood.bugs : review+ batwood.bugs : superreview+	Details \| Diff \| Splinter Review
1.8 Branch Patch 18 years ago Bryan Atwood 94.05 KB, patch	batwood.bugs : review+ batwood.bugs : superreview+	Details \| Diff \| Splinter Review
Crash logs from the branch 18 years ago Smokey Ardisson (offline for a while; not following bugs - do not email) 37.79 KB, text/plain		Details
trunk crash/hang logs 18 years ago Smokey Ardisson (offline for a while; not following bugs - do not email) 289.12 KB, text/plain		Details
Trunk Patch for #112/113 18 years ago Bryan Atwood 93.97 KB, patch	batwood.bugs : review+ batwood.bugs : superreview+	Details \| Diff \| Splinter Review
Branch Patch for #112/113 18 years ago Bryan Atwood 94.08 KB, patch	batwood.bugs : review+ batwood.bugs : superreview+	Details \| Diff \| Splinter Review