Cleanup

Cleanup

[Oleksandr, 2013-07-05 03:33:55]

I have removed the unused code and added TODO's wherever things still need to be
re-factored.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:523: if (!settings->GetStatusBarAsked())
This is the only case where we might have still needed the settings.ini. We need
some sort of flag to indicate that we have already asked the user if he wants to
enable the status bar. I suggest storing that flag in the registry instead.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.cpp
File src/plugin/PluginSettings.cpp (left):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.c...
src/plugin/PluginSettings.cpp:823: {
Instead of storing current tab count in settings_tab.ini I suggest storing it in
AdblockPlusEngine.

[Wladimir Palant, 2013-07-05 09:26:45]

Good first step. I've indicated some things that can be removed as well but
there are more of course.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h
File src/plugin/Config.h (right):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode10
src/plugin/Config.h:10: #endif
We shouldn't do that in the config, it's already defined correctly in
adblockplus.gyp.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode15
src/plugin/Config.h:15: #define FILTERS_HOST
"easylist-downloads.adblockplus.org"
These two macros are unused and can be removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode34
src/plugin/Config.h:34: #define USERS_SCRIPT_UPGRADE        L"/user_upgrade.php"
All these macros need to be removed along with the code that's using them.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode46
src/plugin/Config.h:46: #define USERS_HOST L"127.0.0.1"
These two macros should be removed along with their use in
CPluginClientBase::IsValidDomain().

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode72
src/plugin/Config.h:72: #endif
This macro is unused and can be removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode76
src/plugin/Config.h:76: #define TIMER_THREAD_SLEEP_MAIN_LOOP 60000
This macro is unused and can be removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode213
src/plugin/Config.h:213: #define DOMAIN_HISTORY_MAX_COUNT 5
This macro is only used to set unused macros WM_WHITELIST_DOMAIN_MAX,
WM_WHITELIST_DOMAIN_SUPPORT, WM_WHITELIST_DOMAIN_SUPPORT_MAX,
WM_WHITELIST_DOMAIN_ERROR, WM_WHITELIST_DOMAIN_ERROR_MAX in PluginClass.h, it
can be removed along with these macros.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode216
src/plugin/Config.h:216: #define REGISTRATION_MAX_ATTEMPTS 5
This macro is unused and can be removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode232
src/plugin/Config.h:232: #endif
These two macros are unused and can be removed as well.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode239
src/plugin/Config.h:239: #define AUTOMATIC_SHUTDOWN
This macro is only being used in the unused CPluginClass::LaunchUpdater method -
it can be removed along with that method.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode244
src/plugin/Config.h:244: #define BHO_NAME _T("Adblock Plus BHO/1.0")
This macro is only used in PluginHttpRequest that's being removed now - can be
removed as well.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode249
src/plugin/Config.h:249: #define UNINSTALL_INI_FILE "AdBlockPlus.ini"
This macro is also unused and can be removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode254
src/plugin/Config.h:254: #define TEMP_FILE_PREFIX "ab_"
This macro is only used in PluginConfig.cpp that's being removed now - can be
removed as well.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode258
src/plugin/Config.h:258: #define DEFAULT_LANGUAGE "en-US"
These two macros are also unused and can be removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode262
src/plugin/Config.h:262: #define CONFIG_INI_FILE "config.ini"
This macro is only used in PluginConfig.cpp that is being removed - can be
removed as well.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/Config.h#newcode268
src/plugin/Config.h:268: #define INSTALL_EXE_FILE "Setup.exe"
These two macros are unused and can be removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:523: if (!settings->GetStatusBarAsked())
I suggest storing it in prefs.json instead - via the engine. Then we have all
settings in one place.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginClassThrea...
File src/plugin/PluginClassThread.cpp (right):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginClassThrea...
src/plugin/PluginClassThread.cpp:131: settings->RefreshFilterlist();
We shouldn't do that here - the engine will refresh the filters whenever
necessary. From what I can tell, RefreshFilterlist() method can be removed
altogether.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.cpp
File src/plugin/PluginSettings.cpp (right):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.c...
src/plugin/PluginSettings.cpp:453: //TOTO: Get value from the registry;
TOTO => TODO and please fix indentation

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.c...
src/plugin/PluginSettings.cpp:457: void CPluginSettings::SetStatusBarAsked(bool
asked)
The parameter can probably be removed here - we would never call it with false
as parameter.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h
File src/plugin/PluginSettings.h (right):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:61: bool m_isFirstRunUpdate;
I think that these two properties and the corresponding methods can be removed
as well.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:70: bool m_isDirty;
You can remove that property, all code using it has been removed.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:72: CString m_settingsVersion;
That property is unused.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:76: static CComAutoCriticalSection
s_criticalSectionDomainHistory;
This property is unused.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:89: static bool s_isLightOnly;
This property seems unused.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:96: static CString GetTempFile(const CString&
prefix, const CString& extension=L"");
These two methods seem unused.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:98: CString GetPluginId();
This method isn't even implemented.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:102: bool IsPluginSelftestEnabled();
This method isn't even implemented.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginSettings.h...
src/plugin/PluginSettings.h:105: bool SetFilterRefreshDate(CString filterlist,
time_t refreshtime);
These two methods aren't even implemented.

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginUserSettin...
File src/plugin/PluginUserSettings.cpp (right):

http://codereview.adblockplus.org/11013110/diff/1/src/plugin/PluginUserSettin...
src/plugin/PluginUserSettings.cpp:103: return
CString(dictionary->Lookup("settings", std::string(CW2A(key))).c_str());
CString => CStringW here to match return type?

[Wladimir Palant, 2013-07-11 12:53:10]

My comment in PluginUserSettings.cpp from the previous patchset hasn't been
addressed.

I guess that the statusbarasked pref currently doesn't persist. That's because
that preference isn't in the pref defaults in libadblockplus/lib/prefs.js. Not
persisting values without default is intentional, it prevents us from
accumulating trash in prefs.json (prefs that used to exist but no longer do).
But I guess we need some API to allow adding defaults from outside
libadblockplus.

http://codereview.adblockplus.org/11013110/diff/9001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11013110/diff/9001/src/engine/main.cpp#newc...
src/engine/main.cpp:149: std::string prefValue = "";
Please don't initialize these variables, their initial value is ignored and
overwritten below.

http://codereview.adblockplus.org/11013110/diff/9001/src/engine/main.cpp#newc...
src/engine/main.cpp:151: request >> prefValue;
These lines can be merged:

request >> prefName >> prefValue;

http://codereview.adblockplus.org/11013110/diff/9001/src/engine/main.cpp#newc...
src/engine/main.cpp:152: filterEngine->SetPref(prefName,
filterEngine->GetJsEngine()->NewValue(prefValue));
What about preferences with non-string values? This needs to support strings,
numbers (int64) and booleans to ensure type safety. The preference you are
actually setting is a boolean.

How about sending Communication::ValueType::TYPE_STRING,
Communication::ValueType::TYPE_INT64 or Communication::ValueType::TYPE_BOOL to
indicate the value type? Same in PROC_GET_PREF of course.

Unfortunately, CAdblockPlusClient will need multiple versions of GetPref/SetPref
functions then.

http://codereview.adblockplus.org/11013110/diff/9001/src/engine/main.cpp#newc...
src/engine/main.cpp:161: if ((valuePtr->IsNull()) || (!valuePtr->IsString()))
I think checking valuePtr->IsString() is sufficient - if it is a string then it
definitely isn't null. See also UndefinedValue and NullValue tests for the
JsValue class.

http://codereview.adblockplus.org/11013110/diff/9001/src/engine/main.cpp#newc...
src/engine/main.cpp:162: response << 0;
Please don't use an integer as a boolean, this should be << false (and << true
below of course).

