1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp.

Review commit: https://reviewboard.mozilla.org/r/201036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201036/

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 8929812 [details]
Bug 1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201036/diff/1-2/

Comment 3

[Henri Sivonen (:hsivonen)]

Comment on attachment 8929812 [details]
Bug 1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp.

https://reviewboard.mozilla.org/r/201036/#review210750

::: dom/encoding/FallbackEncoding.cpp:35
(Diff revision 2)
> +                   const nsACString& aKey)
> +{
> +  const nsCString& flat = PromiseFlatCString(aKey);
> +  size_t index;
> +  if (!BinarySearchIf(aProperties, 0, ArrayLength(aProperties),
> +                      [flat](const EncodingProp& aProperty)

Does `[flat]` end up running a copy constructor on every comparison? Would `[&flat]` be better?

r+ with `[&flat]` if that works or with `[flat]` and an explanation why `[&flat]` isn't appropriate.

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 8929812 [details]
Bug 1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp.

https://reviewboard.mozilla.org/r/201036/#review210750

> Does `[flat]` end up running a copy constructor on every comparison? Would `[&flat]` be better?
> 
> r+ with `[&flat]` if that works or with `[flat]` and an explanation why `[&flat]` isn't appropriate.

This is roughly equivalent to:
```
struct Comparator {
  const nsCString flat;
  int operator ()(const EncodingProp& aProperty) const
  { return flat.Compare(aProperty.mKey); }
};
if (!BinarySearchIf(aProperties, 0, ArrayLength(aProperties),
    Comparator{flat}))
```
The copy constructor will be invoked only once when the closure is created, not everytime it is called.

But you are right about that capturing `flat` by reference is safe here and more efficient. I'll change this to [&flat].

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 8929812 [details]
Bug 1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201036/diff/2-3/

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> The copy constructor will be invoked only once when the closure is created,
> not everytime it is called.

I see.

> But you are right about that capturing `flat` by reference is safe here and
> more efficient. I'll change this to [&flat].

Thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59adfaf5e09f
Stop using Encoding::ForName in FallbackEncoding.cpp. r=hsivonen

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/59adfaf5e09f