495496 - change the appearance of the autocomplete popup window

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, enhancement)

Description

[Dan Weber]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_7; en-us) AppleWebKit/528.16 (KHTML, like Gecko) Version/4.0 Safari/528.16
Build Identifier: 2.0b3pre (1.9.0.12pre 2009052216)

Switched from using NSWindow for the autocomplete popup window to using MAAttachedWindow, a class by Matt Gemmell (http://mattgemmell.com/source). Added a 2 more classes, AutoCompleteCell and AutoCompleteCellInfo that are used by the tableview in the AutoCompleteTextField class.

Reproducible: Always

Comment 1

[Dan Weber]

Attachment: makes changes to AutoCompleteDataSource, AutoCompleteTextField, and adds new classes described

Comment 2

[Dan Weber]

Attachment: the gradient needed for the highlight, will be drawn in code later

Comment 3

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

Some drive-by comments:

1) Including the required project changes in the patch is always helpful.

2) Presumably comment 2 includes switching to the appropriate color depending on theme; otherwise, there will be lots of unhappy Graphite Camino users.

3) The font size seems too small for me for the amount of text that's displayed; I found that trying to select a site really bothered me.  Everywhere else we use the small font size, it's for short runs of text; here we have not only long runs of text, but lots of them on top of each other.  I like that I'm able to see more of URLs before getting cut off, but the side effects more-or-less cancel that win out for me.

Comment 4

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

Attachment: crashlog

4) Every time I've quit with that patch applied, I crash; the stack doesn't look helpful, though, mostly OS stuff.  Removing the patch and rebuilding, I've yet to crash again.

Comment 5

[Stuart Morgan]

Comment on attachment 380492 [details] [diff] [review]
makes changes to AutoCompleteDataSource, AutoCompleteTextField, and adds new classes described

I didn't play around with this iteration, so this is just from doing a code read-through; I'll definitely give the next round a shot though :)

Looks good structurally, mostly small comments (in addition to the notes Smokey made above).

> @interface AutoCompleteDataSource : NSObject
> {
>   NSImage*      mGenericSiteIcon;     // owned
>   NSImage*      mGenericFileIcon;     // owned
>   NSString*     mErrorMessage;  // owned
>+  
>+  NSCell*       mHeaderCell;
> 
>   nsIAutoCompleteResults* mResults;   // owned
> }

While we aren't consistent about how we mark owned vs. unowned members across the codebase, we try to be consistent within existing classes at least, so mHeaderCell should be marked "owned" here. I'm confused though; what is it for? It doesn't seem to be used other that being created and released.

>-  id result = [aColumnIdentifier isEqualToString:@"icon"] ? (id)mGenericSiteIcon : (id)@"";
>+  //id result = [aColumnIdentifier isEqualToString:@"icon"] ? (id)mGenericSiteIcon : (id)@"";

We don't check in commented-out old code; that's what SCM is for ;) There's a bunch of that in this patch that needs to be removed.

>+    // set URL
>+    nsCAutoString value;
>+    item->GetURL(value);
>+    [info setSiteURL:[[NSString stringWith_nsACString:value] unescapedURI]];
>...    
>+    // set icon
>+    nsCAutoString url;
>+    item->GetURL(url);
>+    NSString* urlString = [NSString stringWith_nsACString:url];

Now that these are in the same block you can re-use the URL instead of fetching and converting it twice.

>+    if (cachedFavicon) {
>+      [info setSiteIcon:cachedFavicon];
>+    } else if ([urlString hasPrefix:@"file://"]) {
>+      [info setSiteIcon:mGenericFileIcon];
>+    }

This is missing an 'else' to set it to mGenericSiteIcon.

>+  

No whitespace on blank lines please! Needs fixing throughout the patch