http://codereview.adblockplus.org/11013110/diff/9001/src/engine/main.cpp#newc...
src/engine/main.cpp:166: response << valuePtr->AsString();
These two lines can be merged:

  response << true << valuePtr->AsString();

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/AdblockPlusCl...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/AdblockPlusCl...
src/plugin/AdblockPlusClient.cpp:412: return retValue.size() > 0 ?
retValue.at(0) : L"";
This really shouldn't try to read a list of strings if we are only talking about
a single string at most. Also, you shouldn't use ReadStrings() on something that
wasn't written via WriteStrings() - if somebody decides to change WriteStrings()
later he will certainly update ReadStrings() to match but he won't look at the
piece of code here. Better:

  bool success;
  response >> success;
  if (success)
  {
    std::string value;
    response >> value;
    return FromUtf8String(value);
  }
  else
    return L"";

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:433: MessageBox(NULL, L"", L"", MB_OK);
Please address that TODO comment ;)

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginDebug.cpp
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginDebug.c...
src/plugin/PluginDebug.cpp:46: CString processor;
Aren't you leaving that variable uninitialized? How about:

if (isWorkingThread)
  processor = L"tab" + ::GetCurrentProcessId() + L"_thread";
else
  processor = L"tab" + ::GetCurrentProcessId() + L"_ui";

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginSetting...
File src/plugin/PluginSettings.cpp (right):

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:25: return std::wstring(L"@@||" + domain +
L"^$document");
What's wrong with the implicit conversion? Instead I would change the type of
the domain parameter into |const std::wstring&| here to make it cleaner
(shouldn't require changing any callers I think).

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:70: m_settingsVersion("1"), m_isFirstRun(false),
m_dwWorkingThreadId(0), m_isPluginEnabledTab(true)
m_settingsVersion property can be removed as well.

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:250: return res == L"true";
As mentioned before, the lack of type safety here is saddening - particularly
considering that both the prefs API and the engine communication protocol are
type-safe.

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginSettings.h
File src/plugin/PluginSettings.h (right):

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginSetting...
src/plugin/PluginSettings.h:64: CString m_settingsVersion;
Still unused.

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginSetting...
src/plugin/PluginSettings.h:78: static bool s_isLightOnly;
Still unused.

[Oleksandr, 2013-07-22 00:48:16]

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginDebug.cpp
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/11013110/diff/9001/src/plugin/PluginDebug.c...
src/plugin/PluginDebug.cpp:46: CString processor;
Process ID isn't really a reliable identifier here. One process can host 5 tabs
as well as 1 tab.

On 2013/07/11 12:53:10, Wladimir Palant wrote:
> Aren't you leaving that variable uninitialized? How about:
> 
> if (isWorkingThread)
>   processor = L"tab" + ::GetCurrentProcessId() + L"_thread";
> else
>   processor = L"tab" + ::GetCurrentProcessId() + L"_ui";

[Wladimir Palant, 2013-07-22 06:43:11]

I assume that my comment in PluginUserSettings.cpp has been addressed.

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:358: void
CAdblockPlusClient::PostRequest(Communication::OutputBuffer request)
I can't say I like the PostRequest() and FetchResponse() helpers:

1) Its purpose is non-obvious from the name. It is really
CallAdblockPlusEngineProcedure() but merely with exceptions swallowed.

