Last Comment Bug 330534 - In Cookies list, session cookies show as expiring on current date
: In Cookies list, session cookies show as expiring on current date
Status: RESOLVED FIXED
strings change in comment 3
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Preferences (show other bugs)
: unspecified
: PowerPC Mac OS X
-- minor (vote)
: ---
Assigned To: Chris Lawson (gone)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-14 22:30 PST by Chris Lawson (gone)
Modified: 2006-12-20 14:22 PST (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
code changes (5.39 KB, patch)
2006-12-15 13:08 PST, Chris Lawson (gone)
murph: review+
Details | Diff | Splinter Review
nib file with NSDateFormatter removed from table (20.36 KB, application/zip)
2006-12-15 13:15 PST, Chris Lawson (gone)
murph: review+
Details
review comments addressed (4.09 KB, patch)
2006-12-18 12:09 PST, Chris Lawson (gone)
stuart.morgan+bugzilla: superreview+
Details | Diff | Splinter Review

Description User image Chris Lawson (gone) 2006-03-14 22:30:08 PST
We should probably display "until quit" or something similar in the "Expires" column in the cookie list (in the Privacy pref pane), rather than the current date. Yay, one more string for l10n to worry about.
Comment 2 User image Chris Lawson (gone) 2006-12-15 13:08:45 PST
Created attachment 248769 [details] [diff] [review]
code changes

Subclasses NSDateFormatter to spit out a custom (localised) string for the epoch.

Nib change coming soon.
Comment 3 User image Chris Lawson (gone) 2006-12-15 13:13:33 PST
Proposed strings change for the Localizable.strings file in Privacy:

"CookieExpiresOnQuit" = "on quit";

I'm not married to that, though, so if anyone has a better suggestion, feel free.

cl
Comment 4 User image Chris Lawson (gone) 2006-12-15 13:15:03 PST
Created attachment 248771 [details]
nib file with NSDateFormatter removed from table

Here's the nib.
Comment 5 User image Sean Murphy 2006-12-17 12:51:07 PST
Comment on attachment 248769 [details] [diff] [review]
code changes

Nice patch! This is a much better way to indicate and display the expiration of session cookies.

Also, I think you made a smart decision to use a NSDateFormatter subclass to accomplish this, so review+ from me along with the comments below:

+@interface CookieDateFormatter(Private)
+
+- (NSString*)stringForObjectValue:(id)anObject;
+- (BOOL)getObjectValue:(id*)anObject forString:(NSString*)string errorDescription:(NSString**)error;
+- (NSAttributedString*)attributedStringForObjectValue:(id)anObject withDefaultAttributes:(NSDictionary*)attributes;
+
+@end

These methods are all declared by the NSFormatter parent class so we're not required to explicitly show them to the compiler again, since it is already aware of the return types and arguments.  Also, I think declaring private methods as part of a category for a given class is really only useful when the interface and implementation are split up into separate files.  In such a case, you'd typically place the private category declarations inside the implementation file, thus hiding those methods from anyone looking at or #importing the header for your class.  It is arguable, though, that even in cases where everything is inside the same file, moving methods into a category still adequately isolates them from the public class methods.

[anObject isKindOfClass:[NSDate class]]

I'd say this is fine here, but should respondsToSelector: be used here?  I'm always a little cautious to use this kind of coupling in such a polymorphic language.  Nevertheless, subclasses of NSDate (such as NSCalendarDate) will, IIRC, be fine here.  No need to change this, it's more of a question I'm curious about myself.  What does everyone recommend?

Sorry for the criticisms cl :). Excellent work on the patch otherwise.  I'll be glad to see this code checked in.
Comment 6 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-12-17 14:12:06 PST
Does this patch have any bearing on bug 335985, or are you just touching things if they're session cookies?

I think we also want "On Quit" (caps) or something.
Comment 7 User image Chris Lawson (gone) 2006-12-17 14:51:37 PST
(In reply to comment #6)
> Does this patch have any bearing on bug 335985, or are you just touching things
> if they're session cookies?

It doesn't have any direct bearing on that bug, but thanks for pointing that out. I may be able to fix that once this lands (since a fix for that bug prior to this landing would be broken by this patch).

> I think we also want "On Quit" (caps) or something.

Sure. Like I said, whatever. :)

cl
Comment 8 User image Stuart Morgan 2006-12-17 19:08:30 PST
Comment on attachment 248769 [details] [diff] [review]
code changes

>+@interface CookieDateFormatter : NSDateFormatter
>+{
>+  // custom formatter for cookies list to handle session cookie expiration sanely
>+}
>+@end

Comment just before the @interface, rather than in the member declaration section (thanks for commenting it!)

>+@interface CookieDateFormatter(Private)
>+
>+- (NSString*)stringForObjectValue:(id)anObject;
>+- (BOOL)getObjectValue:(id*)anObject forString:(NSString*)string errorDescription:(NSString**)error;
>+- (NSAttributedString*)attributedStringForObjectValue:(id)anObject withDefaultAttributes:(NSDictionary*)attributes;
>+
>+@end

Yeah, overridden methods shouldn't be redeclared.