>     // Create the icon column
>     column = [[[NSTableColumn alloc] initWithIdentifier:@"icon"] autorelease];
>     [column setWidth:[mProxyIcon frame].origin.x + [mProxyIcon frame].size.width];
>     dataCell = [[[NSImageCell alloc] initImageCell:nil] autorelease];
>     [column setDataCell:dataCell];
>     [column setEditable:NO];
>-    [mTableView addTableColumn: column];
>+    //[mTableView addTableColumn: column];

This code is all dead now, and needs to be removed.

>   // create the text columns

Comments that no longer apply need to be changed or removed.

>+  [tableViewContainerView addSubview:scrollView];

If there's no scrolling possible, why use a scroll view?

>+  [mPopupWin setHasArrow:NO];
>+  [mPopupWin setDrawsRoundCornerBesideArrow:YES];
>+  [mPopupWin setArrowBaseWidth:10.0];
>+  [mPopupWin setArrowHeight:10.0];

I haven't looked at the implementation; is this really all necessary after calling setHasArrow:NO?

>+  NSRect scrollViewFrame = NSZeroRect;
>+	scrollViewFrame.size.height = tableHeight;
>+	scrollViewFrame.size.width = locationFrame.size.width - (2 * kFrameMargin);
>+	scrollViewFrame.origin.y = 5.0;
>+	NSRect containerViewFrame = scrollViewFrame;
>+	containerViewFrame.size.height += 10.0;
>+	[[[mTableView enclosingScrollView] superview] setFrame:containerViewFrame];
>+	[[mTableView enclosingScrollView] setFrame:scrollViewFrame];

This section uses tabs rather than spaces. There are a few other places in the patch as well.

>+  [mPopupWin setPoint:NSMakePoint(winFrame.origin.x + locationOrigin.x + locationFrame.size.width / 2.0, winFrame.origin.y + locationOrigin.y + 19.0) side:MAPositionBottom];