2) Even if the purpose were clear, IMHO this makes the code harder to follow
because the exception handling is hidden. Instead of catching exceptions the
calling functions need to check whether they got a valid result.

So I would pretty much suggest removing these helpers. Feel free to ask Felix
for a third opinion if you disagree.

Note that you can reduce code size regardless if you use templates for the
GetPref/SetPref methods.

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:391: void CAdblockPlusClient::SetPref(const
std::string& name, const std::string& value)
I don't think we need that function - the plugin always works with UTF-16, never
with UTF-8.

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:429: std::wstring
CAdblockPlusClient::GetPref(const std::wstring& name, LPCWSTR defaultValue)
I guess you have that here because const char* is implicitly converted to bool
instead of const std::wstring&? Then please actually use const char* as type
instead of LPCWSTR here, I don't see why we should use the Windows-specific
type.

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/PluginDebug.cpp
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/PluginDebug....
src/plugin/PluginDebug.cpp:47: WCHAR tmp[10];
I would rather use wchar_t rather than WCHAR - that's the type that _itow_s
takes according to documentation.

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/PluginDebug....
src/plugin/PluginDebug.cpp:48: _itow_s<10>(::GetCurrentProcessId(), tmp, 10);
I think you can leave out the template parameter here, that's the whole point of
the templated variant:

  _itow_s(::GetCurrentProcessId(), tmp, 10);

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/PluginDebug....
src/plugin/PluginDebug.cpp:52: processor = L"tab" + ::CString(tmp) + L"_ui";
CString() or ::CString()? Please choose one ;)

