Kiosk Mode implementation

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

[Evan Stade, 2009-09-25 20:44:00]

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
Line 862: if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kKiosk))
nit: use {}

http://codereview.chromium.org/244003/diff/1/5
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/244003/diff/1/5#newcode607
Line 607: const wchar_t kKiosk[] = L"kiosk";
nit: can we call this kKioskMode

[Mohamed Mansour, 2009-09-26 17:39:39]

I have done what you told me, but do you think this is a good way to do it? I am
basically setting the screen as fullscreen (with no undo) and blocking the
status bar from showing.

It still seems sketchy since I am checking for the kKioskMode everytime I
request a status bar, thats still O(1) operation but under normal conditions
(normal uses) it doesn't need that check.

[Evan Stade, 2009-09-26 21:16:13]

well, to avoid unnecessary string comparisons, you might cache the result as a
member variable on Browser.

[Ben Goodger (Google), 2009-09-27 18:00:57]

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? If so, what are they?

As it stands I think this switch would be better worded as just "--fullscreen"

[Mohamed Mansour, 2009-09-27 18:13:41]

I thought about that, but kiosk mode is a bit more than full screen. For
example, as far as I know, kiosk mode is:
- Title bar, menus, toolbar, status bar are not displayed.
- Run in full screen mode.
- Keyboard shortcuts differ such as no more F11, no more close window/tab
shortcuts, just ALT+F4 to close.
- No tabs nor popups. Just a single window.

I like Peter to comment as well as the UI team, on the dev mailing list, they
agreed for such implementation of Kiosk Mode.

[Peter Kasting, 2009-09-28 01:09:55]

On 2009/09/27 18:13:41, Mohamed Mansour wrote:
> - Title bar, menus, toolbar, status bar are not displayed.

Except for the status bubble all these are just part of fullscreen mode.

> - Keyboard shortcuts differ such as no more F11, no more close window/tab
> shortcuts, just ALT+F4 to close.

You haven't implemented this for the most part.  See one of my comments below.

> - No tabs nor popups. Just a single window.

How do you propose to implement "no popups"?

http://codereview.chromium.org/244003/diff/3001/4001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/244003/diff/3001/4001#newcode922
Line 922: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
Nit: I think you should move this to the command dispatcher.  Perhaps you should
have some sort of function that disables commands that shouldn't be available in
kiosk mode, including IDC_FULLSCREEN.

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);
Nit: Call browser->ToggleFullscreen() instead of these (see comment there about
moving the kiosk mode check)

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))
Why do you need to do this?  You shouldn't reach here in kiosk mode.

