Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 244003: Kiosk Mode implementation (Closed)

Created:
8 years, 12 months ago by Mohamed Mansour
Modified:
7 years, 3 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Kiosk Mode implementation. Kiosk mode will just hide the status bar and initially set it as full screen. Added some tests to add --kiosk mode as a command switch that tests if its in fullscreen state and doesn't have a status bubble. BUG=23145 TEST=Kiosk Mode functions and Run the ./ui_tests --gtest_filter=KioskModeTest.* Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31412

Patch Set 1 #

Total comments: 3

Patch Set 2 : Fix nits and param naming #

Total comments: 8

Patch Set 3 : Make use of the command dispatcher to disable commands for kiosk mode #

Total comments: 4

Patch Set 4 : Fix fullscreen bubble and remove unwanted right click context menus #

Total comments: 12

Patch Set 5 : Kiosk Mode implementation similiar to all other browers #

Total comments: 12

Patch Set 6 : Fix nits #

Patch Set 7 : Introduce Kiosk fullscreen ui test. #

Total comments: 6

Patch Set 8 : Add status bubble test #

Total comments: 4

Patch Set 9 : Fix nits #

Patch Set 10 : Fix unit test compilation #

Patch Set 11 : ifdef MAC since fullscreen is disabled #

Patch Set 12 : Rebase and try to fix test for mac, add build_config.h #