http://codereview.adblockplus.org/11013110/diff/23010/src/shared/Communication.h
File src/shared/Communication.h (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/shared/Communicatio...
src/shared/Communication.h:47: bool IsEmpty() { return buffer.eof(); }
This is just wrong :-(
eof() doesn't mean that the buffer is empty - it can easily mean that the entire
buffer has been read. And, as I mentioned before, you really shouldn't need
that.

[Oleksandr, 2013-07-22 09:53:31]

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:358: void
CAdblockPlusClient::PostRequest(Communication::OutputBuffer request)
I do disagree, at least partially. The code looks cleaner and more readable this
way IMHO. At least the PostRequest stuff seems to be worth it. Removed the
FetchResponse. Felix?
On 2013/07/22 06:43:11, Wladimir Palant wrote:
> I can't say I like the PostRequest() and FetchResponse() helpers:
> 
> 1) Its purpose is non-obvious from the name. It is really
> CallAdblockPlusEngineProcedure() but merely with exceptions swallowed.
> 
> 2) Even if the purpose were clear, IMHO this makes the code harder to follow
> because the exception handling is hidden. Instead of catching exceptions the
> calling functions need to check whether they got a valid result.
> 
> So I would pretty much suggest removing these helpers. Feel free to ask Felix
> for a third opinion if you disagree.
> 
> Note that you can reduce code size regardless if you use templates for the
> GetPref/SetPref methods.

http://codereview.adblockplus.org/11013110/diff/23010/src/shared/Communication.h
File src/shared/Communication.h (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/shared/Communicatio...
src/shared/Communication.h:47: bool IsEmpty() { return buffer.eof(); }
I guess the name was just wrong. Should've been IsEof(). That's what we were
really interested in.

On 2013/07/22 06:43:11, Wladimir Palant wrote:
> This is just wrong :-(
> eof() doesn't mean that the buffer is empty - it can easily mean that the
entire
> buffer has been read. And, as I mentioned before, you really shouldn't need
> that.

[Felix Dahlke, 2013-07-23 10:26:12]

Just commenting on one thing here. Patch set 4 doesn't seem to include the
previous ones (at least it seems like it), I'll review the remaining stuff once
that is fixed.

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:358: void
CAdblockPlusClient::PostRequest(Communication::OutputBuffer request)
On 2013/07/22 09:53:31, Oleksandr wrote:
> I do disagree, at least partially. The code looks cleaner and more readable
this
> way IMHO. At least the PostRequest stuff seems to be worth it. Removed the
> FetchResponse. Felix?
> On 2013/07/22 06:43:11, Wladimir Palant wrote:
> > I can't say I like the PostRequest() and FetchResponse() helpers:
> > 
> > 1) Its purpose is non-obvious from the name. It is really
> > CallAdblockPlusEngineProcedure() but merely with exceptions swallowed.
> > 
> > 2) Even if the purpose were clear, IMHO this makes the code harder to follow
> > because the exception handling is hidden. Instead of catching exceptions the
> > calling functions need to check whether they got a valid result.
> > 
> > So I would pretty much suggest removing these helpers. Feel free to ask
Felix
> > for a third opinion if you disagree.
> > 
> > Note that you can reduce code size regardless if you use templates for the
> > GetPref/SetPref methods.
> 

