1007604 - do not use nsVoidArray in nsViewManager

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Arpad Borsos [:Swatinem]]

Attachment: viewmanagerarray.patch

Comment 1

[Arpad Borsos [:Swatinem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4eabc2a942a3

Comment 2

[Arpad Borsos [:Swatinem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f40a93e56161

-  int32_t index;
+  uint32_t index;
   for (index = 0; index < gViewManagers->Length(); index++) {
     nsViewManager* vm = gViewManagers->ElementAt(index);

Sorry about that :-(

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4eabc2a942a3
https://hg.mozilla.org/mozilla-central/rev/f40a93e56161

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8419328 [details] [diff] [review]
viewmanagerarray.patch

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

::: view/src/nsViewManager.cpp
@@ +1093,5 @@
>  {
>    NS_PRECONDITION(IsRootVM(), "Must be root VM for this to be called!");
>  
>    int32_t index;
> +  for (index = 0; index < gViewManagers->Length(); index++) {

Do you need to null-check gViewManagers here first? The old code effectively did.

Comment 5

[Arpad Borsos [:Swatinem]]

Attachment: nullcheckviewmanager.patch

(In reply to Jonas Sicking (:sicking) from comment #4)
> Do you need to null-check gViewManagers here first? The old code effectively
> did.

Good catch.

Although I’m not sure that will ever happen. As long as there is a `nsViewManager` alive, `gViewManagers` should always exist. Unless something very weird is happening?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8420399 [details] [diff] [review]
nullcheckviewmanager.patch

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

r=me either way.

::: view/src/nsViewManager.cpp
@@ +1092,5 @@
>  nsViewManager::CallWillPaintOnObservers()
>  {
>    NS_PRECONDITION(IsRootVM(), "Must be root VM for this to be called!");
>  
> +  if (NS_WARN_IF(gViewManagers == nullptr)) {

I prefer !gViewManagers. I even think that's in our coding guidelines.

Comment 7

[Arpad Borsos [:Swatinem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebdfa9cb40f5

!gViewManagers it is.

Comment 8

[Carsten Book [:Tomcat]]

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