Closed
Bug 1253010
Opened 9 years ago
Closed 9 years ago
don't go through XPCOM to create nsIDateTimeFormat instances
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(3 files, 1 obsolete file)
3.29 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
9.80 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
7.66 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
Assignee | |
Comment 1•9 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•9 years ago
|
||
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)
![]() |
Assignee | |
Comment 3•9 years ago
|
||
Attachment #8725862 -
Flags: review?(smontagu)
![]() |
Assignee | |
Comment 4•9 years ago
|
||
Apparently my rebase got botched. Let's try this again; comment 1 still applies.
Attachment #8725877 -
Flags: review?(dkeeler)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8725860 -
Attachment is obsolete: true
Attachment #8725860 -
Flags: review?(dkeeler)
![]() |
||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8725862 -
Flags: review?(smontagu) → review-
![]() |
Assignee | |
Comment 7•9 years ago
|
||
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)
![]() |
Assignee | |
Comment 8•9 years ago
|
||
(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 9•9 years ago
|
||
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+
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d3bc99831ce6
https://hg.mozilla.org/mozilla-central/rev/22d7c93ec657
https://hg.mozilla.org/mozilla-central/rev/051996f60967
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
![]() |
Assignee | |
Updated•9 years ago
|
Flags: needinfo?(smontagu)
You need to log in
before you can comment on or make changes to this bug.
Description
•