I can see how you'd like to avoid the duplication Oleksandr, but I'm not a fan
of this either.

It looks like we're just swallowing all the exceptions anyway, so what we could
do is change CallAdblockPlusEngine so that it works like FetchResponse does now:
Return some special value on error instead of throwing an exception. What do you
think, Wladimir?

[Wladimir Palant, 2013-07-23 12:18:27]

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:358: void
CAdblockPlusClient::PostRequest(Communication::OutputBuffer request)
On 2013/07/23 10:26:12, Felix H. Dahlke wrote:
> It looks like we're just swallowing all the exceptions anyway, so what we
could
> do is change CallAdblockPlusEngine so that it works like FetchResponse does
now:
> Return some special value on error instead of throwing an exception. What do
you
> think, Wladimir?

If you look closely, we aren't really swallowing exceptions - depending on the
caller a different default behavior is in order. So FetchResponse() merely
replaces real exceptions with sort of exceptions based on return value: the
caller needs to check the return value and then go to the "default" execution
branch. There is a readability loss but IMHO no advantage whatsoever (worse: if
we forget to check the return value we might not even crash but produce wrong
results and cause issues that will be hard to investigate).

Arguably, PostRequest() has some value for the cases where we aren't interested
in the response. But then the function name needs to make the intention more
clear: CallEngineIgnoreResponse() if you like (and I would really shorten the
name of CallAdblockPlusEngineProcedure() into CallEngine()).

[Felix Dahlke, 2013-07-23 12:40:15]

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/23010/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:358: void
CAdblockPlusClient::PostRequest(Communication::OutputBuffer request)
On 2013/07/23 12:18:27, Wladimir Palant wrote:
> On 2013/07/23 10:26:12, Felix H. Dahlke wrote:
> > It looks like we're just swallowing all the exceptions anyway, so what we
> could
> > do is change CallAdblockPlusEngine so that it works like FetchResponse does
> now:
> > Return some special value on error instead of throwing an exception. What do
> you
> > think, Wladimir?
> 
> If you look closely, we aren't really swallowing exceptions - depending on the
> caller a different default behavior is in order. So FetchResponse() merely
> replaces real exceptions with sort of exceptions based on return value: the
> caller needs to check the return value and then go to the "default" execution
> branch. There is a readability loss but IMHO no advantage whatsoever (worse:
if
> we forget to check the return value we might not even crash but produce wrong
> results and cause issues that will be hard to investigate).
> 
> Arguably, PostRequest() has some value for the cases where we aren't
interested
> in the response. But then the function name needs to make the intention more
> clear: CallEngineIgnoreResponse() if you like (and I would really shorten the
> name of CallAdblockPlusEngineProcedure() into CallEngine()).

What I meant was that we ignore the actual exception, we just log it in any
case. So I think we should log it in CallEngine(), not in the caller.

Then we have two options for handling errors: Use exceptions or return codes.
With exceptions it would look like this:

When we care:

try
{
  response = CallEngine(request);
}
catch (const std::exception&)
{
  return false;
}

When we don't care:

try
{
  CallEngine(request);
}
catch (const std::exception&) {}

With return codes, it could look like this:

When we care:

if (!CallEngine(request, response))
  return false;

When we don't care:

CallEngine(request);

I generally like exceptions, obviously, but I think in this case it's better to
go with the return code. We don't care what the exception is, and if my
suggestion is implemented, it's already being handled in CallEngine.

As you can see, I'm all for renaming the method to CallEngine() :)

[Oleksandr, 2013-07-24 09:15:06]

Refactored the CallAdblockPlusEngine

[Felix Dahlke, 2013-07-25 13:52:33]

