Show better instructions to IE9+ users

While I was at it, I improved the related texts a bit.

Changing files/dictionary_w.ini did not have any effect for me, I had to hard code it in PluginDictionary.cpp. But I added it to dictionary_w.ini anyway to avoid confusion.

[Felix Dahlke, 2013-06-07 06:28:24]

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

http://codereview.adblockplus.org/10800100/diff/3001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:578: int versionNumber = wcstol(browserVersion, 0,
10);
@Oleksandr:
Can I rely on this being an integer? It's "9" for me (although I have IE10), can
this be a non-integer (e.g. a full semantic version string) in other IE
versions?

Is there a better way to check if we're IE9 or newer?

[Oleksandr, 2013-06-07 07:16:46]

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

http://codereview.adblockplus.org/10800100/diff/3001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:578: int versionNumber = wcstol(browserVersion, 0,
10);
This is guaranteed to be either 1 number or an empty string. Not a full semantic
version. 

You get version 9 for IE10, since we lack a special handling for IE10. (The
registry value should be srcVersion there
http://support.microsoft.com/kb/969393/en-us). Other then that you should have a
number here with a correct IE version.

I think this is the best way to check of we're on IE9 or later.

That being said, I don't think the menu bar was only hidden in IE9. IE8 had it
hidden by default as well. In IE7 it can be hidden as well, in fact. Not 100%
sure about IE6, but I think you can hide it there as well (not by default
though). 


In other words, I don't think we need a version check here at all. Just modify
the message for all versions - it should work.
On 2013/06/07 06:28:24, Felix H. Dahlke wrote:
> @Oleksandr:
> Can I rely on this being an integer? It's "9" for me (although I have IE10),
can
> this be a non-integer (e.g. a full semantic version string) in other IE
> versions?
> 
> Is there a better way to check if we're IE9 or newer?

[Felix Dahlke, 2013-06-07 08:02:47]

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

http://codereview.adblockplus.org/10800100/diff/3001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:578: int versionNumber = wcstol(browserVersion, 0,
10);
On 2013/06/07 07:16:46, Oleksandr wrote:

> IE8 had it
> hidden by default as well. In IE7 it can be hidden as well, in fact. Not 100%
> sure about IE6, but I think you can hide it there as well (not by default
> though). 

Hm, I thought it was shown by default in IE8. But I had to google around for
screenshots to figure that out, I wouldn't bet on that :)

The more important question is: Will right clicking the title bar always work? I
can only check with IE6 and IE10, but in IE6 at least it doesn't work.

[Wladimir Palant, 2013-06-07 08:48:29]

I'll trust you that dictionary_w.ini changes are correct :)

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

http://codereview.adblockplus.org/10800100/diff/3001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:578: int versionNumber = wcstol(browserVersion, 0,
10);
I looked through the IE versions:

* Custom title bar context menu was introduced in IE9, before that you would
right-click the menu bar to get that menu.
* The title/menu bar context menu contains a "Status Bar" entry in all IE
versions but IE6.
* IE6-8 have both menu and status bar displayed by default.
* The important menu entry is View / Status Bar in IE6 and IE7. In IE8 and IE9
it was moved to View / Toolbars / Status Bar (presumably still there in IE10 but
I don't have it).

We shouldn't worry about people hiding the menu bar if it isn't hidden by
default - people who do that will be able to get it back. Also, we shouldn't
tell people to use the menu if the menu is hidden by default.

IMHO we can have a single message: "Right-click the menu bar/title bar of the
Internet Explorer window and enable the status bar." It will work for all
versions with the exception of IE6. Given that pretty much all IE6 users have
the status bar enabled anyway, it should be acceptable to not display any hint
there.

Side-note: You should fix the GetBrowserVersion() function to return a number
rather than convert the string in the caller.

[Felix Dahlke, 2013-06-07 08:51:44]

Thanks for checking that :) It does seem like one message will do then. IE6
users are already a small fraction of our potential user base, IE6 users who
disabled the status bar yet don't know how to enable it seem like a pretty tiny
fraction. And that's only relevant if we couldn't toggle the status bar for them
anyway.

I'll change it to use just a single message then.

On 2013/06/07 08:48:29, Wladimir Palant wrote:
> I'll trust you that dictionary_w.ini changes are correct :)
> 
>
http://codereview.adblockplus.org/10800100/diff/3001/src/plugin/PluginClass.cpp
> File src/plugin/PluginClass.cpp (right):
> 
>
http://codereview.adblockplus.org/10800100/diff/3001/src/plugin/PluginClass.c...
> src/plugin/PluginClass.cpp:578: int versionNumber = wcstol(browserVersion, 0,
> 10);
> I looked through the IE versions:
> 
> * Custom title bar context menu was introduced in IE9, before that you would
> right-click the menu bar to get that menu.
> * The title/menu bar context menu contains a "Status Bar" entry in all IE
> versions but IE6.
> * IE6-8 have both menu and status bar displayed by default.
> * The important menu entry is View / Status Bar in IE6 and IE7. In IE8 and IE9
> it was moved to View / Toolbars / Status Bar (presumably still there in IE10
but
> I don't have it).
> 
> We shouldn't worry about people hiding the menu bar if it isn't hidden by
> default - people who do that will be able to get it back. Also, we shouldn't
> tell people to use the menu if the menu is hidden by default.
> 
> IMHO we can have a single message: "Right-click the menu bar/title bar of the
> Internet Explorer window and enable the status bar." It will work for all
> versions with the exception of IE6. Given that pretty much all IE6 users have
> the status bar enabled anyway, it should be acceptable to not display any hint
> there.
> 
> Side-note: You should fix the GetBrowserVersion() function to return a number
> rather than convert the string in the caller.

[Felix Dahlke, 2013-06-07 09:17:40]

There, new patch set.

[Oleksandr, 2013-06-07 09:20:25]

On 2013/06/07 09:17:40, Felix H. Dahlke wrote:
> There, new patch set.

I'd say "title >bar< or menu bar", to be extra specific. But I'm not a native
speaker, so LGTM :D

[Wladimir Palant, 2013-06-07 13:17:08]

Want to wait for my dictionary changes before pushing? I think merging your
string change shouldn't be a big deal :)

http://codereview.adblockplus.org/10800100/diff/10001/src/plugin/PluginDictio...
File src/plugin/PluginDictionary.cpp (right):

http://codereview.adblockplus.org/10800100/diff/10001/src/plugin/PluginDictio...
src/plugin/PluginDictionary.cpp:266:
m_dictionary["ERROR_CAN_NOT_ENABLE_STATUS_BAR"] = "The Adblock Plus menu is
located in the status bar. Please right click the title or menu bar and enable
the status bar.";
I would change the order: menu first, then title. If the menu is visible then
right-clicking the menu will always work. The title content menu won't help you
much in IE6-8 however.

[Felix Dahlke, 2013-06-07 15:31:09]

Merged with the dictionary changes and incorporated your feedback. Just pushed
this.