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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by yoshiki
Modified:
1 year, 4 months ago
CC:
chromium-reviews_chromium.org, 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) Lint Patch
M ash/magnifier/magnification_controller.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments 0 errors 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 ? errors 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 0 errors 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 ? errors 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 3 errors 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 ? errors 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 ? errors 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 0 errors 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 ? errors Download
M chrome/browser/chromeos/preferences.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments ? errors Download
M chrome/browser/chromeos/preferences.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments ? errors 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 ? errors 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 ? errors Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 25
yoshiki
Zach, could you take a look?
1 year, 6 months ago #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 ...
1 year, 6 months ago #2
yoshiki
As chatted, I removed the almost all the part of the seting UI (chrome://settings), to ...
1 year, 6 months ago #3
Zachary Kuznia
lgtm
1 year, 6 months ago #4
yoshiki
@sky, @davemoore, @jhawkins, Can you take a look at following files as owners? - @sky: ...
1 year, 6 months ago #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?
1 year, 6 months ago #6
sky
ash LGTM
1 year, 6 months ago #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 ...
1 year, 6 months ago #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, ...
1 year, 6 months ago #9
Ben Goodger (Google)
chrome gyp lgtm
1 year, 6 months ago #10
DaveMoore
chromeos lgtm, still waiting for Ben to comment on use of gmock.
1 year, 6 months ago #11
Ben Goodger (Google)
On 2012/10/12 16:57:25, DaveMoore wrote: > chromeos lgtm, still waiting for Ben to comment on ...
1 year, 6 months ago #12
yoshiki
On 2012/10/12 17:18:44, Ben Goodger (Google) wrote: > On 2012/10/12 16:57:25, DaveMoore wrote: > > ...
1 year, 6 months ago #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
1 year, 6 months ago #14
I haz the power (commit-bot)
Change committed as 162080
1 year, 6 months ago #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
1 year, 6 months ago #16
I haz the power (commit-bot)
Change committed as 162142
1 year, 6 months ago #17
Ben Goodger (Google)
On 2012/10/16 16:37:10, I haz the power (commit-bot) wrote: > Change committed as 162142 I ...
1 year, 6 months ago #18
yoshiki
On 2012/10/16 18:18:32, Ben Goodger (Google) wrote: > I think it's fair to request that ...
1 year, 6 months ago #19
yoshiki
@ben, ping?
1 year, 5 months ago #20
yoshiki
@Ben: ping on comment #19.
1 year, 5 months ago #21
yoshiki
@ben: PTAL again. I removed the gmock part from the CL. The removal causes some ...
1 year, 4 months ago #22
Ben Goodger (Google)
lgtm for new test.
1 year, 4 months ago #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
1 year, 4 months ago #24
I haz the power (commit-bot)
1 year, 4 months ago #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 1280:2d3e6564b7b6