1110835 - Cleanup nsSecureBrowserUIImpl

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Comment 1

[Tom S]

Attachment: v1 - Cleanup form submission checks

Comment 2

[Tom S]

Attachment: v1 - Cleanup UpdateSecurity etc.

Most of this stuff still doesn't really make sense. We actually compute parts of the security state inside MapInternalToExternalState! Having this split between internal and external isn't helping at all from my point of view. 

Do we have to use ReentrantMonitorAutoEnter, because nsSecureBrowserUIImpl is used from multiple threads?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8535668 [details] [diff] [review]
v1 - Cleanup form submission checks

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

I'm actually aiming to get rid of this code in bug 832837, so we can probably punt on cleaning this up for now.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8535672 [details] [diff] [review]
v1 - Cleanup UpdateSecurity etc.

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

Looks good. r=me with nits addressed.

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +1269,5 @@
>          temp_NewToplevelSecurityStateKnown = mNewToplevelSecurityStateKnown;
>        }
>  
>        if (temp_NewToplevelSecurityStateKnown)
> +        UpdateSecurityState(aRequest, false, false);

nit: let's add {} around this conditional body

@@ +1288,5 @@
>      NS_QueryNotificationCallbacks(channel, sink);
>  }
>  
> +void
> +nsSecureBrowserUIImpl::UpdateSecurityState(nsIRequest* aRequest, bool withNewLocation,

nit: keep lines <80 chars, please

@@ +1327,4 @@
>      mNotifiedToplevelIsEV = mNewToplevelIsEV;
>    }
>  
> +  if (flagsChanged || withNewLocation || withUpdateStatus)

nit: braces around conditional body

@@ +1339,2 @@
>    {
>      ReentrantMonitorAutoEnter lock(mReentrantMonitor);

As we discussed, the lock isn't necessary, but feel free to address that in a follow-up bug (when we remove the lock, we should assert we're on the main thread in all public functions).

@@ +1346,3 @@
>      PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>             ("SecureUI:%p: UpdateSecurityState: calling OnSecurityChange\n", this
>              ));

nit: might as well move these stray '));' to the previous line (keeping to <80 chars, though)

@@ +1346,5 @@
>      PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>             ("SecureUI:%p: UpdateSecurityState: calling OnSecurityChange\n", this
>              ));
>  
> +    toplevelEventSink->OnSecurityChange(aRequest, newState);

Should 'newState' be 'state' here?

@@ +1352,3 @@
>      PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>             ("SecureUI:%p: UpdateSecurityState: NO mToplevelEventSink!\n", this
>              ));

nit: same as with previous '));'

@@ +1446,5 @@
>      temp_NewToplevelSecurityStateKnown = mNewToplevelSecurityStateKnown;
>    }
>  
>    if (temp_NewToplevelSecurityStateKnown)
> +    UpdateSecurityState(aRequest, true, false);

nit: braces around conditional body

Comment 5

[Tom S]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8f722dd135b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a7b81c249a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3a7b81c249a