Status bar menu added

Status bar menu added

[Wladimir Palant, 2013-08-01 10:48:19]

http://codereview.adblockplus.org/11292028/diff/1/locales/en.ini
File locales/en.ini (right):

http://codereview.adblockplus.org/11292028/diff/1/locales/en.ini#newcode49
locales/en.ini:49: enable-on-site=Enable on site
These strings should say which site is meant. Also, "Enable on site" implies (to
somebody who forgot about disabling ABP on that site) that you have to enable
ABP manually on each website. The string we use in Firefox is "Disable on ?1?"
where ?1? is replaced by the host name. There is a checkmark displayed next to
it if the filter is active.

http://codereview.adblockplus.org/11292028/diff/1/locales/en.ini#newcode51
locales/en.ini:51: enable=Enable
The string used in Firefox is "Disable everywhere", again with a checkmark. This
is consistent, also IMHO it does a better job explaining what this entry does.

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

http://codereview.adblockplus.org/11292028/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1236: int navigationErrorId = 0;
These two variables are unused now, please remove.

Nice case for declaring variables close to their use :)

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

http://codereview.adblockplus.org/11292028/diff/1/src/plugin/PluginTabBase.h#...
src/plugin/PluginTabBase.h:25: }
I don't see the point of moving this function. Functions are declared in an
anonymous namespace to ensure that they are only used internally and cannot be
accidentally referenced from other files. Declaring the function in a header
file defeats the whole purpose of this approach. If you want to use that
function in multiple places then you should declare it properly in a header and
define in the corresponding C++ file. I think that PluginUtil is the place to
such utility functions.

[Wladimir Palant, 2013-08-02 11:06:10]

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/AdblockPlus.rc
File src/plugin/AdblockPlus.rc (right):

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/AdblockPlus.r...
src/plugin/AdblockPlus.rc:74: MENUITEM "Settings",                   
ID_SETTINGS
What about this menu item, is it localized?

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/AdblockPlus.r...
src/plugin/AdblockPlus.rc:77: MENUITEM "Disable plugin",             
ID_WHITELISTDOMAIN
Maybe change these labels to better reflect what we have in the dictionary?

[Felix Dahlke, 2013-08-02 13:52:58]

Might have commented on unrelated changes, but while we're at it...

(I know you've got a lot in your queue right now so I won't nag too much, but I
do prefer reviews without unrelated changes :P)

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

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:1292: HRESULT hr =
browser->Navigate(BString(UserSettingsFileUrl()), &vFlags, NULL, NULL, NULL);
While we're at it, I think we should store BString(UserSettingsFileUrl()) in a
temp since it's repeated below.

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:1313: 
Superfluous whitespace.

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:1366: ctext = dictionary->Lookup("menu",
"disable-on-site");
Looks like the indentation is off here.

[Felix Dahlke, 2013-08-05 13:42:23]

LGTM

[Oleksandr, 2013-08-05 13:59:22]

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/AdblockPlus.rc
File src/plugin/AdblockPlus.rc (right):

http://codereview.adblockplus.org/11292028/diff/7001/src/plugin/AdblockPlus.r...
src/plugin/AdblockPlus.rc:74: MENUITEM "Settings",                   
ID_SETTINGS
It is.
"menu-settings"

On 2013/08/02 11:06:10, Wladimir Palant wrote:
> What about this menu item, is it localized?

[Wladimir Palant, 2013-08-14 06:40:02]

LGTM