Added support for Text only zoom...

Added support for Text only zoom

the keys to use are Ctrl+Shift+(+/-) and Ctrl+Shift+MouseWheel and also
it supports ThinkPads (tested with one)

TEST=1- look at any web page
     2- use Shift-Ctrl-+ and see it zoom only in text mode

BUG=6383
(http://code.google.com/p/chromium/issues/detail?id=6383)

[nakro, 2009-06-26 11:31:29]

Several issues to keep in mind
- There seems to be a bug in vista where Ctrl+Shift+0 does not get passes to
WM_KEYDOWN
  i have verified it, and if you google it you will find others encountered this
as well
  this does not affect the code and the code i replaced also had this
combination
  which never worked.
  (it is possible this bug only appears on hebrew enabled vista, but i have no
   way to verify it)
- There are issues with Synaptec (known maker of touchpads) that it uses the ALT
key
  to make the regions on the touchpad who were marked for scrolling
  to mean "treat it as a normal area",
  so this means that by chance, the Ctrl+shift approach is better
  than the Alt+wheel approach
- It seems that Synaptec does not generate WM_MOUSEWHEEL as often
  when the Ctrl or Ctrl+Shift are pressed, i have verified this
  but i don't know the cause, their site mentioned nothing about this
  (or i could not locate it)
  still, you do get the event, only in a lower gnaularity
- I have no access to unix/mac, so i could not make sure this even
  compiles on Posix, but i did my best to make sure,
  i had to modify some GTK code as i changed 'ContentsZoomChange' in
  'browser.h'....

One sad note : i first submitted one extra file, and not knowing how to modify
the update list, i deleted that issue and re-submitted without it..... i guess i
could have modified the gcl file file, but i wasn't sure what it would do...
sorry

[Evan Martin, 2009-06-26 15:26:33]

http://codereview.chromium.org/147202/diff/1/13
File chrome/common/page_zoom.h (right):

http://codereview.chromium.org/147202/diff/1/13#newcode21
Line 21: PageZoom() {}  // For scoping only.
#include "base/basictypes.h"
above

DISALLOW_IMPLICIT_CONSTRUCTORS(PageZoom)
here

[Mohamed Mansour, 2009-06-26 15:53:49]

Good job, just a few lints. Make sure you update the copyright on the files you
have touched, it should be written as "Copyright (c) 2009 The Chromium Authors "
...

http://codereview.chromium.org/147202/diff/1/2
File chrome/app/chrome_dll_resource.h (right):

http://codereview.chromium.org/147202/diff/1/2#newcode4
Line 4: // edited by hand.  Do not generate!!!
You must not edit this file, it is auto generated.

http://codereview.chromium.org/147202/diff/1/9
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/147202/diff/1/9#newcode81
Line 81: virtual void ContentsZoomChange(PageZoom::Function zoom_type);
as well include page_zoom.h here.

http://codereview.chromium.org/147202/diff/1/11
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/147202/diff/1/11#newcode79
Line 79: tab_contents->delegate()->ContentsZoomChange(PageZoom::SMALLER);
You include header file (page_zoom.h) whenever your using it. So include it in
this file. Many other places are the same thing. Its safer this way.

http://codereview.chromium.org/147202/diff/1/6
File chrome/browser/views/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/147202/diff/1/6#newcode74
Line 74: tab_contents->delegate()->ContentsZoomChange(PageZoom::SMALLER);
here too, include page_zoom.h

[jeremy, 2009-06-26 15:57:41]

This really looks great, thanks for doing this!

In the mail you sent with the review you document some behavior peculiar to
Windows (or perhaps just one version of Windows), could you possibly add a short
comment in an appropriate place in the code with this info so it's easy to find?

http://codereview.chromium.org/147202/diff/1/12
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/147202/diff/1/12#newcode1840
Line 1840: int zoom_requested;
I don't know if this is a valid concern, but to be defensive, could you assign
-1 or some other obvious invalid value to zoom_requested here?

[jeremy, 2009-06-26 15:58:58]

Mohammed: My understanding is that there is no need to updated the copyright
date on existing files, just use the current year in the header when creating
new ones.

[nakro, 2009-06-26 16:44:56]

i have fixed all issues except the one raised by mohamed as i could not find who
generates 'chrome_dll_resource.h' file
and it appears it is not generated at all

also, i have several  'tab_contents_view_gtk.cc' on my disk but all in excluded
folders, so i figured they are just some left overs ????

when the first person who tries to compile it on unix/mac has a problem, have
him e-mail me and i can help, and maybe it will all work just fine!

http://codereview.chromium.org/147202/diff/1/2
File chrome/app/chrome_dll_resource.h (right):

http://codereview.chromium.org/147202/diff/1/2#newcode4
Line 4: // edited by hand.  Do not generate!!!
If this file is auto generated, there should have been a 
"this file is auto generated warning"

if it is indeed auto generated it should also have been in the debug folder
somewhere

but ok, i am kool with it
who is the master file who generates this ? i could not locate it

for example, i did
http://www.google.com/codesearch?q=IDR_SAD_PLUGIN+package:"src.chromium.org/s...

and could not see any file who seems to generate it
but i am sure it is my lack of knowledge, show me the way please....

http://codereview.chromium.org/147202/diff/1/12
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/147202/diff/1/12#newcode1840
Line 1840: int zoom_requested;
On 2009/06/26 15:57:41, jeremy wrote:
> I don't know if this is a valid concern, but to be defensive, could you assign
> -1 or some other obvious invalid value to zoom_requested here?

Done.

http://codereview.chromium.org/147202/diff/1/9
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/147202/diff/1/9#newcode81
Line 81: virtual void ContentsZoomChange(PageZoom::Function zoom_type);
On 2009/06/26 15:53:49, Mohamed Mansour wrote:
> as well include page_zoom.h here.

Done.

http://codereview.chromium.org/147202/diff/1/11
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/147202/diff/1/11#newcode79
Line 79: tab_contents->delegate()->ContentsZoomChange(PageZoom::SMALLER);
On 2009/06/26 15:53:49, Mohamed Mansour wrote:
> You include header file (page_zoom.h) whenever your using it. So include it in
> this file. Many other places are the same thing. Its safer this way.

Done.

http://codereview.chromium.org/147202/diff/1/13
File chrome/common/page_zoom.h (right):

http://codereview.chromium.org/147202/diff/1/13#newcode21
Line 21: PageZoom() {}  // For scoping only.
On 2009/06/26 15:26:33, Evan Martin wrote:
> #include "base/basictypes.h"
> above
> 
> DISALLOW_IMPLICIT_CONSTRUCTORS(PageZoom)
> here

Done.

[brettw, 2009-06-26 17:00:48]

http://codereview.chromium.org/147202/diff/1/12
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/147202/diff/1/12#newcode1840
Line 1840: int zoom_requested;
On 2009/06/26 15:57:41, jeremy wrote:
> I don't know if this is a valid concern, but to be defensive, could you assign
> -1 or some other obvious invalid value to zoom_requested here?

I would probably use STANDARD as the default here.

http://codereview.chromium.org/147202/diff/1/12#newcode1843
Line 1843: case PageZoom::TEXT_ONLY_SMALLER :
Don't add spaces before the colons.

http://codereview.chromium.org/147202/diff/1/7
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/147202/diff/1/7#newcode605
Line 605: const int zoom_text_only = GetAsyncKeyState(VK_SHIFT) & 0x8000;
In the bug I suggested Ctrl+Alt rather than Ctrl+Shift. The reason is that the
hotkey for "zoom in" is listed as Ctrl-Plus which actually requires the use of
the shift key. Because of this confusion, I would prefer to keep this command
shift-neutral so you can do either.

[jorlow, 2009-06-26 17:10:26]

On 2009/06/26 15:58:58, jeremy wrote:
> Mohammed: My understanding is that there is no need to updated the copyright
> date on existing files, just use the current year in the header when creating
> new ones.

No, you _are_ supposed to update the date whenever you touch a file that doesn't
have the current date in the copyright.

[nakro, 2009-06-26 17:32:40]

brettw, i only had have one question and when i get the answer i will post
(hopefully) the final patch

http://codereview.chromium.org/147202/diff/1/7
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/147202/diff/1/7#newcode605
Line 605: const int zoom_text_only = GetAsyncKeyState(VK_SHIFT) & 0x8000;
I am aware of this, but i did check and FF and chrome work with (Ctrl) + (=) as
nobody bothers to add the shift

but no problem, just keep in mind that if you are sure about this, you will not
have text only zoom on thinkpads, as the ALT disables the "scroll area" (at
least with MY synaptec drivers, i even tried the latest in the hope they solved
it)

so which do you prefer ? Alt+Ctrl for keybaord
and Shift + Ctrl for mouse ? or all Alt + Ctrl and accept no thinkpad support
for txt only zoom?

[nakro, 2009-06-26 18:00:10]

Brett, just to make it clear, you can also chose that i would add a menu item
(like FF and dafari) and then this solves this ambiguity
as there is only one Ctrl+(key | wheel)
perhaps with a hot-key to toggle it, so you don't have to navigate to the menu
each time you change your mind

anyways, i will await your decision