315563 - Convert various embedding to use the frozen string API

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect, P2)

Description

[Benjamin Smedberg]

In preparation for stopping exporting nonfrozen functions from libxul, I'm
going through the tree and fixing the various code that currently use the
internal linkage to use frozen linkage. This bug is about parts of embedding/ that are not internal to libxul.

Comment 1

[Benjamin Smedberg]

Attachment: ActiveX control/plugin to use frozen string API

Comment 2

[Darin Fisher]

Comment on attachment 202256 [details] [diff] [review]
ActiveX control/plugin to use frozen string API

>Index: embedding/browser/activex/src/control/MozillaBrowser.cpp

>                 ios->GetProtocolHandler(kDesignModeScheme, getter_AddRefs(ph));
>                 if (ph &&
>                     NS_SUCCEEDED(ph->GetScheme(phScheme)) &&
>-                    phScheme.LowerCaseEqualsASCII(kDesignModeScheme))
>+                    phScheme.Equals(NS_LITERAL_CSTRING(kDesignModeScheme)))

hrm... Equals is not a replacement for LowerCaseEqualsASCII.
more to the point, i don't think there's any reason to call
GetScheme here and compare it against kDesignModeScheme.
necko promises to return the protocol handler requested (
except in cases where a proxy may be used, but that isn't
an issue for data: URLs).


>     nsAutoString strURI(NS_LITERAL_STRING("view-source:"));
>+    strURI.Append(NS_ConvertUTF8toUTF16(aURI));

It would almost be better to convert aURI into strURI first
and then use .Insert to eliminate an intermediate buffer.

It'd also be nice if the frozen string API had an AppendUTF8toUTF16.
I suppose we could implement that by turning the nsStringEncoding
parameter of NS_CStringToUTF16 into a bit field parameter and support
some flags to force the result to be appended.


>Index: embedding/browser/activex/src/control/PropertyDlg.cpp

>-        LossyCopyUTF16toASCII(mType, contentType);
>+        CopyUTF16toASCII(mType, contentType);

eek.. why isn't the version of this in nsStringAPI.h prefixed
with "Lossy"?  ack... i didn't catch that in the other patch.
i think we should use the Lossy prefix.


>Index: embedding/browser/activex/src/plugin/PrefObserver.cpp

>+#include "nsEmbedString.h"

nsStringAPI.h instead?  try not to mention nsEmbedString.h if it
can be helped since it only exists for source compat (sort of).


>Index: embedding/browser/activex/src/plugin/XPConnect.cpp

>+            nsAutoString valueWide;
>+            NS_CStringToUTF16(value, NS_CSTRING_ENCODING_ASCII, valueWide);

avoid nsAutoString... that guy is deprecated in the frozen string api.
also, probably better to use CopyASCIItoUTF16.


>+            nsAutoString valueWide;
>+            NS_CStringToUTF16(value, NS_CSTRING_ENCODING_ASCII, valueWide);

ditto


r=darin with these nits picked

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

> necko promises to return the protocol handler requested

what about the default protocol handler?

Comment 4

[Darin Fisher]

(In reply to comment #3)
> > necko promises to return the protocol handler requested
> 
> what about the default protocol handler?

hrm, true.  perhaps this code exists because adam thought there might be cases where the activex control might be compiled without built-in support for the data: protocol?  i guess if that is a realistic configuration, then the check is needed.

Comment 5

[Benjamin Smedberg]

Fixed on trunk.