Issue 29330618: Issue #1234 - Eliminate CString from PluginSettings.* and PluginUserSettings.*

Issue 29330618: Issue #1234 - Eliminate CString from PluginSettings.* and PluginUserSettings.* (Closed)

Created:
Nov. 20, 2015, 6:37 p.m. by Eric

Modified:
Nov. 26, 2015, 12:50 p.m.

Reviewers:
sergei, Oleksandr

CC:
Felix Dahlke

Visibility:
Public.

Description

Issue #1234 - Eliminate CString from PluginSettings.* and PluginUserSettings.* Converted CString variables to std::wstring in PluginSettings.* and PluginUserSettings.*. Inlined a few functions that were only called once. Corrected incorrect returns of DISP_E_EXCEPTION wtih DISP_E_BADINDEX.

Patch Set 1 #

Total comments: 13

Patch Set 2 : address comments #

Created: Nov. 25, 2015, 5:15 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+372 lines, -390 lines)			Patch
M	src/plugin/AdblockPlusClient.h	View	1	3 chunks	+3 lines, -4 lines	0 comments	Download
M	src/plugin/PluginClass.cpp	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	src/plugin/PluginSettings.h	View	1	1 chunk	+93 lines, -96 lines	0 comments	Download
M	src/plugin/PluginSettings.cpp	View	1	1 chunk	+245 lines, -257 lines	0 comments	Download
M	src/plugin/PluginUserSettings.cpp	View	1	13 chunks	+30 lines, -31 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

Eric

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/AdblockPlusClient.h File src/plugin/AdblockPlusClient.h (left): https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/AdblockPlusClient.h#oldcode96 src/plugin/AdblockPlusClient.h:96: std::wstring GetAppLocale(); Dead code. https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginSettings.h File src/plugin/PluginSettings.h (left): https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginSettings.h#oldcode87 ...

Nov. 20, 2015, 6:46 p.m. (2015-11-20 18:46:04 UTC) #1

Oleksandr

Changing return value definitely looks like something for a different change set. Also, I personally ...

Nov. 25, 2015, 3:22 a.m. (2015-11-25 03:22:08 UTC) #2

sergei

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginSettings.cpp File src/plugin/PluginSettings.cpp (right): https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginSettings.cpp#newcode106 src/plugin/PluginSettings.cpp:106: filterList.insert(std::make_pair(it.url,it.title)); the space after comma is missed https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginUserSettings.cpp File ...

Nov. 25, 2015, 9:27 a.m. (2015-11-25 09:27:09 UTC) #3

Eric

> Also, I personally preferred the functions inlined here > not inlined. But I don't ...

Nov. 25, 2015, 5:31 p.m. (2015-11-25 17:31:02 UTC) #4

> Also, I personally preferred the functions inlined here 
> not inlined. But I don't feel too strong about that.

There's another reason to remove them from the settings class. In both cases,
not only were they only called once, but they are not actually settings, since
they aren't configurable in any way. 

On the other hand, the name PluginUserSettings is something of a misnomer, since
provide an interface not only to settings proper, but also to some application
information and to translation strings.

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginSe...
File src/plugin/PluginSettings.cpp (right):

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginSe...
src/plugin/PluginSettings.cpp:106:
filterList.insert(std::make_pair(it.url,it.title));
On 2015/11/25 09:27:08, sergei wrote:
> the space after comma is missed

Done.

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginUs...
File src/plugin/PluginUserSettings.cpp (right):

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginUs...
src/plugin/PluginUserSettings.cpp:186: std::wstring message =
dictionary->Lookup(ToUtf8String(std::wstring(section)),
ToUtf8String(std::wstring(key)));
On 2015/11/25 09:27:08, sergei wrote:
> Nit: strictly speaking we should use the length obtained via `SysStringLen`

Done.

There are a few other places where we need this conversion. We probably ought to
define a conversion constructor for it, analogous to the one we have for CString
conversion:

  std::wstring ToWstring( BSTR )

... but not in this change set.

> we don't need ATL::CComBSTR here and below.

I'm calling this one as an unrelated change that ought to have an issue
association. It's not just in this file; there are plenty other places where
have CComBSTR where we don't need. We don't have a ticket for cleaning up this
usage, and such changes should be linked to one.

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginUs...
src/plugin/PluginUserSettings.cpp:225: return DISP_E_BADINDEX;
On 2015/11/25 09:27:09, sergei wrote:
> I would not mind to commit it but it's likely not what we want. 

It fixes a defect, actually. Rather than bicker over it here, I've reverted it
and created a new ticket describing the defect.

https://issues.adblockplus.org/ticket/3350

> IDispatch's
> error codes are quite specific for each method and I'm pretty sure that any
> caller expects IDispatch::Invoke to return only error codes listed here
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms221479(v=vs.85).aspx.

Microsoft Excel returns DISP_E_BADINDEX, as a web search clearly shows. See, for
example,
http://stackoverflow.com/questions/16851642/c-sharp-excel-interop-exception-f...
where the .NET runtime correctly converts the numeric HRESULT to both its
symbolic origin and also a human language error message (albeit an unclear one).
The point here is the Microsoft is supporting this return value, even if they
haven't bothered to update their documentation.

> So, only DISP_E_EXCEPTION seems to be the correct return value for custom
> errors.

That same page you referenced says the following about DISP_E_EXCEPTION:

    The application needs to raise an exception. In this case, the structure
    passed in pexcepinfo should be filled in. 

The defect is that we're not filling in any exception structure.

sergei

Nov. 26, 2015, 11:06 a.m. (2015-11-26 11:06:18 UTC) #6

LGTM

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginUs...
File src/plugin/PluginUserSettings.cpp (right):

https://codereview.adblockplus.org/29330618/diff/29330619/src/plugin/PluginUs...
src/plugin/PluginUserSettings.cpp:186: std::wstring message =
dictionary->Lookup(ToUtf8String(std::wstring(section)),
ToUtf8String(std::wstring(key)));
On 2015/11/25 17:31:02, Eric wrote:
> On 2015/11/25 09:27:08, sergei wrote:
> > Nit: strictly speaking we should use the length obtained via `SysStringLen`
> 
> Done.
> 
> There are a few other places where we need this conversion. We probably ought
to
> define a conversion constructor for it, analogous to the one we have for
CString
> conversion:
> 
>   std::wstring ToWstring( BSTR )
> 
> ... but not in this change set.
Sure, feel free to create the issue for it.

Expand All Messages | Collapse All Messages