Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5979857238360064: Issues #1163, #1173 - refactor CPluginUserSettings (Closed)

Created:
Aug. 6, 2014, 4:44 p.m. by Eric
Modified:
Feb. 23, 2015, 2:49 p.m.
Reviewers:
sergei, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

Issues #1163, #1173 - refactor CPluginUserSettings Added exception handlers around the bodies of entry points that need them. Document the absence of handlers where they're not needed. Refactored the control flows in the IDispatch::Invoke implementation of CPluginUserSettings. Eliminate linear searches of string comparisons in both GetIDsOfNames() and Invoke(). Replaced the linear search (a loop) in GetIDsOfNames() by an unordered_map lookup. Replaced the linear search (an if-else sequence) in Invoke() by a switch statement. The code within the switch statement differs mostly in indentation from the previous version. Exceptions: * Fixed defect within case dispatchID_GetMessage. The type check for the VARIANT argument was missing. * Fixed a defect in case dispatchID_IsUpdate. The VARIANT_BOOL return value had been returning a C++ bool. * Swapped terms in yoda expressions. * Added braces to conditions.

Patch Set 1 : #

Total comments: 11

Patch Set 2 : #

Total comments: 10

Patch Set 3 : #

Total comments: 1

Patch Set 4 : rebased to current tip #

Patch Set 5 : Fixed defects involving pVarResult #

Total comments: 14

Patch Set 6 : nit fixes #

Total comments: 1

Patch Set 7 : rebase + add one final space character #

Unified diffs Side-by-side diffs Delta from patch set Stats (+377 lines, -320 lines) Patch
M src/plugin/PluginUserSettings.h View 1 2 3 3 chunks +5 lines, -6 lines 0 comments Download
M src/plugin/PluginUserSettings.cpp View 1 2 3 4 5 6 1 chunk +371 lines, -303 lines 0 comments Download
M test/plugin/UserSettingsTest.cpp View 1 2 3 4 chunks +1 line, -11 lines 0 comments Download

Messages

Total messages: 19
Eric
Sorry, but the diff facility within this review tool does not show the changes only ...
Aug. 6, 2014, 5 p.m. (2014-08-06 17:00:04 UTC) #1
Oleksandr
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp#newcode34 src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); It's outside the scope of this review, ...
Aug. 17, 2014, 10:52 p.m. (2014-08-17 22:52:00 UTC) #2
Eric
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp#newcode34 src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); On 2014/08/17 22:52:01, Oleksandr wrote: > [...] ...
Sept. 29, 2014, 6:45 p.m. (2014-09-29 18:45:41 UTC) #3
Oleksandr
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp#newcode34 src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); Creating a low priority issue in our ...
Oct. 2, 2014, 8:36 p.m. (2014-10-02 20:36:53 UTC) #4
Eric
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/plugin/PluginUserSettings.cpp#newcode34 src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); On 2014/10/02 20:36:53, Oleksandr wrote: > Creating ...
Oct. 14, 2014, 10:23 p.m. (2014-10-14 22:23:49 UTC) #5
Eric
New patch set updates the code to account for changes since review was created.
Jan. 4, 2015, 9:09 p.m. (2015-01-04 21:09:59 UTC) #6
sergei
http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/plugin/PluginUserSettings.cpp#newcode148 src/plugin/PluginUserSettings.cpp:148: if (!pDispparams || !pExcepinfo) pExcepinfo can be nullptr, it's ...
Jan. 9, 2015, 3:42 p.m. (2015-01-09 15:42:54 UTC) #7
Eric
http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/plugin/PluginUserSettings.cpp#newcode148 src/plugin/PluginUserSettings.cpp:148: if (!pDispparams || !pExcepinfo) On 2015/01/09 15:42:54, sergei wrote: ...
Jan. 9, 2015, 4:25 p.m. (2015-01-09 16:25:24 UTC) #8
Eric
New patch set. Also added conformant license headers. See https://adblockplus.org/en/license-headers
Jan. 9, 2015, 4:38 p.m. (2015-01-09 16:38:15 UTC) #9
sergei
http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/plugin/PluginUserSettings.cpp#newcode169 src/plugin/PluginUserSettings.cpp:169: if (pVarResult) On 2015/01/09 16:25:24, Eric wrote: > On ...
Jan. 30, 2015, 4:01 p.m. (2015-01-30 16:01:40 UTC) #10
Eric
> http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/plugin/PluginUserSettings.cpp > File src/plugin/PluginUserSettings.cpp (right): > > http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/plugin/PluginUserSettings.cpp#newcode2 > src/plugin/PluginUserSettings.cpp:2: * This file is ...
Feb. 2, 2015, 1:51 p.m. (2015-02-02 13:51:45 UTC) #11
Felix Dahlke
On 2015/02/02 13:51:45, Eric wrote: > > > http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/plugin/PluginUserSettings.cpp > > File src/plugin/PluginUserSettings.cpp (right): > ...
Feb. 2, 2015, 2:15 p.m. (2015-02-02 14:15:48 UTC) #12
Eric
Two new patch sets. The first is a rebase to tip, to account for a ...
Feb. 2, 2015, 2:46 p.m. (2015-02-02 14:46:39 UTC) #13
Eric
BUMP. The latest patch has languished for a week now with no comment from anybody. ...
Feb. 11, 2015, 5:08 p.m. (2015-02-11 17:08:43 UTC) #14
Oleksandr
Looks great in all. Just some minor style nits from me. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): ...
Feb. 12, 2015, 4:46 a.m. (2015-02-12 04:46:22 UTC) #15
Eric
All nits fixed. One nexus of really awful code not fixed as out of scope ...
Feb. 13, 2015, 2:57 p.m. (2015-02-13 14:57:57 UTC) #16
Oleksandr
Just one small nit. LGTM otherwise. http://codereview.adblockplus.org/5979857238360064/diff/5743868070854656/src/plugin/PluginUserSettings.cpp File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5743868070854656/src/plugin/PluginUserSettings.cpp#newcode450 src/plugin/PluginUserSettings.cpp:450: catch(...) Nit: Space ...
Feb. 19, 2015, 9:01 a.m. (2015-02-19 09:01:05 UTC) #17
sergei
LGTM
Feb. 19, 2015, 10:43 a.m. (2015-02-19 10:43:02 UTC) #18
Eric
Feb. 23, 2015, 1:39 p.m. (2015-02-23 13:39:25 UTC) #19
Patch set 7 differs from patch set 6 only by a single whitespace character.
Rebase makes it look different.

Powered by Google App Engine
This is Rietveld