Closed Bug 879137 Opened 7 years ago Closed 4 years ago

Move non-nsNSSComponent stuff out of nsNSSComponent.cpp

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: briansmith, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

I'm trying to clean up nsNSSComponent.cpp to make it easier to understand (especially startup/shutdown) by moving everything that isn't part of nsNSSComponent out of the source file.
Attachment #757801 - Flags: superreview?(honzab.moz)
Attachment #757801 - Flags: review?(cviecco)
Comment on attachment 757801 [details] [diff] [review]
Part 1: Move nsCryptoHash and nsCryptoHMAC to their own source file

Review of attachment 757801 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me
Attachment #757801 - Flags: review?(cviecco) → review+
Comment on attachment 757801 [details] [diff] [review]
Part 1: Move nsCryptoHash and nsCryptoHMAC to their own source file

Honza, I know you are busy with the cache rewrite. This is just trivial code moving and cviecco already reviewed it. That's good enough.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c401b8894929
Attachment #757801 - Flags: superreview?(honzab.moz)
Whiteboard: [leave open]
Content Listeners are registered using a declarative mechanism that you can see here:
http://hg.mozilla.org/mozilla-central/annotate/36da3cb92193/security/manager/ssl/src/nsNSSModule.cpp#l334

I manually verified that the content listeners still work by clicking on the DER-encoded root cert at http://www.cacert.org/?id=3.
Attachment #763383 - Flags: review?(dkeeler)
This just moves PSMContentListener to its own source file + moves PSMContentDownloader to that same source file.

I did not make any changes to the implementation of either class.

Move that Part 2 and Part 3 are applied on top of the patch for bug 867465.
Attachment #763384 - Flags: review?(dkeeler)
This is basically just moving all the smart card event logic from nsNSSComponent.cpp to nsSmartCardMonitor.cpp. However, I made the things that were previously methods of nsNSSComponent static top-level functions, and/or inlined them into other functions.
Attachment #763389 - Flags: review?(mhamrick)
Comment on attachment 763383 [details] [diff] [review]
Part 2: Remove unnecessary manual registration of PSM content listeners

Review of attachment 763383 [details] [diff] [review]:
-----------------------------------------------------------------

Ok - assuming the registration works how I guess it must work for this to work, I think this works.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2098,5 @@
>  
>  uint32_t
>  getPSMContentType(const char * aContentType)
>  { 
> +  // Don't forget to update the registration of content listeners in nsNSSModule.cpp 

Maybe get rid of the trailing space while you're here.
Also, I assume you're talking about kNSSCategories? Maybe it would be helpful to mention this specifically.
Attachment #763383 - Flags: review?(dkeeler) → review+
Comment on attachment 763384 [details] [diff] [review]
Part 3: Move PSMContentListener to its own source file

Review of attachment 763384 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: security/manager/ssl/src/PSMContentListener.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef mozilla_psm_PSMCOntentListener_h_
> +#define mozilla_psm_PSMCOntentListener_h_

Probably don't want these "O"s capitalized: mozilla_psm_PSMContentListener_h_

@@ +34,2 @@
>  
> +#endif // mozilla_psm_PSMCOntentListener_h

capitalized O here again

::: security/manager/ssl/src/moz.build
@@ +65,5 @@
>      'nsSSLStatus.cpp',
>      'nsStreamCipher.cpp',
>      'nsTLSSocketProvider.cpp',
>      'nsUsageArrayHelper.cpp',
> +	'PSMContentListener.cpp',

Looks like there's some tab/space inconsistency here - I think the rest of the lines use spaces.
Attachment #763384 - Flags: review?(dkeeler) → review+
Comment on attachment 763383 [details] [diff] [review]
Part 2: Remove unnecessary manual registration of PSM content listeners

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9980cb3db2
Attachment #763383 - Flags: checkin+
Attachment #763389 - Flags: review?(mhamrick) → review?(dkeeler)
Comment on attachment 763389 [details] [diff] [review]
Part 4: move DOM smartcard insertion/removal event code out of nsNSSComponent.cpp

Review of attachment 763389 [details] [diff] [review]:
-----------------------------------------------------------------

Ok - as a code move with some slight modifications, this looks good to me.
One thing is there are a few trailing spaces in what got moved. Do you have a strong preference for fixing those or not?

::: security/manager/ssl/src/nsSmartCardMonitor.cpp
@@ +17,5 @@
> +
> +#include "mozilla/unused.h"
> +#include "GeneratedEvents.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsThreadUtils.h"

So, technically this doesn't follow the code style guidelines for the order of #includes, but what you're doing here makes sense, so I think it's not a big deal.

@@ +258,5 @@
> +
> +// ISuuports implementation for nsTokenEventRunnable
> +NS_IMPL_THREADSAFE_ISUPPORTS1(nsTokenEventRunnable, nsIRunnable)
> +
> +nsTokenEventRunnable::nsTokenEventRunnable(const nsAString &aType, 

nit: whitespace

@@ +367,5 @@
> +  if (!domWin) {
> +    return NS_OK;
> +  }
> +
> +  // first walk the children and dispatch their events 

nit: whitespace (looks like these were copied from the original - I understand not wanting to change anything in the move, but since we're touching every line here, it would be a shame not to fix these while we're here...)
Attachment #763389 - Flags: review?(dkeeler) → review+
Assignee: brian → nobody
I think the final patch was obviated by bug 1038913, so this can finally be closed.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.