|locationOrigin.x + locationFrame.size.width / 2.0| can be replaced with NSMidX(locationFrame). It would probably also be better to use convertBaseToScreen: to get screen coordinates, rather than doing the window frame math yourself. (This math is all probably not UI-scaling-safe, but IIRC the current autocomplete isn't either so we can deal with that in a later bug.)

What's the magic number 19.0?

>+  [mPopupWin windowDidResize:nil];

This feels hacky; why is it necessary?

>-    NSString *result = [mDataSource resultForRow:aRow columnIdentifier:@"col1"];
>+    NSString *result = [mDataSource resultForRow:aRow columnIdentifier:@"url"];

Asking for an imaginary column (and having the datasource support that) seems a bit odd. Why not get the info object, and ask it for the URL?

>+  int prevRow = [mTableView selectedRow];
>+  int row = prevRow;

prevRow is never used.

>+- (void)drawInteriorWithFrame:(NSRect)theCellFrame inView:(NSView *)theControlView;

No need to redeclare inherited methods.

>+#pragma mark -
>+#pragma mark AutoCompleteCell

There's nothing else in the file; why the marks?

>+  NSMutableDictionary *urlAttributes = [[[NSMutableDictionary alloc] initWithObjectsAndKeys:
>+                                         [NSColor blackColor], NSForegroundColorAttributeName,
>+                                         [NSFont systemFontOfSize:11.0], NSFontAttributeName,
>+                                         paragraphStyle, NSParagraphStyleAttributeName,
>+                                         nil] autorelease];

While we don't have a set style for this kind of thing, I find it to be far more readable when aligned on the commas:

  NSMutableDictionary *urlAttributes = [[[NSMutableDictionary alloc] initWithObjectsAndKeys:
                                                   [NSColor blackColor], NSForegroundColorAttributeName,
                                         [NSFont systemFontOfSize:11.0], NSFontAttributeName,
                                                         paragraphStyle, NSParagraphStyleAttributeName,
                                                                         nil] autorelease];

That's just a personal preference though, so if you disagree feel free to leave it as is :)

>+	NSSize urlSize = [url sizeWithAttributes:urlAttributes];
>+  NSSize titleSize = [title sizeWithAttributes:titleAttributes];

Compute these at the point where you actually need them.
  
>+    [title drawInRect:NSMakeRect(cellFrame.origin.x + 10, 
>+                                 cellFrame.origin.y + cellFrame.size.height * 0.5 - titleSize.height * 0.5,
>+                                 cellFrame.size.width - 20,
>+                                 titleSize.height)

Use constants rather than magic values (e.g., declare 'const int kHorizontalPadding = 10' above this line, then use that instead of 10 and 20). It would also add a bit of clarity if you compute an intermediate value 'kVerticalPadding = (NSHeight(cellFrame) - titeSize.height) * 0.5' and then use that in the draw call.

>+      for (int i = cellFrame.origin.x; i < (cellFrame.origin.x + cellFrame.size.width); i += gradientSize.width) {
>+        [gradient drawInRect:NSMakeRect(i, cellFrame.origin.y - 1, gradientSize.width, cellFrame.size.height + 2)
>+                    fromRect:NSMakeRect(0, 0, gradientSize.width, gradientSize.height)
>+                   operation:NSCompositeSourceOver
>+                    fraction:1.0];

This will be replaced with CHGradient, but as an FYI if you do end up needing to do something like this we have 
drawTiledInRect:origin:operation: in NSImage+Utils.

>+    [siteIcon drawInRect:NSMakeRect(cellFrame.origin.x + 10, cellFrame.origin.y, 16, 16)
>+                fromRect:NSMakeRect(0, 0, siteIcon.size.width, siteIcon.size.height) 
>+               operation:NSCompositeSourceOver 
>+                fraction:1.0];
>+    [siteIcon setFlipped:NO];
>+    NSRect urlRect = NSMakeRect(cellFrame.origin.x + 30, 
>+                                cellFrame.origin.y + cellFrame.size.height * 0.5 - urlSize.height * 0.5,
>+                                [controlView frame].size.width / 2.0 - cellFrame.origin.x - 30,
>+                                urlSize.height);
>+    [url drawInRect:urlRect withAttributes:urlAttributes];
>+    NSRect titleRect = NSMakeRect(urlRect.origin.x + urlRect.size.width + 5, 
>+                                  cellFrame.origin.y + cellFrame.size.height * 0.5 - titleSize.height * 0.5,
>+                                  [controlView frame].size.width - urlRect.origin.x - urlRect.size.width - 10,
>+                                  titleSize.height);

30, 10, and 16 are all magic values that should get constants, and the same note about the vertical padding applies here. You should also be consistent about dividing by 2.0 or multiplying by 0.5.

I'm confused by the two width calculations. You are subtracting absolute coordinate values (origin.x) from a width, which feels wrong. Perhaps you could generate two rects with NSDivideRect and then pad from there, to make the intent of the calculations more obvious?

>+    mIsHeader = NO;

No need to init BOOL members to NO; that's done automagically.

>+  [copy setSiteIcon:[mSiteIcon retain]];

This leaks the icon.

>+- (void)setSiteIcon:(NSImage *)anImage {
>+  if (anImage != mSiteIcon) {
>+    [mSiteIcon release];
>+    mSiteIcon = [anImage retain];
>+  }
>+}
>+
>+- (NSImage *)siteIcon {
>+  return mSiteIcon;
>+}

You should use either technique 1 or 2 from
http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAccessorMethods.html
for all of the accessor pairs. This version makes hard-to-see bugs easy to write, so we avoid it unless there's a compelling performance reason.

>+    mSiteURL = [aURL retain];

You generally want to use copy for string properties (probably doesn't matter here, but it's a good habit).

Comment 6

[Dan Weber]

Attachment: Patch 2

Should fix a bunch of problems with the previous patch. Highlight is drawn, uses appropriate tint (no longer need the image file). Shouldn't crash on quit anymore. Played around with the font sizes a little.

Comment 7

[Dan Weber]

Attachment: Patch 3

Patch 3 is basically the same as Patch 2 except that I have added little scroll arrows to the window to show that there are more results (like a menu). The problem is that I haven't quite figured out how to redraw them correctly when the selection is changed by dragging (as opposed to the arrow keys). Suggestions welcome...

Comment 8

[Stuart Morgan]

Comment on attachment 380965 [details] [diff] [review]
Patch 2

This looks really nice :) It's hard to go back to the old autocomplete list now.

A couple of general style notes before I forget:
- The blank lines in this patch still have whitespace.
- We prefer new comments be full sentences, with capitalization and a period (I know that will be inconsistent with some of the rest of the code, but since the existing codebase is already inconsistent we don't worry about it and just apply it to new/changed comments).


>   if (!item)
>+    return info;
>+  else {

Our style is not to do else after an if that returns (especially when it's for handling an error-like condition, since it obfuscates the core logic of the method as nesting increases).

>+    // set URL
>     nsCAutoString value;
>     item->GetURL(value);
>+    [info setSiteURL:[[NSString stringWith_nsACString:value] unescapedURI]];
>...
>+    NSString* urlString = [NSString stringWith_nsACString:value];

This string is still being calculated twice; move this up and use it in the setSiteURL: call.

>+  NSImageView*              mUpArrowImageView;
>+  NSImageView*              mDownArrowImageView;

I thought the plan was to try capping the result list and seeing how well that worked? It's probably best to start from there, and then only add the scrolling (in a later bug) if we find we need it.

>+  NSScrollView *scrollView = [[[NSScrollView alloc] initWithFrame:NSMakeRect(0,0,0,0)] autorelease];

NSZeroRect

>+  NSView* tableViewContainerView = [[[NSView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)] autorelease];

Why 100 x 100 here, but 0 x 0 for the other view?

>+  mPopupWin = [[[MAAttachedWindow alloc] initWithView:tableViewContainerView
>+                                     attachedToPoint:NSZeroPoint
>+                                            inWindow: [self window]

:-alignment is off by one, and there's a stray space after inWindow:

>+                                          atDistance:8.0] retain];

Use a descriptive constant rather than a magic number. (Also, the retain on an alloc'd window is leaking the window).

Also as a more general note, this method is pretty long; it might be nice to split out some of the construction into helper methods (e.g., an autocompleteWindow method that just creates and configures the MAAttachedWindow, so you can just do |mPopWin = [[self autocompleteWindow] retain]| here without the details of the configuration.

>+  scrollViewFrame.origin.y = 5.0;
>...
>+  containerViewFrame.size.height += 10.0;

These magic values need constants (well, I'm guessing it's really just magic value, doubled the second time for top and bottom padding).

>+  [mPopupWin setPoint:NSMakePoint(locationOrigin.x + NSMidX(locationFrame), locationOrigin.y) side:MAPositionBottom];

The fact that locationOrigin and locationFrame are in different coordinate systems is confusing (I had to go back and read the full code in context to understand why locatioOrigin.x + NSMidX(locationFrame) wasn't wrong, since logically it should just be the NSMidX). How about just converting the whole frame's coordinates rather that the origin, and getting everything you need (origin, size, etc.) from that one variable instead of having two that can be out of sync.

>+  /*if (row == [mTableView numberOfRows] - 1) {
>+    // we selected the last row, so hide the down arrow
>+    [self resizePopup:YES];
>+  }*/

Remove this.

>+                                           [NSFont systemFontOfSize:12.0], NSFontAttributeName,
>...
>+                                         [NSFont systemFontOfSize:11.0], NSFontAttributeName,

Having slightly differently sized fonts right next to each other feels really wrong to me; things that are only very slightly different in typography tend to clash. I would vote for 12, since I think the "smaller text is harder to skim" trumps "slightly more text available".

>+    // we're highlighted, so draw selection gradient

I'd suggest moving the whole construction and drawing of the highlight gradient into a helper method, since it's an easily separable block (and we may eventually want to cache some of the color/gradient info for the life of the cell, so it would be good to isolate that). Also, my recollection is that the gradient-ness was new in Leopard, so we'll need to get values for Tiger as well.

>+  titleTextRect.origin.x += kIntercellHorizontalPadding;
>+  titleTextRect.size.width -= kIntercellHorizontalPadding;
>+  urlTextRect.origin.x += kIntercellHorizontalPadding;
>+  urlTextRect.size.width -= kIntercellHorizontalPadding;

NSInsetRect?

The whole calculation of titleTexRect and urlTextRect would be another good candidate for moving to a helper method (it can take the initial rect and two rect pointers--to be filled in with the final values, as with NSDivideRect--as arguments).

>+  //urlTextRect.origin.y += [title sizeWithAttributes:titleAttributes].height - [url sizeWithAttributes:urlAttributes].height;

Remove.

Comment 9

[Stuart Morgan]

One more note, captured from irc discussion: for title we probably want NSLineBreakByTruncatingMiddle (since sometimes there's a bunch of "boilerplate" at the beginning of all the pages on a site).

We should probably look at URL in a later bug; middle might work, but might stomp domain, which is mostly likely to be useful to users; we may want something smart that will elide from the end of the domain to the end (e.g., http://foo.longdomainname.com/...blarg.html)

Comment 10

[Dan Weber]

Attachment: Patch 4

More minor fixes

Comment 11

[Stuart Morgan]

Comment on attachment 381908 [details] [diff] [review]
Patch 4

Looks like one more round of changes, then we can send it off for sr :)

I should have mentioned when I commented about whitespace that we prefer not to mix mass-whitespace editing of a file with actual code changes, since it reduces the usefulness of cvs blame (instead we either do a patch with just whitespace changes, or we just fix up places in or immediately around where we are touching). You don't need to go through the tedious process of removing the whitespace changes from your patch; just something to keep in mind for future reference.

A general comment: be sure to review all your new comments and turn them into full sentences for the next spin; many are still fragments.

>-@end
>+@end
>\ No newline at end of file

Can you make sure your version of this file has a newline? (It may already; I can never tell from the diff output if it's complaining about the old file, the new file, or both).

>+const float kPopupWindowDistanceFromPoint = 8.0;

Move this to just above the line that actually uses it, since it's only used there. Also, make the name more descriptive (kPopupWindowOffsetBelowLocationField maybe, if I'm understanding the effect correctly)

>-  nsCOMPtr<nsIAutoCompleteSession> session =
>-    do_GetService(NS_GLOBALHISTORY_AUTOCOMPLETE_CONTRACTID);
>+  nsCOMPtr<nsIAutoCompleteSession> session = do_GetService(NS_GLOBALHISTORY_AUTOCOMPLETE_CONTRACTID);

Remove this change; we're trying to cut back on long lines.

>+  const float kHorizontalPadding = 5.0;
>...
>+  scrollViewFrame.origin.y = kHorizontalPadding;
>...
>+  containerViewFrame.size.height += kHorizontalPadding * 2.0;

Looks like you mean kVerticalPadding.

>+    [self setStringUndoably:[(AutoCompleteCellInfo *)[mDataSource resultForRow:[mTableView selectedRow] columnIdentifier:@"main"] siteURL] fromLocation:0];

Break this line just before the fromLocation (and :-align it with setStringUndoably:)

>+  [self selectRowAt:-1];

Can you add a comment explaining what this is for?

>+- (void)setupTextAttributesForTitle:(NSString *)title url:(NSString *)url;
>+- (void)setupColors;
>+- (void)setupTitleRect:(NSRect *)titleTextRect urlRect:(NSRect *)urlTextRect fromRect:(NSRect)textRect;

s/setup/setUp/ (setup is a noun, set up is a verb)

> +  mTitleAttributes = [[[NSMutableDictionary alloc] initWithObjectsAndKeys:...] autorelease];

All of these member objects that are set to autoreleased values are bugs waiting to happen. It only works because they are really local variables to the draw method, being "returned" from helpers as members, and thus only used there--which is sketchy. I think the best thing to do would be to refactor in ways that make them unnecessary.
- If you have a drawHighlightInRect: method, you can put the color and gradient construction in there, and eliminate the NSColor members.
- mTitleAttributes and mUrlAttributes can just become titleAttributes and urlAttributes methods that return the autoreleased dicitionary (which you can store in a local variable in the draw method, since that's the only place it's used).
- The two height members can be computed locally in the draw method, and the urlTextRect->origin.y adjustment can be done after calling the rect setup method instead of inside it (which I think is reasonable, since it's not part of the core layout logic for the column themselves)

>+  const int kHorizontalPadding = 10;
>+  const int kTextVerticalPadding = (NSHeight(cellFrame) - mTitleTextHeight) * 0.5;
>+
>+  // create "column" frames
>+  NSRect insetRect = NSInsetRect(cellFrame, kHorizontalPadding, 0);
>+  NSRect imageRect;
>+  NSRect textRect;
>+  NSDivideRect(insetRect, &imageRect, &textRect, iconSize.width, NSMinXEdge);
>+  textRect = NSInsetRect(textRect, 0, kTextVerticalPadding);
>+  NSRect titleTextRect;
>+  NSRect urlTextRect;
>+  [self setupTitleRect:&titleTextRect urlRect:&urlTextRect fromRect:textRect];

This, on the other hand, would make sense to suck into the rect helper method, so that it spits out three columns instead of two (at which point the padding variables can be contained inside that method). The only value you need that's a local is the icon size, but you could just make 16 a file-level constant that is used both to initialize iconSize and to slice a "column" out of the overall rect.


Also, we need to make sure we file a follow-up bug for making the highlight-drawing 10.4-friendly once this patch is finished.

Comment 12

[Dan Weber]

Attachment: Patch 5

Comment 13

[Stuart Morgan]

Comment on attachment 382619 [details] [diff] [review]
Patch 5

Looks great! r=me with minor nits addressed; once you've posted a version with these tweaks we'll send it off for sr.

>+	const float kPopupWindowOffsetFromLocationField = 8.0;
>+	mPopupWin = [[MAAttachedWindow alloc] initWithView:tableViewContainerView

These lines have tabs, as do a few later in the patch (in another file).

>+const NSSize kIconSize = {16, 16};

s/const/static const/


Also, this version of the patch still doesn't have the Camino.xcodeproj/project.pbxproj changes; please make sure the next version does so that it will be easier to land (if there's an issue with the patch generation, we can talk about it in IRC today).

Comment 14

[Stuart Morgan]

Sorry, one more thing that had been bothering me but I couldn't put my finger on earlier: Could you rename AutoCompleteCellInfo to something like LocationAutoCompleteResult?

It doesn't actually have anything to do with cells other than happening to be displayed by them; it's a model object, but this name makes me think it's tied to the view end of things. (And since we have other things that autocomplete, giving some specificity as to which autocompletion this is for will be helpful.)

Comment 15

[Dan Weber]

Attachment: Patch 6

Comment 16

[Stuart Morgan]

Comment on attachment 384718 [details] [diff] [review]
Patch 6

pink, note that while this looks really long, over half of it is the third-party MAAttachedWindow class.

Comment 17

[Mike Pinkerton (not reading bugmail)]

+++ src/extensions/AutoCompleteCell.m	23 Jun 2009 20:37:04 -0000
@@ -0,0 +1,108 @@

this file needs a license.

+- (NSDictionary *)titleAttributes {
+  NSMutableParagraphStyle *paragraphStyle = [[[NSMutableParagraphStyle alloc] init] autorelease];
+  [paragraphStyle setLineBreakMode:NSLineBreakByTruncatingMiddle];
+  return [[[NSMutableDictionary alloc] initWithObjectsAndKeys:

why not use [+NSMutableDictionary dictionaryWithObjectsAndKeys...]?

+- (NSDictionary *)urlAttributes {
+  NSMutableParagraphStyle *paragraphStyle = [[[NSMutableParagraphStyle alloc] init] autorelease];
+  [paragraphStyle setLineBreakMode:NSLineBreakByTruncatingTail];
+  return [[[NSMutableDictionary alloc] initWithObjectsAndKeys:

same as above.

+- (void)createImageRect:(NSRect *)imageRect titleRect:(NSRect *)titleTextRect urlRect:(NSRect *)urlTextRect fromRect:(NSRect)cellFrame {

All the methods in this file need function-level comments describing what they're used for, why they'd be called, any pre/post-conditions, any assumptions made, etc etc.

--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/extensions/AutoCompleteResult.h	23 Jun 2009 20:37:04 -0000

this file needs a license too.

+@interface AutoCompleteResult : NSObject {

this class needs a class-level comment describing what it's used for and how it's to be used.

--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/extensions/AutoCompleteResult.m	23 Jun 2009 20:37:04 -0000

license.

+- (void)dealloc {
+  [mSiteIcon release];
+  [mSiteURL release];
+  [mSiteTitle release];
+  [super dealloc];
+}

this would be really really bad if nobody calls setTitle or setSiteURL, as you'll be releasing something you have a weak-reference to. You should probably copy the strings in the init method, or handle this better.

+//
+//  MAAttachedWindow.h
+//
+//  Created by Matt Gemmell on 27/09/2007.
+//  Copyright 2007 Magic Aubergine.
+//

do we have permission to include this? What license is this under? We need to nail this down and include it in the code somehow so he doesn't turn around later and change the license on us.

It also says "You can redistribute the original, unmodified code, but you have to include the full license text below." I don't see that being done here.

There's a crapload of code here for this attached window, do we really need all of this?

Comment 18

[Stuart Morgan]

(In reply to comment #17)
> this would be really really bad if nobody calls setTitle or setSiteURL, as
> you'll be releasing something you have a weak-reference to.

It'll be fine for the same reason that setTitle and setSiteURL's autorelease works: retain counts don't matter for string constants.

> It also says "You can redistribute the original, unmodified code, but you have
> to include the full license text below." I don't see that being done here.

We are including it as part of bug 495620. I'm not sure how we want to handle the source files themselves though... reference our license.html file?

> There's a crapload of code here for this attached window, do we really need all
> of this?

Do we really want to reinvent the wheel just to save a couple hundred lines of code? We haven't worried about exactly how many lines of Breakpad or Sparkle code we do or don't use, and I'm sure that at least the latter has sections that aren't relevant to us.

Comment 19

[Dan Weber]

Attachment: Patch 7

Comment 20

[Mike Pinkerton (not reading bugmail)]

> It'll be fine for the same reason that setTitle and setSiteURL's autorelease
> works: retain counts don't matter for string constants.

Hrm. Ok. I could swear they did matter.

> We are including it as part of bug 495620. I'm not sure how we want to handle
> the source files themselves though... reference our license.html file?

Did we ever ask Gerv for what to do regarding the source files in particular?
 
> Do we really want to reinvent the wheel just to save a couple hundred lines of
> code? We haven't worried about exactly how many lines of Breakpad or Sparkle
> code we do or don't use, and I'm sure that at least the latter has sections
> that aren't relevant to us.

So the answer is "yes, we probably need it". I was just asking. It really surprised me how much new code there was there, given how little code it was replacing.

Assuming everything above has been addressed (there's no easy way to verify in such a large patch), sr=pink.

Comment 21

[Stuart Morgan]

(In reply to comment #20)
> Did we ever ask Gerv for what to do regarding the source files in particular?

We opted for just putting it all in, since it's the safest option and is consistent with all the rest of our source.

Comment 22

[Stuart Morgan]

Attachment: Patch 8

I was applying this locally and discovered that the project had bitrotted slightly. I also added a license that was missing from one of the headers, and s/2008/2009/ the licenses.

This is ready for checking in as soon as we have a branch!

Comment 23

[Stuart Morgan]

Landed on CVS trunk, with a follow-up to fix the paths in the Xcode project.