330534 - In Cookies list, session cookies show as expiring on current date

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Chris Lawson (gone)]

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 1

[Chris Lawson (gone)]

Note to self: useful link

http://developer.apple.com/documentation/Cocoa/Conceptual/DataFormatting/Articles/CreatingACustomFormatter.html

Comment 2

[Chris Lawson (gone)]

Attachment: code changes

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

Nib change coming soon.

Comment 3

[Chris Lawson (gone)]

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

[Chris Lawson (gone)]

Attachment: nib file with NSDateFormatter removed from table

Here's the nib.

Comment 5

[Sean Murphy]

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

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

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

[Chris Lawson (gone)]

(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

[Stuart Morgan]

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

[Mike Pinkerton (not reading bugmail)]

looks ok to me, i was going to make the exact same comments smorgan did.

Comment 10

[Chris Lawson (gone)]

Attachment: review comments addressed

Comment 11

[Stuart Morgan]

Comment on attachment 249027 [details] [diff] [review]
review comments addressed

sr=smorgan

Comment 12

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH

Comment 13

[Wevah (Nate Weaver)]

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

[Wevah (Nate Weaver)]

s/showing up in the prefpane/showing up in the table/

Comment 15

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

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

[Chris Lawson (gone)]

(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

[froodian (Ian Leue)]

(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

[Stuart Morgan]

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

[Wevah (Nate Weaver)]

(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

[Wevah (Nate Weaver)]

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

[froodian (Ian Leue)]

Checked in smorgan's fix.

Comment 22

[froodian (Ian Leue)]

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

[Wevah (Nate Weaver)]

If it works, I'm for it; I'm still obviously not paying very good attention to the patches in this bug.

Comment 24

[Stuart Morgan]

(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

[Wevah (Nate Weaver)]

Right. My confusion here is my fault. Carry on.