983989 - Cleanup XPCConvert::NativeData2JS

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The structure of XPCConvert::NativeData2JS is basically

switch (type.TagPart()) {
  // some cases
  default: {
    switch (type.TagPart()) {
      // some more cases
    }
  }
}

This is kinda silly.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Reindent

This makes the indentation of cases consistent, and reindents the nested switch to make the next patch easier to review.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Simplify code flow

This drops the nested switch and makes sure we return from all cases, rather than breaking to the "return true;" at the end of the function.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Cleanup

Removed the C-style casts that were silently casting away constness too, and moved some assignments out of if conditions.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8391702 [details] [diff] [review]
Part a: Reindent

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

rs=bholley assuming that this is only whitespace change.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8391703 [details] [diff] [review]
Part b: Simplify code flow

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

<3,r=bholley

::: js/xpconnect/src/XPCConvert.cpp
@@ +340,5 @@
>  
> +            return XPCVariant::VariantDataToJS(variant,
> +                                               pErr, d);
> +        }
> +        // else...

I think this comment can go.

@@ +355,1 @@
>  #endif

I have no idea what this DEBUG block is trying to check. Can you remove it while you're there?

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8391704 [details] [diff] [review]
Part c: Cleanup

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

::: js/xpconnect/src/XPCConvert.cpp
@@ +341,5 @@
>  
>              return XPCVariant::VariantDataToJS(variant,
>                                                 pErr, d);
>          }
> +

Ah ok, you removed this here.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/592e6de7211b
https://hg.mozilla.org/integration/mozilla-inbound/rev/91885eced736
https://hg.mozilla.org/integration/mozilla-inbound/rev/14ab9b8d15be

Comment 8

[Ed Morley [:emorley]]

Push(es) backed out for something therein causing failures of form:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37568923&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7db109e3a02b

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a84cc0abfc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3453b35da7f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e163d84250f0

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a84cc0abfc5
https://hg.mozilla.org/mozilla-central/rev/3453b35da7f5
https://hg.mozilla.org/mozilla-central/rev/e163d84250f0