Remove "set wallpaper..." option in system context menu when guest logged in

Remove "set wallpaper..." option in system context menu when guest logged in


BUG=120763
TEST=Log in as guest user, click right mouse button on wallpaper, there should
be no "set wallpaper..." options in the popup context menu.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131806

[bshe, 2012-04-04 23:46:18]

Hi Scott.

Since you wrote the code of work space context menu, could you please take a
look at this small CL?

Thanks!

[sky, 2012-04-05 03:48:14]

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:61: // TODO(sky): move this. Since this menu is
now specific to the background it
Since this is targeted at m20, can you clean this up and remove the context menu
from workspacecontroller.

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:67: if
(!Shell::GetInstance()->user_wallpaper_delegate()->IsLoggedInAsGuest()) {
If this is false, you'll show an empty menu. In this case don't do anything.

[bshe, 2012-04-05 15:42:31]

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:61: // TODO(sky): move this. Since this menu is
now specific to the background it
Just to clarify, do you want to remove this in workspace completely? Do we have
plan to add menu for workspace in the future? I plan to move it into desktop
background controller. Do you have any other suggestion? Thanks!

On 2012/04/05 03:48:14, sky wrote:
> Since this is targeted at m20, can you clean this up and remove the context
menu
> from workspacecontroller.

[sky, 2012-04-05 16:44:54]

On Thu, Apr 5, 2012 at 8:42 AM,  <bshe@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
> File ash/wm/workspace_controller.cc (right):
>
>
https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
> ash/wm/workspace_controller.cc:61: // TODO(sky): move this. Since this
> menu is now specific to the background it
> Just to clarify, do you want to remove this in workspace completely?

Yes.

> Do
> we have plan to add menu for workspace in the future?

No.

> I plan to move it
> into desktop background controller. Do you have any other suggestion?

Maybe it should be a standalone class that the shell creates. That way
it can call into whatever it needs to and doesn't get tied to a
specific component that happens to currently want to use it.

  -Scott

> Thanks!
>
>
> On 2012/04/05 03:48:14, sky wrote:
>>
>> Since this is targeted at m20, can you clean this up and remove the
>
> context menu
>>
>> from workspacecontroller.
>
>
> https://chromiumcodereview.appspot.com/9960024/

[bshe, 2012-04-10 18:27:42]

Hi Scott.

Could you please take another look at this CL? I add a ShellContextMenu(happy to
tweak name) class to shell and removed the menu in WorkspaceController.

There is one thing I am not quite sure. It seems the ShowMenu code is behind
!OS_MACOSX flag in WorkspaceController. I am not sure if it is specific to
workspace menu. I just removed it in ShellContextMenu as this is a general menu
class now. But I might be wrong.

Also, ash/shell.cc delta 1 includes a merge delta. The only thing I modify in
this file is adding the ShellContextMenu class and ShowMenu through the class.

Thanks!

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:61: // TODO(sky): move this. Since this menu is
now specific to the background it
On 2012/04/05 03:48:14, sky wrote:
> Since this is targeted at m20, can you clean this up and remove the context
menu
> from workspacecontroller.

Done.

https://chromiumcodereview.appspot.com/9960024/diff/1/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:67: if
(!Shell::GetInstance()->user_wallpaper_delegate()->IsLoggedInAsGuest()) {
On 2012/04/05 03:48:14, sky wrote:
> If this is false, you'll show an empty menu. In this case don't do anything.

Done.

[sky, 2012-04-10 19:55:45]

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.h (right):

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/desktop_background...
ash/desktop_background/desktop_background_controller.h:28: virtual bool
IsLoggedInAsGuest() = 0;
This is really generic. How about making it explicit: CanOpenSetWallpaperPage,
or maybe the delegate should be NULL if logged in as a guest.

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/shell_context_menu.h
File ash/shell_context_menu.h (right):

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/shell_context_menu...
ash/shell_context_menu.h:6: #define _ASH_SHELL_CONTEXT_MENU_H_
remove leading _

[bshe, 2012-04-10 20:46:14]

Done. Thanks!

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.h (right):

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/desktop_background...
ash/desktop_background/desktop_background_controller.h:28: virtual bool
IsLoggedInAsGuest() = 0;
It looks like guest user uses the delegate to get default wallpaper index. So I
changed the function name to CanOpenSetWallpaperPage rather than set delegate to
NULL. Seems like a better option.

On 2012/04/10 19:55:45, sky wrote:
> This is really generic. How about making it explicit: CanOpenSetWallpaperPage,
> or maybe the delegate should be NULL if logged in as a guest.

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/shell_context_menu.h
File ash/shell_context_menu.h (right):

https://chromiumcodereview.appspot.com/9960024/diff/10/ash/shell_context_menu...
ash/shell_context_menu.h:6: #define _ASH_SHELL_CONTEXT_MENU_H_
On 2012/04/10 19:55:45, sky wrote:
> remove leading _

Done.

[sky, 2012-04-10 21:52:35]

LGTM

[commit-bot: I haz the power, 2012-04-11 14:05:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/9960024/18001

[commit-bot: I haz the power, 2012-04-11 16:56:35]

Try job failure for 9960024-18001 (retry) on mac_rel for steps "ui_tests,
browser_tests, unit_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...