Patch Set 13 : Finally fix MACOSX #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -7 lines) Patch
M chrome/browser/automation/automation_provider.h View 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/automation/automation_provider.cc View 7 8 9 10 11 2 chunks +23 lines, -0 lines 0 comments Download
M chrome/browser/browser.h View 3 4 5 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/browser.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +13 lines, -0 lines 0 comments Download
M chrome/browser/browser_init.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/browser_uitest.cc View 7 8 9 10 11 12 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/browser/browser_window.h View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/browser_window_cocoa.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/cocoa/browser_window_cocoa.mm View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/gtk/browser_window_gtk.h View 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/gtk/browser_window_gtk.cc View 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/views/frame/browser_view.h View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/views/frame/browser_view.cc View 1 2 4 5 6 7 8 9 10 11 12 2 chunks +14 lines, -2 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +6 lines, -2 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +8 lines, -3 lines 0 comments Download
M chrome/test/automation/automation_messages_internal.h View 7 8 9 10 11 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/test/automation/browser_proxy.h View 7 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/test/automation/browser_proxy.cc View 7 8 9 1 chunk +20 lines, -0 lines 0 comments Download
M chrome/test/test_browser_window.h View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
Evan Stade
http://codereview.chromium.org/244003/diff/1/4 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/244003/diff/1/4#newcode830 Line 830: if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kKiosk)) this new conditional not necessary. http://codereview.chromium.org/244003/diff/1/4#newcode862 ...
8 years, 12 months ago (2009-09-25 20:44:00 UTC) #1
Mohamed Mansour
I have done what you told me, but do you think this is a good ...
8 years, 11 months ago (2009-09-26 17:39:39 UTC) #2
Evan Stade
well, to avoid unnecessary string comparisons, you might cache the result as a member variable ...
8 years, 11 months ago (2009-09-26 21:16:13 UTC) #3
Ben Goodger (Google)
http://codereview.chromium.org/244003/diff/3001/4002 File chrome/browser/browser_init.cc (right): http://codereview.chromium.org/244003/diff/3001/4002#newcode557 Line 557: browser->window()->SetFullscreen(true); Do you have further plans for this? ...
8 years, 11 months ago (2009-09-27 18:00:57 UTC) #4
Mohamed Mansour
I thought about that, but kiosk mode is a bit more than full screen. For ...
8 years, 11 months ago (2009-09-27 18:13:41 UTC) #5
Peter Kasting
On 2009/09/27 18:13:41, Mohamed Mansour wrote: > - Title bar, menus, toolbar, status bar are ...
8 years, 11 months ago (2009-09-28 01:09:55 UTC) #6
Mohamed Mansour
Thanks Peter for your suggestion, its much cleaner now and commands are disabled in kiosk ...
8 years, 11 months ago (2009-09-28 03:57:40 UTC) #7
Peter Kasting
On 2009/09/28 03:57:40, Mohamed Mansour wrote: > Implementing "nopopups" could be a future feature, I ...
8 years, 11 months ago (2009-09-28 05:40:04 UTC) #8
Peter Kasting
http://codereview.chromium.org/244003/diff/3001/4003 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/244003/diff/3001/4003#newcode834 Line 834: if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode)) On 2009/09/28 03:57:40, Mohamed Mansour wrote: ...
8 years, 11 months ago (2009-09-28 05:40:22 UTC) #9
M-A Ruel
Please don't check-in that without a full set of unit tests. This is the kind ...
8 years, 11 months ago (2009-09-28 17:51:58 UTC) #10
Mohamed Mansour
On 2009/09/28 17:51:58, Marc-Antoine Ruel wrote: > Please don't check-in that without a full set ...
8 years, 11 months ago (2009-09-29 04:55:02 UTC) #11
Mohamed Mansour
Peter, thanks for the comments, I tried to follow what you said. In addition to ...
8 years, 11 months ago (2009-09-29 04:58:52 UTC) #12
Peter Kasting
http://codereview.chromium.org/244003/diff/5002/2004 File chrome/browser/browser.cc (left): http://codereview.chromium.org/244003/diff/5002/2004#oldcode2371 Line 2371: // Initialize other commands whose state changes based ...
8 years, 11 months ago (2009-09-29 17:11:20 UTC) #13
Mohamed Mansour
I rebased this CL and removed all the unwanted conditions. Kiosk mode should "only" be ...
8 years, 10 months ago (2009-11-06 01:04:06 UTC) #14
Peter Kasting
http://codereview.chromium.org/244003/diff/10001/11001 File chrome/browser/browser.cc (right): http://codereview.chromium.org/244003/diff/10001/11001#newcode920 Line 920: // Do not exit full screen if its ...
8 years, 10 months ago (2009-11-06 01:19:14 UTC) #15
Mohamed Mansour
Fixed the nits. http://codereview.chromium.org/244003/diff/10001/11001 File chrome/browser/browser.cc (right): http://codereview.chromium.org/244003/diff/10001/11001#newcode920 Line 920: // Do not exit full ...
8 years, 10 months ago (2009-11-06 02:36:56 UTC) #16
M-A Ruel
still need unit tests.
8 years, 10 months ago (2009-11-06 02:39:59 UTC) #17
Peter Kasting
On 2009/11/06 02:39:59, Marc-Antoine Ruel wrote: > still need unit tests. What can be tested ...
8 years, 10 months ago (2009-11-06 02:56:50 UTC) #18
M-A Ruel
On 2009/11/06 02:56:50, Peter Kasting wrote: > On 2009/11/06 02:39:59, Marc-Antoine Ruel wrote: > > ...
8 years, 10 months ago (2009-11-06 03:01:13 UTC) #19
Peter Kasting
On 2009/11/06 03:01:13, Marc-Antoine Ruel wrote: > As an example; it'll be too late in ...
8 years, 10 months ago (2009-11-06 03:16:17 UTC) #20
Evan Stade
ui_controls + view id
8 years, 10 months ago (2009-11-06 03:24:07 UTC) #21
Mohamed Mansour
Here you go. A test that goes with it and it works. (tested locally)
8 years, 10 months ago (2009-11-06 05:26:24 UTC) #22
Peter Kasting
Any way to test the lack of status bubble? http://codereview.chromium.org/244003/diff/15003/14003 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/244003/diff/15003/14003#newcode1116 Line ...
8 years, 10 months ago (2009-11-06 05:55:16 UTC) #23
M-A Ruel
+ Peter's comments except one. :) http://codereview.chromium.org/244003/diff/15003/14003 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/244003/diff/15003/14003#newcode1116 Line 1116: if (browser) ...
8 years, 10 months ago (2009-11-06 13:18:15 UTC) #24
Mohamed Mansour
On 2009/11/06 05:55:16, Peter Kasting wrote: > Any way to test the lack of status ...
8 years, 10 months ago (2009-11-06 14:40:40 UTC) #25
M-A Ruel
http://codereview.chromium.org/244003/diff/12004/12007 File chrome/browser/browser.cc (right): http://codereview.chromium.org/244003/diff/12004/12007#newcode918 Line 918: // In kiosk mode, we always want to ...
8 years, 10 months ago (2009-11-06 14:44:17 UTC) #26
Peter Kasting
On 2009/11/06 13:18:15, Marc-Antoine Ruel wrote: > On 2009/11/06 05:55:17, Peter Kasting wrote: > > ...
8 years, 10 months ago (2009-11-06 19:25:44 UTC) #27
Mohamed Mansour
http://codereview.chromium.org/244003/diff/15003/14003 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/244003/diff/15003/14003#newcode1116 Line 1116: if (browser) { On 2009/11/06 05:55:17, Peter Kasting ...
8 years, 10 months ago (2009-11-07 06:15:26 UTC) #28
M-A Ruel
compile failure
8 years, 10 months ago (2009-11-07 13:33:02 UTC) #29
Mohamed Mansour
Fixed unit_tests compilation.
8 years, 10 months ago (2009-11-07 15:11:49 UTC) #30
Mohamed Mansour
But ui_tests for kiosk fail on the mac :(
8 years, 10 months ago (2009-11-07 15:22:17 UTC) #31
M-A Ruel
On 2009/11/07 15:22:17, Mohamed Mansour wrote: > But ui_tests for kiosk fail on the mac ...
8 years, 10 months ago (2009-11-07 21:01:21 UTC) #32
Mohamed Mansour
Don't worry about it, I would have to ifdef the test out because pinkerton said ...
8 years, 10 months ago (2009-11-07 23:26:23 UTC) #33
M-A Ruel
On 2009/11/07 23:26:23, Mohamed Mansour wrote: > Don't worry about it, I would have to ...
8 years, 10 months ago (2009-11-08 00:29:16 UTC) #34
Mohamed Mansour
Hrmm, I ifdefd MAC for the kiosk test, and its still somehow reading it. Can ...
8 years, 10 months ago (2009-11-08 02:42:31 UTC) #35
M-A Ruel
On 2009/11/08 02:42:31, Mohamed Mansour wrote: > Hrmm, I ifdefd MAC for the kiosk test, ...
8 years, 10 months ago (2009-11-08 16:42:51 UTC) #36
Mohamed Mansour
No go :( It is still running that test on the Mac even after I ...
8 years, 10 months ago (2009-11-08 18:09:56 UTC) #37
Mohamed Mansour
Finally its fixed now, thanks maruel! I will be away next week with no Internet, ...
8 years, 10 months ago (2009-11-08 19:24:54 UTC) #38
M-A Ruel
8 years, 10 months ago (2009-11-08 20:22:48 UTC) #39
lgtm

Powered by Google App Engine
This is Rietveld 408576698