Chromium Code Reviews
Help | Chromium Project | Sign in
(10)

Issue 11065008: Full Screen Magnifier: Add MagnificationManager (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by yoshiki
Modified:
2 years, 4 months ago
CC:
chromium-reviews, dtseng+watch_chromium.org, sadrul, nkostylev+watch_chromium.org, hashimoto+watch_chromium.org, ben+watch_chromium.org, aboxhall+watch_chromium.org, yoshiki+watch_chromium.org, yuzo+watch_chromium.org, davidbarr+watch_chromium.org, arv, dmazzoni+watch_chromium.org, oshima+watch_chromium.org, ctguil+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, zork+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Added comment. #

Total comments: 2

Patch Set 3 : rebase #

Patch Set 4 : Removed the modification about the seting UI (chrome://settings). #

Patch Set 5 : Review fix (#2) #

Patch Set 6 : fix typo. #

Total comments: 8

Patch Set 7 : Review fix (#8) #

Patch Set 8 : rebase @168708 #

Patch Set 9 : Remove gmock from the CL #

Patch Set 10 : rebase @169437 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+343 lines, -42 lines) Patch
M ash/magnifier/magnification_controller.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/accessibility/accessibility_util.h View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/accessibility/accessibility_util.cc View 1 2 3 4 5 6 7 4 chunks +8 lines, -26 lines 0 comments Download
A chrome/browser/chromeos/accessibility/magnification_manager.h View 1 2 3 4 5 6 7 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/accessibility/magnification_manager.cc View 1 2 3 4 5 6 7 1 chunk +148 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/accessibility/magnification_manager_browsertest.cc View 1 2 3 4 5 6 7 8 1 chunk +119 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/chrome_browser_main_chromeos.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/chrome_browser_main_chromeos.cc View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/login/wizard_controller.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/chromeos/preferences.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/preferences.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/options/browser_options_handler.cc View 1 2 3 4 5 6 7 2 chunks +10 lines, -11 lines 0 comments Download
M chrome/chrome_browser_chromeos.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 25 (0 generated)
yoshiki
Zach, could you take a look?
2 years, 5 months ago (2012-10-05 04:06:23 UTC) #1
Zachary Kuznia
http://codereview.chromium.org/11065008/diff/7002/chrome/browser/chromeos/accessibility/magnification_manager.cc File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): http://codereview.chromium.org/11065008/diff/7002/chrome/browser/chromeos/accessibility/magnification_manager.cc#newcode72 chrome/browser/chromeos/accessibility/magnification_manager.cc:72: pref_change_registrar_->Add(prefs::kScreenMagnifierEnabled, this); Do you need to remove from the ...
2 years, 5 months ago (2012-10-05 04:42:20 UTC) #2
yoshiki
As chatted, I removed the almost all the part of the seting UI (chrome://settings), to ...
2 years, 5 months ago (2012-10-11 04:03:23 UTC) #3
Zachary Kuznia
lgtm
2 years, 5 months ago (2012-10-11 08:45:11 UTC) #4
yoshiki
@sky, @davemoore, @jhawkins, Can you take a look at following files as owners? - @sky: ...
2 years, 5 months ago (2012-10-11 09:11:35 UTC) #5
James Hawkins
http://codereview.chromium.org/11065008/diff/12018/chrome/browser/ui/webui/options/browser_options_handler.cc File chrome/browser/ui/webui/options/browser_options_handler.cc (right): http://codereview.chromium.org/11065008/diff/12018/chrome/browser/ui/webui/options/browser_options_handler.cc#newcode1305 chrome/browser/ui/webui/options/browser_options_handler.cc:1305: PrefService* user_pref_service = Profile::FromWebUI(web_ui())->GetPrefs(); Can you explain this change?
2 years, 5 months ago (2012-10-11 15:42:32 UTC) #6
sky
ash LGTM
2 years, 5 months ago (2012-10-11 17:29:58 UTC) #7
DaveMoore
http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/accessibility/magnification_manager.cc File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/accessibility/magnification_manager.cc#newcode62 chrome/browser/chromeos/accessibility/magnification_manager.cc:62: #if defined(USE_ASH) Should there be a check here that ...
2 years, 5 months ago (2012-10-11 20:09:27 UTC) #8
yoshiki
@davemoore, @jhawkins: PTAL? Thanks. http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/accessibility/magnification_manager.cc File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): http://codereview.chromium.org/11065008/diff/12018/chrome/browser/chromeos/accessibility/magnification_manager.cc#newcode62 chrome/browser/chromeos/accessibility/magnification_manager.cc:62: #if defined(USE_ASH) On 2012/10/11 20:09:27, ...
2 years, 5 months ago (2012-10-12 15:00:54 UTC) #9
Ben Goodger (Google)
chrome gyp lgtm
2 years, 5 months ago (2012-10-12 16:45:02 UTC) #10
DaveMoore
chromeos lgtm, still waiting for Ben to comment on use of gmock.
2 years, 5 months ago (2012-10-12 16:57:25 UTC) #11
Ben Goodger (Google)
On 2012/10/12 16:57:25, DaveMoore wrote: > chromeos lgtm, still waiting for Ben to comment on ...
2 years, 5 months ago (2012-10-12 17:18:44 UTC) #12
yoshiki
On 2012/10/12 17:18:44, Ben Goodger (Google) wrote: > On 2012/10/12 16:57:25, DaveMoore wrote: > > ...
2 years, 5 months ago (2012-10-16 03:49:43 UTC) #13
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11065008/43003
2 years, 5 months ago (2012-10-16 03:59:27 UTC) #14
I haz the power (commit-bot)
Change committed as 162080
2 years, 5 months ago (2012-10-16 05:55:48 UTC) #15
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11065008/61001
2 years, 5 months ago (2012-10-16 14:29:41 UTC) #16
I haz the power (commit-bot)
Change committed as 162142
2 years, 5 months ago (2012-10-16 16:37:10 UTC) #17
Ben Goodger (Google)
On 2012/10/16 16:37:10, I haz the power (commit-bot) wrote: > Change committed as 162142 I ...
2 years, 5 months ago (2012-10-16 18:18:32 UTC) #18
yoshiki
On 2012/10/16 18:18:32, Ben Goodger (Google) wrote: > I think it's fair to request that ...
2 years, 5 months ago (2012-10-17 06:16:05 UTC) #19
yoshiki
@ben, ping?
2 years, 5 months ago (2012-10-23 00:31:08 UTC) #20
yoshiki
@Ben: ping on comment #19.
2 years, 4 months ago (2012-11-16 07:43:03 UTC) #21
yoshiki
@ben: PTAL again. I removed the gmock part from the CL. The removal causes some ...
2 years, 4 months ago (2012-11-21 07:57:53 UTC) #22
Ben Goodger (Google)
lgtm for new test.
2 years, 4 months ago (2012-11-26 16:40:25 UTC) #23
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11065008/83017
2 years, 4 months ago (2012-11-26 17:18:07 UTC) #24
I haz the power (commit-bot)
2 years, 4 months ago (2012-11-26 19:20:31 UTC) #25
Message was sent while issue was closed.
Change committed as 169457
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cf4c24d