>+- (BOOL)getObjectValue:(id*)anObject forString:(NSString*)string errorDescription:(NSString**)error
>+{
>+  return [super getObjectValue:anObject forString:string errorDescription:error]; 
>+}
>+
>+- (NSAttributedString*)attributedStringForObjectValue:(id)anObject withDefaultAttributes:(NSDictionary*)attributes
>+{
>+  return [super attributedStringForObjectValue:anObject withDefaultAttributes:attributes];
>+}

Remove these entirely, as they don't do anything; that's the beauty of subclassing.


Pink has enough on his plate at the moment, so just respin with those changes and target me for sr.
Comment 9 User image Mike Pinkerton (not reading bugmail) 2006-12-18 09:52:15 PST
looks ok to me, i was going to make the exact same comments smorgan did.
Comment 10 User image Chris Lawson (gone) 2006-12-18 12:09:45 PST
Created attachment 249027 [details] [diff] [review]
review comments addressed
Comment 11 User image Stuart Morgan 2006-12-18 13:54:30 PST
Comment on attachment 249027 [details] [diff] [review]
review comments addressed

sr=smorgan
Comment 12 User image froodian (Ian Leue) 2006-12-18 16:10:35 PST
Checked in on trunk and MOZILLA_1_8_BRANCH
Comment 13 User image Wevah 2006-12-19 18:46:04 PST
I just noticed that the localized string key and not the value is showing up in the prefpane. We can't use NSLocalizedString() here; we need to use -[PreferencePaneBase getLocalizedString:] .
Comment 14 User image Wevah 2006-12-19 18:46:53 PST
s/showing up in the prefpane/showing up in the table/
Comment 15 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-12-19 18:58:38 PST
What if we put the string in the prefPane's own localizable.string?  iirc, that's what we're supposed to do (anyway) for prefPane strings.
Comment 16 User image Chris Lawson (gone) 2006-12-19 19:12:00 PST
(In reply to comment #13)
> I just noticed that the localized string key and not the value is showing up in
> the prefpane. We can't use NSLocalizedString() here; we need to use
> -[PreferencePaneBase getLocalizedString:] .

Which we can't use, because CookieDateFormatters don't store their parent prefpane as a member variable, and as they aren't a subclass of PreferencePaneBase, they know nothing about the |getLocalizedString:| method.

How do we want to solve this?

cl
Comment 17 User image froodian (Ian Leue) 2006-12-19 19:20:43 PST
(In reply to comment #15)
> What if we put the string in the prefPane's own localizable.string?  iirc,
> that's what we're supposed to do (anyway) for prefPane strings.
> 

To be clear, it *is* currently in the prefPane's own localizable.strings (per comment 3).  According to cl, it will work properly with the current implementation if put in the app's strings.  We can certainly do that, but we should discuss whether that's a tenable long-term solution or not.
Comment 18 User image Stuart Morgan 2006-12-19 19:51:10 PST
The right way to do this is to replace that line with:
  return NSLocalizedStringFromTableInBundle(@"CookieExpiresOnQuit", nil, [NSBundle bundleForClass:[self class]], nil);
I don't have time to check that in tonight, but sr=smorgan for someone else to do so.
Comment 19 User image Wevah 2006-12-20 00:27:37 PST
(In reply to comment #18)
> The right way to do this is to replace that line with:
>   return NSLocalizedStringFromTableInBundle(@"CookieExpiresOnQuit", nil,
> [NSBundle bundleForClass:[self class]], nil);
> I don't have time to check that in tonight, but sr=smorgan for someone else to
> do so.

That's exactly what PreferencePaneBase's -getLocalizedString: does, though.
Comment 20 User image Wevah 2006-12-20 00:30:51 PST
What CL said is true; I wasn't paying attention and didn't notice that this was in a formatter subclass.

I think that since this class could conceivably be used in places other than the preference pane, the string could go in the app's Localizable.strings.
Comment 21 User image froodian (Ian Leue) 2006-12-20 00:47:30 PST
Checked in smorgan's fix.
Comment 22 User image froodian (Ian Leue) 2006-12-20 00:49:08 PST
Hmm, sorry, I missed that there was still conversation about whether this was necessarily what we wanted.  I guess we can always change it later if we want to use it somewhere else in the app.
Comment 23 User image Wevah 2006-12-20 01:04:11 PST
If it works, I'm for it; I'm still obviously not paying very good attention to the patches in this bug.
Comment 24 User image Stuart Morgan 2006-12-20 07:33:38 PST
(In reply to comment #19)
> That's exactly what PreferencePaneBase's -getLocalizedString: does, though.

Right, which is what we wanted; we just didn't have any way to call it.

(In reply to comment #20)
> I think that since this class could conceivably be used in places other than
> the preference pane, the string could go in the app's Localizable.strings.

Right now it's embedded in the privacy pref pane code; if we ever needed it elsewhere we'd have to move the code anyway, so we could move the string at the same time.
Comment 25 User image Wevah 2006-12-20 14:22:58 PST
Right. My confusion here is my fault. Carry on.

Note You need to log in before you can comment on or make changes to this bug.