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

RESOLVED FIXED

Status

Camino Graveyard
Preferences
--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Chris Lawson (gone), Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.1})

Details

(Whiteboard: strings change in comment 3)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Note to self: useful link

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

Comment 2

11 years ago
Created attachment 248769 [details] [diff] [review]
code changes

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

Nib change coming soon.
Attachment #248769 - Flags: review?(stridey)

Updated

11 years ago
Attachment #248769 - Flags: review?(stridey) → review?(camino)
(Assignee)

Comment 3

11 years ago
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
Status: NEW → ASSIGNED
Whiteboard: strings change in comment 3
(Assignee)

Comment 4

11 years ago
Created attachment 248771 [details]
nib file with NSDateFormatter removed from table

Here's the nib.
Attachment #248771 - Flags: review?(camino)
QA Contact: preferences

Comment 5

11 years ago
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.
Attachment #248769 - Flags: review?(camino) → review+

Updated

11 years ago
Attachment #248771 - Flags: review?(camino) → review+
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.
(Assignee)

Comment 7

11 years ago
(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

Updated

11 years ago
Attachment #248771 - Attachment is patch: false
Attachment #248771 - Attachment mime type: text/plain → application/zip

Updated

11 years ago
Attachment #248769 - Flags: review?(stuart.morgan)

Updated

11 years ago
Attachment #248769 - Flags: superreview?(mikepinkerton)

Comment 8

11 years ago
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.
Attachment #248769 - Flags: superreview?(mikepinkerton)
Attachment #248769 - Flags: review?(stuart.morgan)
looks ok to me, i was going to make the exact same comments smorgan did.
(Assignee)

Comment 10

11 years ago
Created attachment 249027 [details] [diff] [review]
review comments addressed
Attachment #248769 - Attachment is obsolete: true
Attachment #249027 - Flags: superreview?(stuart.morgan)

Comment 11

11 years ago
Comment on attachment 249027 [details] [diff] [review]
review comments addressed

sr=smorgan
Attachment #249027 - Flags: superreview?(stuart.morgan) → superreview+
Whiteboard: strings change in comment 3 → [needs checkin] strings change in comment 3

Comment 12

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Whiteboard: [needs checkin] strings change in comment 3 → strings change in comment 3

Comment 13

11 years ago
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

11 years ago
s/showing up in the prefpane/showing up in the table/
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.
(Assignee)

Comment 16

11 years ago
(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

11 years ago
(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

11 years ago
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

11 years ago
(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

11 years ago
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

11 years ago
Checked in smorgan's fix.

Comment 22

11 years ago
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

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

Comment 24

11 years ago
(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

11 years ago
Right. My confusion here is my fault. Carry on.
You need to log in before you can comment on or make changes to this bug.