Closed Bug 1253010 Opened 9 years ago Closed 9 years ago

don't go through XPCOM to create nsIDateTimeFormat instances

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
nsX509CertValidity has several copy-pasted routines that differ only slightly in the parameters they use for formatting times. Let's have a single place to do the formatting and pass in the appropriate parameters. It's not great to write functions with five parameters. OTOH, I think the callsites become clearer with this change.
Attachment #8725860 - Flags: review?(dkeeler)
To avoid hitting the component manager from the security manager, we need a way of creating nsIDateTimeFormat instances that don't go through the component manager. The only wrinkle is that these formatters are platform-dependent. Let's write a thin wrapper over nsIDateTimeFormat that abstracts away the platform details.
Attachment #8725861 - Flags: review?(smontagu)
Apparently my rebase got botched. Let's try this again; comment 1 still applies.
Attachment #8725877 - Flags: review?(dkeeler)
Attachment #8725860 - Attachment is obsolete: true
Attachment #8725860 - Flags: review?(dkeeler)
Comment on attachment 8725877 [details] [diff] [review] part 1 - refactor nsX509CertValidity time formatting Review of attachment 8725877 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable. I just have a few style nits that were present in the original code and a few suggestions for simplifications. ::: security/manager/ssl/nsNSSCertValidity.cpp @@ +47,5 @@ > return rv; > } > > +nsresult > +nsX509CertValidity::FormatTime(PRTime* aTimeDate, Maybe just make the aTimeDate parameter a reference? @@ +53,5 @@ > + const nsDateFormatSelector aDateFormatSelector, > + const nsTimeFormatSelector aTimeFormatSelector, > + nsAString& aFormattedTimeDate) > +{ > + if (!mTimesInitialized) Let's update the style while we're here: nit: braces around conditional body @@ +58,5 @@ > + return NS_ERROR_FAILURE; > + > + nsresult rv; > + nsCOMPtr<nsIDateTimeFormat> dateFormatter = > + do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID, &rv); nit: let's only indent this 2 spaces (rather than 3) @@ +59,5 @@ > + > + nsresult rv; > + nsCOMPtr<nsIDateTimeFormat> dateFormatter = > + do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID, &rv); > + if (NS_FAILED(rv)) return rv; nit: if (NS_FAILED(rv)) { return rv; } @@ +61,5 @@ > + nsCOMPtr<nsIDateTimeFormat> dateFormatter = > + do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID, &rv); > + if (NS_FAILED(rv)) return rv; > + > + nsAutoString date; Hmmm - can we not just pass aFormattedTimeDat directly to dateFormatter->FormatPRExplodedTime? @@ +64,5 @@ > + > + nsAutoString date; > + PRExplodedTime explodedTime; > + PR_ExplodeTime(*aTimeDate, aParamFn, &explodedTime); > + dateFormatter->FormatPRExplodedTime(nullptr, aDateFormatSelector, Presumably this can fail, so we should check for that. @@ +122,5 @@ > > NS_IMETHODIMP nsX509CertValidity::GetNotAfterGMT(nsAString &aNotAfterGMT) > { > + return FormatTime(&mNotAfter, PR_GMTParameters, > + kDateFormatLong, kTimeFormatSeconds, These all use kDateFormatLong, right? Seems like we could just hard-code that in FormatTime (which gets rid of a parameter).
Attachment #8725877 - Flags: review?(dkeeler) → review+
Comment on attachment 8725861 [details] [diff] [review] part 2 - provide a non-XPCOM way to create new nsIDateTimeFormat instances Review of attachment 8725861 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8725861 - Flags: review?(smontagu) → review+
Attachment #8725862 - Flags: review?(smontagu) → review-
Was attachment 8725862 [details] [diff] [review] supposed to be r+'d? It's a bit odd to get an r- without any explanation of what should be changed. :)
Flags: needinfo?(smontagu)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #5) > @@ +61,5 @@ > > + nsCOMPtr<nsIDateTimeFormat> dateFormatter = > > + do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID, &rv); > > + if (NS_FAILED(rv)) return rv; > > + > > + nsAutoString date; > > Hmmm - can we not just pass aFormattedTimeDat directly to > dateFormatter->FormatPRExplodedTime? Ah, yes we can. We should do that. > @@ +122,5 @@ > > > > NS_IMETHODIMP nsX509CertValidity::GetNotAfterGMT(nsAString &aNotAfterGMT) > > { > > + return FormatTime(&mNotAfter, PR_GMTParameters, > > + kDateFormatLong, kTimeFormatSeconds, > > These all use kDateFormatLong, right? Seems like we could just hard-code > that in FormatTime (which gets rid of a parameter). That's a good point, I'll do that.
Comment on attachment 8725862 [details] [diff] [review] part 3 - create all nsIDateTimeFormat instances directly Review of attachment 8725862 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Nathan Froyd [:froydnj] from comment #7) > Was attachment 8725862 [details] [diff] [review] supposed to be r+'d? It's > a bit odd to get an r- without any explanation of what should be changed. :) Yes it was, sorry about that.
Attachment #8725862 - Flags: review- → review+
Flags: needinfo?(smontagu)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: