Issue 1490 - Enable pre-configurable properties in Internet Explorer

Issue 1490 - Enable pre-configurable properties in Internet Explorer

[sergei, 2015-06-19 12:30:29]

LGTM, but the commit id should be valid commit id. I guess it makes sense to
update it after committing of
https://codereview.adblockplus.org/5653480979038208/

https://codereview.adblockplus.org/29317083/diff/29317084/.hgsubstate
File .hgsubstate (right):

https://codereview.adblockplus.org/29317083/diff/29317084/.hgsubstate#newcode2
.hgsubstate:2: cf69fe05ea8c9e9a84d3c8320f03a22f797ca1fe libadblockplus
It does not exist, at least yet, isn't it?

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp
File src/engine/Main.cpp (right):

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp...
src/engine/Main.cpp:441: catch (std::runtime_error&)
`const`

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp...
src/engine/Main.cpp:443: return L"";
I would put it after try-catch.

[sergei, 2015-06-19 12:31:47]

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp
File src/engine/Main.cpp (right):

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp...
src/engine/Main.cpp:441: catch (std::runtime_error&)
On 2015/06/19 12:30:28, sergei wrote:
> `const`

And `std::exception` would be better than `std::runtime_error`.

[Eric, 2015-06-19 14:51:41]

https://codereview1.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp
File src/engine/Main.cpp (right):

https://codereview1.adblockplus.org/29317083/diff/29317084/src/engine/Main.cp...
src/engine/Main.cpp:434: std::wstring LoadPreconfigurationValue(const
std::wstring& preconfigName)
This name would be better as "RegistryPreconfigurationValue" or
"PreconfigurationValueFromRegistry".

The name present now is an imperative verb (what action to do). This name would
work better as a declarative noun (what result to return). As a rule, when you
care about the return value, declaratives (nouns) work better.

https://codereview1.adblockplus.org/29317083/diff/29317084/src/engine/Main.cp...
src/engine/Main.cpp:434: std::wstring LoadPreconfigurationValue(const
std::wstring& preconfigName)
Also. This function can go in the anonymous namespace immediately above.

https://codereview1.adblockplus.org/29317083/diff/29317084/src/engine/Main.cp...
src/engine/Main.cpp:441: catch (std::runtime_error&)
On 2015/06/19 12:31:47, sergei wrote:
> And `std::exception` would be better than `std::runtime_error`.

"runtime_error" is correct. If the constructor were to throw logic_error, we
would not want to mask it here; it would be better to let that exception
propagate upward and handle it more generically.

https://codereview1.adblockplus.org/29317083/diff/29317084/src/engine/Main.cp...
src/engine/Main.cpp:443: return L"";
On 2015/06/19 12:30:28, sergei wrote:
> I would put it after try-catch.

The code reads better to have the return statement here. It's much clearer that
the empty string is a substitute value for a registry key that's absent.

Compiler path analysis will catch a missing return statement just fine. It's an
artifact of old, less competent compilers that the practice arose of putting a
return statement at the end of a function block.

https://codereview1.adblockplus.org/29317083/diff/29317084/src/plugin/PluginC...
File src/plugin/PluginClass.cpp (right):

https://codereview1.adblockplus.org/29317083/diff/29317084/src/plugin/PluginC...
src/plugin/PluginClass.cpp:919: if
(CPluginClient::GetInstance()->GetPref(L"suppress_first_run_page", false))
I dislike the way that default values for preferences are spread throughout the
code rather than localized in 'libadblockplus'. It's really a design defect,
because it means that each client of 'libadblockplus' has to get the defaults
consistent with each other.

If that design defect were fixed, there would be no need for the second argument
here.

I understand that this might be considered out of scope. But since we're working
on preferences generally, now would be a good time to correct the flaw.

[mathias, 2015-06-22 07:31:19]

https://issues.adblockplus.org/ticket/2703
Off-topic: Does the mail notification work again?

[Oleksandr, 2015-06-22 08:01:33]

On 2015/06/22 07:31:19, mathias wrote:
> https://issues.adblockplus.org/ticket/2703
> Off-topic: Does the mail notification work again?

Off-topic: No

[Oleksandr, 2015-06-22 08:27:53]

https://codereview.adblockplus.org/29317083/diff/29317084/.hgsubstate
File .hgsubstate (right):

https://codereview.adblockplus.org/29317083/diff/29317084/.hgsubstate#newcode2
.hgsubstate:2: cf69fe05ea8c9e9a84d3c8320f03a22f797ca1fe libadblockplus
On 2015/06/19 12:30:28, sergei wrote:
> It does not exist, at least yet, isn't it?

Right, it would be these changes added, essentially:
https://codereview.adblockplus.org/5653480979038208/

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp
File src/engine/Main.cpp (right):

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp...
src/engine/Main.cpp:434: std::wstring LoadPreconfigurationValue(const
std::wstring& preconfigName)
On 2015/06/19 14:51:40, Eric wrote:
> Also. This function can go in the anonymous namespace immediately above.

Done.

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp...
src/engine/Main.cpp:441: catch (std::runtime_error&)
On 2015/06/19 14:51:41, Eric wrote:
> On 2015/06/19 12:31:47, sergei wrote:
> > And `std::exception` would be better than `std::runtime_error`.
> 
> "runtime_error" is correct. If the constructor were to throw logic_error, we
> would not want to mask it here; it would be better to let that exception
> propagate upward and handle it more generically.
> 

Acknowledged.

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp...
src/engine/Main.cpp:443: return L"";
On 2015/06/19 12:30:28, sergei wrote:
> I would put it after try-catch.

Acknowledged.

https://codereview.adblockplus.org/29317083/diff/29317084/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/29317083/diff/29317084/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:919: if
(CPluginClient::GetInstance()->GetPref(L"suppress_first_run_page", false))
On 2015/06/19 14:51:41, Eric wrote:
> I dislike the way that default values for preferences are spread throughout
the
> code rather than localized in 'libadblockplus'. It's really a design defect,
> because it means that each client of 'libadblockplus' has to get the defaults
> consistent with each other.
> 
> If that design defect were fixed, there would be no need for the second
argument
> here.
> 
> I understand that this might be considered out of scope. But since we're
working
> on preferences generally, now would be a good time to correct the flaw.

I agree, but yes, I think it's out of scope here.

[sergei, 2015-06-22 09:17:06]

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp
File src/engine/Main.cpp (right):

https://codereview.adblockplus.org/29317083/diff/29317084/src/engine/Main.cpp...
src/engine/Main.cpp:434: std::wstring LoadPreconfigurationValue(const
std::wstring& preconfigName)
However our functions usually contain the verb in the name, so should it be like
"GetPreconfigurationValueFromRegistry"?

[Eric, 2015-06-25 18:06:55]

LGTM