http://codereview.adblockplus.org/11013110/diff/37001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11013110/diff/37001/src/engine/main.cpp#new...
src/engine/main.cpp:201: response << true;
So anything that is not bool, int or string will report success but not add any
value?

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:357: // A helper method to reuse the code
Ignoring this, since I commented on it in patch set 5. Would have been nice to
have all the changes in a single patch set though :)

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/AdblockPlusT...
File src/plugin/AdblockPlusTab.cpp (right):

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/AdblockPlusT...
src/plugin/AdblockPlusTab.cpp:20: void CPluginTab::OnNavigate(const CString&
url)
I don't think there's any point in overwriting this function anymore.

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/PluginDebug.cpp
File src/plugin/PluginDebug.cpp (right):

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/PluginDebug....
src/plugin/PluginDebug.cpp:48: _itow_s(::GetCurrentProcessId(), tmp, 10);
It'd be easier/safer to use std::wstringstream here.

std::wstringstream stream;
stream << GetCurrentProcessId();
std::wstring processId = stream.str();

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/PluginUserSe...
File src/plugin/PluginUserSettings.cpp (right):

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/PluginUserSe...
src/plugin/PluginUserSettings.cpp:103: return
CString(dictionary->Lookup("settings", std::string(CW2A(key))).c_str());
Better use ToUtf8String() instead of std::string(CW2A()). But I do wonder why
Lookup uses std::string in the signature to begin with, I think we generally
wanted to use std::wstring in the plugin.

Also, this can be simplified by not duplicating logic from the other sGetMessage
function:

return CString(sGetMessage("settings", ToUtf8String(key)).c_str());

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:106: bool
CallEngine(Communication::OutputBuffer& message, Communication::InputBuffer*
inputBuffer = NULL)
Since we always read the data anyway, I think we should use a reference instead
of a pointer and use an input buffer that is going to be ignored as the default
value.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:112: if (inputBuffer != NULL)
if (inputBuffer) does the same.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:232: if (!CallEngine(request, &response))
return false;;
One semicolon will do :)

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:266: if (!CallEngine(request, &response))
return false;;
As above, one semicolon will do.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/PluginSettin...
File src/plugin/PluginSettings.cpp (right):

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/PluginSettin...
src/plugin/PluginSettings.cpp:25: return std::wstring(L"@@||") +
domain.GetString() + std::wstring(L"^$document");
What is this change for? It worked before, didn't it?

[Oleksandr, 2013-07-26 14:20:04]

Uploaded the whole change set, as Felix suggested. Unfortunately patch set #5
was only partial, so if you'd like to see what exactly has been changed, I'd
recommend comparing #4 with #6. Alternatively I can remove the #5.

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/AdblockPlusT...
File src/plugin/AdblockPlusTab.cpp (right):

http://codereview.adblockplus.org/11013110/diff/37001/src/plugin/AdblockPlusT...
src/plugin/AdblockPlusTab.cpp:20: void CPluginTab::OnNavigate(const CString&
url)
You're right. I'd still leave the CPuginTab class as an abstraction for future.
On 2013/07/25 13:52:33, Felix H. Dahlke wrote:
> I don't think there's any point in overwriting this function anymore.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/PluginSettin...
File src/plugin/PluginSettings.cpp (right):

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/PluginSettin...
src/plugin/PluginSettings.cpp:25: return std::wstring(L"@@||") +
domain.GetString() + std::wstring(L"^$document");
This is just to remove the warning C4927 here.
http://msdn.microsoft.com/en-us/library/727wstex(v=vs.80).aspx

On 2013/07/25 13:52:33, Felix H. Dahlke wrote:
> What is this change for? It worked before, didn't it?

[Wladimir Palant, 2013-07-26 16:45:37]