http://codereview.chromium.org/244003/diff/3001/4003#newcode865
Line 865: if (IsFullscreen()) { {
Typo

[Mohamed Mansour, 2009-09-28 03:57:40]

Thanks Peter for your suggestion, its much cleaner now and commands are disabled
in kiosk mode.

Implementing "nopopups" could be a future feature, I don't think IE has that. I
think, we would need to intercept OpenURL for POPUP dispositions (similar).

Can you please re-review, and let me know what the best way to remove the full
screen bubble without putting a kKioskMode condition like my first CL.

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 01:09:55, Peter Kasting wrote:
> Why do you need to do this?  You shouldn't reach here in kiosk mode.

I needed that so that I could remove the "fullscreen" bubble, don't you think it
is in the right place here? How else would I not show the full screen bubble. We
cannot have that for kiosk mode. In my current CL I removed it, awaiting for
some feedback for that.

[Peter Kasting, 2009-09-28 05:40:04]

On 2009/09/28 03:57:40, Mohamed Mansour wrote:
> Implementing "nopopups" could be a future feature, I don't think IE has that.

You're pretty hosed without it if a site actually opens a popup, because you
don't have an easy way to close.  Seems like we should do this...

[Peter Kasting, 2009-09-28 05:40:22]

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:
> On 2009/09/28 01:09:55, Peter Kasting wrote:
> > Why do you need to do this?  You shouldn't reach here in kiosk mode.
> 
> I needed that so that I could remove the "fullscreen" bubble, don't you think
it
> is in the right place here?

No, this is code that runs when you're toggling fullscreen mode _off_.  If it is
somehow hiding the fullscreen bubble, something is very confused.  Given your
typo, I wonder if you actually test-compiled-and-ran your patch?

http://codereview.chromium.org/244003/diff/1003/15
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/244003/diff/1003/15#newcode726
Line 726: command_updater_.UpdateCommandEnabled(IDC_RESTORE_TAB, show_main_ui);
Nearly all the other browser commands should be disabled too, shouldn't they? 
Since you shouldn't be able to actually trigger most of them.

Instead of a function here that gets called after the main InitCommandState()
code runs, perhaps you should just check kiosk mode atop that function and
change most of the "true"s there to "is_kiosk", and have |normal_window| be
calculated with an added "&& !is_kiosk"?  You'll also need to fiddle with the
UpdateCommandsForXXX() functions...

http://codereview.chromium.org/244003/diff/1003/17
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/244003/diff/1003/17#newcode556
Line 556: if (command_line_.HasSwitch(switches::kKioskMode)) {
Nit: No {}

[M-A Ruel, 2009-09-28 17:51:58]

Please don't check-in that without a full set of unit tests. This is the kind of
feature that will get very low coverage and could be broken for a long time
before anyone noticing. Also, I don't think this will ever be in the manual QA
loop so your only hope to keep this feature workings is unit and ui tests.

[Mohamed Mansour, 2009-09-29 04:55:02]

On 2009/09/28 17:51:58, Marc-Antoine Ruel wrote:
> Please don't check-in that without a full set of unit tests. This is the kind
of
> feature that will get very low coverage and could be broken for a long time
> before anyone noticing. Also, I don't think this will ever be in the manual QA
> loop so your only hope to keep this feature workings is unit and ui tests.
Correct, I will do that tomorrow.

[Mohamed Mansour, 2009-09-29 04:58:52]

Peter, thanks for the comments, I tried to follow what you said. In addition to
this change, I have removed the unwanted context menu items from the renderer,
such as "open in new tab", "open in new window", etc, since they are not needed.
If there is a cleaner way, I am listening. 

I was mistaken regarding the check for the fullscreen bubble, it was supposed to
be in the second if condition (I tested this time). That check is needed to not
instantiate the fullscreen bubble, if I didn't put it there, the bubble will
appear.

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 05:40:22, Peter Kasting wrote:
> No, this is code that runs when you're toggling fullscreen mode _off_.  If it
is
> somehow hiding the fullscreen bubble, something is very confused.  Given your
> typo, I wonder if you actually test-compiled-and-ran your patch?

Sorry, It was the other if condition, a mistake in my end.

http://codereview.chromium.org/244003/diff/1003/15
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/244003/diff/1003/15#newcode726
Line 726: command_updater_.UpdateCommandEnabled(IDC_RESTORE_TAB, show_main_ui);
On 2009/09/28 05:40:22, Peter Kasting wrote:
> Nearly all the other browser commands should be disabled too, shouldn't they? 
> Since you shouldn't be able to actually trigger most of them.
> 
> Instead of a function here that gets called after the main InitCommandState()
> code runs, perhaps you should just check kiosk mode atop that function and
> change most of the "true"s there to "is_kiosk", and have |normal_window| be
> calculated with an added "&& !is_kiosk"?  You'll also need to fiddle with the
> UpdateCommandsForXXX() functions...

Done.

http://codereview.chromium.org/244003/diff/1003/17
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/244003/diff/1003/17#newcode556
Line 556: if (command_line_.HasSwitch(switches::kKioskMode)) {
On 2009/09/28 05:40:22, Peter Kasting wrote:
> Nit: No {}

Done.

[Peter Kasting, 2009-09-29 17:11:20]

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 on fullscreen
mode.
Nit: Why remove this comment?

http://codereview.chromium.org/244003/diff/5002/2004
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/244003/diff/5002/2004#newcode2265
Line 2265: command_updater_.UpdateCommandEnabled(IDC_CLOSE_POPUPS, true);
Why are all these (and the three blocks below as well) still on?  Doesn't seem
like we should have these?

http://codereview.chromium.org/244003/diff/5002/2004#newcode2324
Line 2324: command_updater_.UpdateCommandEnabled(IDC_CREATE_SHORTCUTS,
!is_kiosk_);
I think this should be false.

http://codereview.chromium.org/244003/diff/5002/2004#newcode2333
Line 2333: command_updater_.UpdateCommandEnabled(IDC_SAVE_PAGE, !is_kiosk_);
Why add these last two?

http://codereview.chromium.org/244003/diff/5002/2004#newcode2341
Line 2341: bool normal_window = type() == TYPE_NORMAL && !is_kiosk_;
Nit: Add () around first condition for clarity.

http://codereview.chromium.org/244003/diff/5002/2004#newcode2555
Line 2555: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
Nit: Can't you check |is_kiosk_| here?

http://codereview.chromium.org/244003/diff/5002/2004#newcode2752
Line 2752: // In Kiosk mode, make sure all opened windows/tabs are opened in
foreground.
This seems wrong.  In Kiosk mode we shouldn't _have_ any tabs, because the user
can't change between them, close them, etc.

Instead shouldn't this be CURRENT_TAB?

BTW, this is not going to prevent popups.  Note how this function doesn't even
handle NEW_POPUP.  That is done much deeper, in the glue layer; you'd need to
change how the various WebKit oracle functions return.

http://codereview.chromium.org/244003/diff/5002/2005
File chrome/browser/browser.h (right):

http://codereview.chromium.org/244003/diff/5002/2005#newcode801
Line 801: // Cache the result if the browser is running in kiosk mode.
Nit: Grammar; this should just be "True if the browser is running in kiosk
mode."

http://codereview.chromium.org/244003/diff/5002/2006
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/244003/diff/5002/2006#newcode556
Line 556: if (command_line_.HasSwitch(switches::kKioskMode)) {
Nit: Maybe this should instead be inside Browser itself?

Also, no {}

http://codereview.chromium.org/244003/diff/5002/2007
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/244003/diff/5002/2007#newcode59
Line 59: is_kiosk_(CommandLine::ForCurrentProcess()->HasSwitch(
Doesn't this run in the renderer process, not the browser process?  How is it
going to see the kiosk mode switch?

http://codereview.chromium.org/244003/diff/5002/2007#newcode138
Line 138: AppendMenuItem(IDS_CONTENT_CONTEXT_COPYIMAGELOCATION);
I'm confused by how you're deciding what to turn off and what to leave on.  You
left COPYLINKLOCATION and COPYEMAILADDRESS on but turned COPYIMAGE,
COPYAUDIOLOCATION and COPYVIDEOLOCATION off.  What's the pattern?

http://codereview.chromium.org/244003/diff/5002/2008
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/244003/diff/5002/2008#newcode109
Line 109: // Cache the result if the browser is running in kiosk mode.
Nit: Same grammar comment as above

[Mohamed Mansour, 2009-11-06 01:04:06]

I rebased this CL and removed all the unwanted conditions. Kiosk mode should
"only" be a fullscreen chrome with the removal of the status bar (as shown in
other browsers). That will make many vendors happy and main reason to use Chrome
as their browser of choice.

[Peter Kasting, 2009-11-06 01:19:14]

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 already in full screen kiosk mode.
Nit: Slightly more clear: "In kiosk mode we always want to be fullscreen.  When
the browser first starts we're not yet fullscreen, so let the initial toggle go
through."

http://codereview.chromium.org/244003/diff/10001/11001#newcode2640
Line 2640: if (is_kiosk_)
Nit: I'd just replace this conditional with:
return (window_ && !is_kiosk_) ? window_->GetStatusBubble() : NULL;

http://codereview.chromium.org/244003/diff/10001/11002
File chrome/browser/browser.h (right):

http://codereview.chromium.org/244003/diff/10001/11002#newcode143
Line 143: const bool is_kiosk() const { return is_kiosk_; }
Do we ever call this?

http://codereview.chromium.org/244003/diff/10001/11002#newcode824
Line 824: const bool is_kiosk_;
Why store this in a member instead of just checking the command line where we
need to know the answer?

http://codereview.chromium.org/244003/diff/10001/11003
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/244003/diff/10001/11003#newcode562
Line 562: // Toggle Fullscreen for kiosk mode.
Nit: Slightly more clear: "In kiosk mode, we want to always be fullscreen, so
switch to that now."

http://codereview.chromium.org/244003/diff/10001/11005
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/244003/diff/10001/11005#newcode693
Line 693: const wchar_t kKioskMode[] = L"kiosk";
Please fix per the comment above.  While you're here, please also fix
kEnableNewAutoFill, which clearly also didn't read the directions.  (Remember to
also adjust instances of this in other files to match the order too)

[Mohamed Mansour, 2009-11-06 02:36:56]

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 screen if its already in full screen kiosk mode.
On 2009/11/06 01:19:14, Peter Kasting wrote:
> Nit: Slightly more clear: "In kiosk mode we always want to be fullscreen. 
When
> the browser first starts we're not yet fullscreen, so let the initial toggle
go
> through."

Done.

http://codereview.chromium.org/244003/diff/10001/11001#newcode2640
Line 2640: if (is_kiosk_)
On 2009/11/06 01:19:14, Peter Kasting wrote:
> Nit: I'd just replace this conditional with:
> return (window_ && !is_kiosk_) ? window_->GetStatusBubble() : NULL;

It expands two lines, should I condense it to one.
if (... && ...)
  return ...
else 
  ...

I think the approach done here is cleaner.

http://codereview.chromium.org/244003/diff/10001/11002
File chrome/browser/browser.h (right):

http://codereview.chromium.org/244003/diff/10001/11002#newcode143
Line 143: const bool is_kiosk() const { return is_kiosk_; }
On 2009/11/06 01:19:14, Peter Kasting wrote:
> Do we ever call this?

Done.

http://codereview.chromium.org/244003/diff/10001/11002#newcode824
Line 824: const bool is_kiosk_;
On 2009/11/06 01:19:14, Peter Kasting wrote:
> Why store this in a member instead of just checking the command line where we
> need to know the answer?

Done. Alright removed, I thought it is faster to store it then rechecking the
command line, but since its a hashmap, it should be fine.

http://codereview.chromium.org/244003/diff/10001/11003
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/244003/diff/10001/11003#newcode562
Line 562: // Toggle Fullscreen for kiosk mode.
On 2009/11/06 01:19:14, Peter Kasting wrote:
> Nit: Slightly more clear: "In kiosk mode, we want to always be fullscreen, so
> switch to that now."

Done.

http://codereview.chromium.org/244003/diff/10001/11005
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/244003/diff/10001/11005#newcode693
Line 693: const wchar_t kKioskMode[] = L"kiosk";
On 2009/11/06 01:19:14, Peter Kasting wrote:
> Please fix per the comment above.  While you're here, please also fix
> kEnableNewAutoFill, which clearly also didn't read the directions.  (Remember
to
> also adjust instances of this in other files to match the order too)

Done.

[M-A Ruel, 2009-11-06 02:39:59]

still need unit tests.

[Peter Kasting, 2009-11-06 02:56:50]

On 2009/11/06 02:39:59, Marc-Antoine Ruel wrote:
> still need unit tests.

What can be tested here?

[M-A Ruel, 2009-11-06 03:01:13]

On 2009/11/06 02:56:50, Peter Kasting wrote:
> On 2009/11/06 02:39:59, Marc-Antoine Ruel wrote:
> > still need unit tests.
> 
> What can be tested here?

That the expected behavior doesn't change. No dev will run with this flag.
Probably nobody in dev channel either. As an example; it'll be too late in beta
channel to realize that the fullscreen dropdown toolbar is reappearing even in
kiosk mode. So I think each "hack" should be unit tested.

[Peter Kasting, 2009-11-06 03:16:17]

On 2009/11/06 03:01:13, Marc-Antoine Ruel wrote:
> As an example; it'll be too late in beta
> channel to realize that the fullscreen dropdown toolbar is reappearing even in
> kiosk mode. So I think each "hack" should be unit tested.

What I'm saying is, how do you propose to unittest that when you run Chrome with
--kiosk, no fullscreen dropdown appears?

[Evan Stade, 2009-11-06 03:24:07]

ui_controls + view id

[Mohamed Mansour, 2009-11-06 05:26:24]

Here you go. A test that goes with it and it works. (tested locally)

[Peter Kasting, 2009-11-06 05:55:16]

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 1116: if (browser) {
Nit: No {}

http://codereview.chromium.org/244003/diff/15003/14012
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/244003/diff/15003/14012#newcode465
Line 465: if (!is_fullscreen) {
Nit: I wouldn't worry about this.  If you really want to check it, I'd just
DCHECK(is_fullscreen) atop the function.

http://codereview.chromium.org/244003/diff/15003/14013
File chrome/test/automation/browser_proxy.h (right):

http://codereview.chromium.org/244003/diff/15003/14013#newcode207
Line 207: // Fills |*is_fullscreen| if the browser is currently in fullscreen
Nit: "Sets |is_fullscreen| to whether the browser is currently in fullscreen
mode" might be a little clearer

[M-A Ruel, 2009-11-06 13:18:15]

+ 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) {
On 2009/11/06 05:55:17, Peter Kasting wrote:
> Nit: No {}

I disagree. Keep the {} :)

http://codereview.chromium.org/244003/diff/15003/14005
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/244003/diff/15003/14005#newcode918
Line 918: // In kiosk mode, we always want to be fullscreen. When the browser
first
You should add a comment at the function declaration too.

[Mohamed Mansour, 2009-11-06 14:40:40]

On 2009/11/06 05:55:16, Peter Kasting wrote:
> Any way to test the lack of status bubble?

Here you go, now it tests if the status bubble is not there.

[M-A Ruel, 2009-11-06 14:44:17]

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 be fullscreen. When the browser
first
Still think there should be a comment in browser.h

http://codereview.chromium.org/244003/diff/12004/12009
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/244003/diff/12004/12009#newcode311
Line 311: browser->IsFullscreenBubbleVisible(&is_visible);
ASSERT_TRUE(browser->IsFullscreenBubbleVisible(&is_visible));

[Peter Kasting, 2009-11-06 19:25:44]

On 2009/11/06 13:18:15, Marc-Antoine Ruel wrote:
> On 2009/11/06 05:55:17, Peter Kasting wrote:
> > Nit: No {}
> 
> I disagree. Keep the {} :)

File style here is no {}, and team style is nearly all that way too.  There is
some code in views that differs but otherwise we are largely consistent about
{}.

[Mohamed Mansour, 2009-11-07 06:15:26]

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 wrote:
> Nit: No {}

Done.

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 be fullscreen. When the browser
first
On 2009/11/06 14:44:17, Marc-Antoine Ruel wrote:
> Still think there should be a comment in browser.h

Done.

http://codereview.chromium.org/244003/diff/12004/12009
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/244003/diff/12004/12009#newcode311
Line 311: browser->IsFullscreenBubbleVisible(&is_visible);
On 2009/11/06 14:44:17, Marc-Antoine Ruel wrote:
> ASSERT_TRUE(browser->IsFullscreenBubbleVisible(&is_visible));

Done.

[M-A Ruel, 2009-11-07 13:33:02]

compile failure

[Mohamed Mansour, 2009-11-07 15:11:49]

Fixed unit_tests compilation.

[Mohamed Mansour, 2009-11-07 15:22:17]

But ui_tests for kiosk fail on the mac :(

[M-A Ruel, 2009-11-07 21:01:21]

On 2009/11/07 15:22:17, Mohamed Mansour wrote:
> But ui_tests for kiosk fail on the mac :(

I can take a look at this since I know you don't have a mac but I can only check
in a few days.

[Mohamed Mansour, 2009-11-07 23:26:23]

Don't worry about it, I would have to ifdef the test out because pinkerton
said they disabled fullscreen on the mac.

 - Mohamed Mansour


On Sat, Nov 7, 2009 at 4:01 PM, <maruel@chromium.org> wrote:

> On 2009/11/07 15:22:17, Mohamed Mansour wrote:
>
>> But ui_tests for kiosk fail on the mac :(
>>
>
> I can take a look at this since I know you don't have a mac but I can only
> check
> in a few days.
>
>
> http://codereview.chromium.org/244003
>

[M-A Ruel, 2009-11-08 00:29:16]

On 2009/11/07 23:26:23, Mohamed Mansour wrote:
> Don't worry about it, I would have to ifdef the test out because pinkerton
> said they disabled fullscreen on the mac.

Oh right forgot about that. Can you #ifdef !defined(OS_MAC) for
kEnableNewAutoFill to be clear the functionality is disabled on Mac?

[Mohamed Mansour, 2009-11-08 02:42:31]

Hrmm, I ifdefd MAC for the kiosk test, and its still somehow reading it. Can any
of you take a look when you have a chance please. Other tests started failing as
well :/ I think it is from Ben's recent change that made the try bots unhappy.

[M-A Ruel, 2009-11-08 16:42:51]

On 2009/11/08 02:42:31, Mohamed Mansour wrote:
> Hrmm, I ifdefd MAC for the kiosk test, and its still somehow reading it. Can
any
> of you take a look when you have a chance please. Other tests started failing
as
> well :/ I think it is from Ben's recent change that made the try bots unhappy.

You need:

#include "build/build_config.h"

[Mohamed Mansour, 2009-11-08 18:09:56]

No go :( It is still running that test on the Mac even after I included
build/build_config.h

 - Mohamed Mansour


On Sun, Nov 8, 2009 at 11:42 AM, <maruel@chromium.org> wrote:

> On 2009/11/08 02:42:31, Mohamed Mansour wrote:
>
>> Hrmm, I ifdefd MAC for the kiosk test, and its still somehow reading it.
>> Can
>>
> any
>
>> of you take a look when you have a chance please. Other tests started
>> failing
>>
> as
>
>> well :/ I think it is from Ben's recent change that made the try bots
>> unhappy.
>>
>
> You need:
>
> #include "build/build_config.h"
>
>
> http://codereview.chromium.org/244003
>

[Mohamed Mansour, 2009-11-08 19:24:54]

Finally its fixed now, thanks maruel! I will be away next week with no Internet,
going to a Smart Meters conference in Toronto.

[M-A Ruel, 2009-11-08 20:22:48]

lgtm