Closed Bug 1253010 Opened 4 years ago Closed 4 years ago

don't go through XPCOM to create nsIDateTimeFormat instances

Categories

(Core :: General, defect)

defect
Not set

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.