This review is quite a mess - I think that I commented on the same issues
multiple times but I cannot figure out what is already reviewed and what isn't.
Maybe try splitting large changes into smaller isolated parts next time, with a
separate review for each?

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:232: if (!CallEngine(request, &response))
return false;;
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:266: if (!CallEngine(request, &response))
return false;;
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:279: if (!CallEngine(request, &response))
return std::vector<std::wstring>();
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:286: if
(!CallEngine(Communication::PROC_AVAILABLE_SUBSCRIPTIONS, &response)) return
std::vector<SubscriptionDescription>();
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:293: if
(!CallEngine(Communication::PROC_LISTED_SUBSCRIPTIONS, &response)) return
std::vector<SubscriptionDescription>();
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:312: if
(!CallEngine(Communication::PROC_GET_EXCEPTION_DOMAINS)) return
std::vector<std::wstring>();
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:361: if (!CallEngine(request, &response))
return defaultValue;
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:380: if (!CallEngine(request, &response))
return defaultValue;
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:398: if (!CallEngine(request, &response))
return defaultValue;
Style nit: please put the return statement on the next line.

http://codereview.adblockplus.org/11013110/diff/41001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:399: bool success;
Wrong indentation?

http://codereview.adblockplus.org/11013110/diff/41001/src/shared/Communication.h
File src/shared/Communication.h (right):

http://codereview.adblockplus.org/11013110/diff/41001/src/shared/Communicatio...
src/shared/Communication.h:48: currentType = copy.currentType; return *this; }
Weird indentation here - how about splitting this into multiple lines properly?
Same applies to the copy constructor above of course.

http://codereview.adblockplus.org/11013110/diff/52001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11013110/diff/52001/src/engine/main.cpp#new...
src/engine/main.cpp:193: if (valuePtr->IsNull() || valuePtr->IsUndefined())
valuePtr->IsNull() will be true for undefined as well, so checking IsUndefined()
as well is unnecessary. Actually, the entire block is unnecessary - null and
undefined will result in false for IsBool(), IsNumber() and IsString() (see
JsValue unit test), we can just treat them like any unknown type.

http://codereview.adblockplus.org/11013110/diff/52001/src/engine/main.cpp#new...
src/engine/main.cpp:204: response << true;
I don't get this, shouldn't you first write success and only then the value you
are sending? Should be:

  response << true << valuePtr->AsBool();

And similarly below.

http://codereview.adblockplus.org/11013110/diff/52001/src/plugin/AdblockPlus.rc
File src/plugin/AdblockPlus.rc (left):

http://codereview.adblockplus.org/11013110/diff/52001/src/plugin/AdblockPlus....
src/plugin/AdblockPlus.rc:74: MENUITEM "Update Adblock Plus",      
ID_PLUGIN_UPDATE
We should leave the option to trigger an update check manually.

http://codereview.adblockplus.org/11013110/diff/52001/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/11013110/diff/52001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.h:36: void PostRequest(Communication::OutputBuffer
request);
I don't think you are still implementing this method.

http://codereview.adblockplus.org/11013110/diff/52001/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11013110/diff/52001/src/plugin/PluginClass....
src/plugin/PluginClass.cpp:73: // Load settings (fetch the available
subscriptions, if still not fetched)
This comment is wrong, the plugin no longer deals with subscriptions directly.
Which leads to the question - is it necessary to explicitly instantiate settings
here?

[Felix Dahlke, 2013-08-05 13:08:58]

It looks like some of Wladimir's comments are still not addressed, please make
sure to address everything.

[Oleksandr, 2013-08-05 23:11:00]

http://codereview.adblockplus.org/11013110/diff/52001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11013110/diff/52001/src/engine/main.cpp#new...
src/engine/main.cpp:193: if (valuePtr->IsNull() || valuePtr->IsUndefined())
This was addressed in:
https://hg.adblockplus.org/adblockplusie/rev/8908c2f7d176

 On 2013/07/26 16:45:37, Wladimir Palant wrote:
> valuePtr->IsNull() will be true for undefined as well, so checking
IsUndefined()
> as well is unnecessary. Actually, the entire block is unnecessary - null and
> undefined will result in false for IsBool(), IsNumber() and IsString() (see
> JsValue unit test), we can just treat them like any unknown type.

[Felix Dahlke, 2013-08-06 10:27:36]

LGTM, assuming all of Wladimir's comments have been addressed.