Full Screen Magnifier: Add MagnificationManager

Full Screen Magnifier: Add MagnificationManager

MagnificationManager controls the full screen magnifier from chrome-browser side (not ash side. Note that MagnificationController is on ash side).

MagnificationManager does:
- Watch logged-in. Change the behavior between the login screen and user desktop. 
- Watch change of the pref. When the pref changes, the setting of the magnifier will interlock with it.

In addition, this patch make the magnifier settings per-user instead of per-device.

BUG=151891
TEST=manual, browser_test passes.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162080

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162142

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169457

[yoshiki, 2012-10-05 04:06:23]

Zach, could you take a look?

[Zachary Kuznia, 2012-10-05 04:42:20]

http://codereview.chromium.org/11065008/diff/7002/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

http://codereview.chromium.org/11065008/diff/7002/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/magnification_manager.cc:72:
pref_change_registrar_->Add(prefs::kScreenMagnifierEnabled, this);
Do you need to remove from the old profile?

[yoshiki, 2012-10-11 04:03:23]

As chatted, I removed the almost all the part of the seting UI
(chrome://settings), to prevent conflict with zork's CL.

http://codereview.chromium.org/11065008/diff/7002/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

http://codereview.chromium.org/11065008/diff/7002/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/magnification_manager.cc:72:
pref_change_registrar_->Add(prefs::kScreenMagnifierEnabled, this);
On 2012/10/05 04:42:21, Zachary Kuznia wrote:
> Do you need to remove from the old profile?

Done.

[Zachary Kuznia, 2012-10-11 08:45:11]

lgtm

[yoshiki, 2012-10-11 09:11:35]

@sky, @davemoore, @jhawkins,
Can you take a look at following files as owners?

- @sky: ash/*
- @davemoore: chrome/browser/chromeos/*
- @jhawkins: chrome/browser/ui/webui/options/*

[James Hawkins, 2012-10-11 15:42:32]

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:1305: PrefService*
user_pref_service = Profile::FromWebUI(web_ui())->GetPrefs();
Can you explain this change?

[sky, 2012-10-11 17:29:58]

ash LGTM

[DaveMoore, 2012-10-11 20:09:27]

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
chrome/browser/chromeos/accessibility/magnification_manager.cc:62: #if
defined(USE_ASH)
Should there be a check here that it's not called in the non ash case?

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
chrome/browser/chromeos/accessibility/magnification_manager.cc:133:
MagnificationManager* MagnificationManager::CreateInstance() {
Please add a check that this isn't called more than once, or remove the comment
in the header that says it shouldn't be.

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
File chrome/browser/chromeos/accessibility/magnification_manager_browsertest.cc
(right):

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
chrome/browser/chromeos/accessibility/magnification_manager_browsertest.cc:29:
#include "testing/gtest/include/gtest/gtest.h"
I believe we are avoiding new uses of gmock. Adding Ben for comment

[yoshiki, 2012-10-12 15:00:54]

@davemoore, @jhawkins: PTAL? Thanks.

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
chrome/browser/chromeos/accessibility/magnification_manager.cc:62: #if
defined(USE_ASH)
On 2012/10/11 20:09:27, DaveMoore wrote:
> Should there be a check here that it's not called in the non ash case?

Done.

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
chrome/browser/chromeos/accessibility/magnification_manager.cc:133:
MagnificationManager* MagnificationManager::CreateInstance() {
On 2012/10/11 20:09:27, DaveMoore wrote:
> Please add a check that this isn't called more than once, or remove the
comment
> in the header that says it shouldn't be.

Done.

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
File chrome/browser/chromeos/accessibility/magnification_manager_browsertest.cc
(right):

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/ac...
chrome/browser/chromeos/accessibility/magnification_manager_browsertest.cc:29:
#include "testing/gtest/include/gtest/gtest.h"
On 2012/10/11 20:09:27, DaveMoore wrote:
> I believe we are avoiding new uses of gmock. Adding Ben for comment

Thanks, waiting for Ben's comment.

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/11065008/diff/12018/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:1305: PrefService*
user_pref_service = Profile::FromWebUI(web_ui())->GetPrefs();
On 2012/10/11 15:42:33, James Hawkins wrote:
> Can you explain this change?

This change makes the magnifier settings per-user instead of per-device. Added
this to the description.

[Ben Goodger (Google), 2012-10-12 16:45:02]

chrome gyp lgtm

[DaveMoore, 2012-10-12 16:57:25]

chromeos lgtm, still waiting for Ben to comment on use of gmock.

[Ben Goodger (Google), 2012-10-12 17:18:44]

On 2012/10/12 16:57:25, DaveMoore wrote:
> chromeos lgtm, still waiting for Ben to comment on use of gmock.

No gmock in chrome.

[yoshiki, 2012-10-16 03:49:43]

On 2012/10/12 17:18:44, Ben Goodger (Google) wrote:
> On 2012/10/12 16:57:25, DaveMoore wrote:
> > chromeos lgtm, still waiting for Ben to comment on use of gmock.
> 
> No gmock in chrome.

Historically gmock has been used in chrome/browser/chromeos and getting rid of
gmock from that directory is out of scope of this patch. I needed to add some
gmock code to suppress warnings from
chromeos/accessibility/magnification_manager_browsertest.cc. I'll talk to the
owner of the the directory about use of gmock there.

[commit-bot: I haz the power, 2012-10-16 03:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11065008/43003

[commit-bot: I haz the power, 2012-10-16 05:55:48]

Change committed as 162080

[commit-bot: I haz the power, 2012-10-16 14:29:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11065008/61001

[commit-bot: I haz the power, 2012-10-16 16:37:10]

Change committed as 162142

[Ben Goodger (Google), 2012-10-16 18:18:32]

On 2012/10/16 16:37:10, I haz the power (commit-bot) wrote:
> Change committed as 162142

I think it's fair to request that new uses of gmock not be added. I raised that
issue and you committed anyway. Please revert this CL immediately.

[yoshiki, 2012-10-17 06:16:05]

On 2012/10/16 18:18:32, Ben Goodger (Google) wrote:
> I think it's fair to request that new uses of gmock not be added. I raised
that
> issue and you committed anyway. Please revert this CL immediately.

Sorry I wasn't clear about why gmock was used in the patch. I needed to add some
gmock code because of dependencies to the existing code which needs existing
gmock-based mock classes, as well as other 'CrosInProcessBrowserTest'-based
browser tests in 'chrome/browser/chromeos'.

To avoid using gmock in the new browser test, we need to replace the 27 existing
gmock-based mock classes in 'chromeos/dbus' and eliminate them from many places
in 'chrome/browser/chromeos' directory (50 .cc/h files include "gmock.h"), which
will be a large undertaking. I think we should do it in the long term, but I
don't think that should block this patch. I'd propose to resubmit this patch for
now, and start gmock removal as a separate project. What do you think?

[yoshiki, 2012-10-23 00:31:08]

@ben, ping?

[yoshiki, 2012-11-16 07:43:03]

@Ben: ping on comment #19.

[yoshiki, 2012-11-21 07:57:53]

@ben: PTAL again. I removed the gmock part from the CL. The removal causes some
warnings from gmock when running tests because CrosInProcessBrowserTest uses
gmock, but these warnings will be disappeared as gmock is removed from
CrosInProcessBrowserTest.

[Ben Goodger (Google), 2012-11-26 16:40:25]

lgtm for new test.

[commit-bot: I haz the power, 2012-11-26 17:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11065008/83017

[commit-bot: I haz the power, 2012-11-26 19:20:31]

Change committed as 169457